STACK_ALLOCATED()
316 views
Skip to first unread message
Stefan Zager
Jan 27, 2022, 1:18:28 PM1/27/22
to Chromium-dev
Hey all,
I am interested in porting the blink-specific STACK_ALLOCATED() macro to base/. I have a specific non-blink use case in mind, but I also think it could be generally useful. This would also involve porting the enforcement mechanism from blink-gc-plugin to the chromium style plugin.
Does this sound like a good idea?
dan...@chromium.org
Jan 27, 2022, 1:20:02 PM1/27/22
to Stefan Zager, Chromium-dev
Could you explain the use case?
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHOQ7J9Zng8Z1BxHPh9jnABi%2B38x30pkvSODL7MnzmydtUU%3DXQ%40mail.gmail.com.
Stefan Zager
Jan 27, 2022, 1:35:40 PM1/27/22
to dan...@chromium.org, Stefan Zager, Chromium-dev
On Thu, Jan 27, 2022 at 10:19 AM <dan...@chromium.org> wrote:
Could you explain the use case?
This class is the point of entry to several tree structures that describe compositing information, and all access to property tree data should go through PropertyTrees (which is owned by cc::LayerTreeHost and cc::LayerTreeHostImpl).
PropertyTrees, and the types it contains, currently have virtually no protections against misuse. That is an obstacle to the project I am working on: Non-Blocking Commit. I can lock down access to member fields (which are almost entirely public), but what I can't do is prevent other code from holding persistent, non-scoped pointers to TransformPropertyTree, TransformNode, etc.
For my project, it's essential to prevent persistent (i.e., across event loop task scope) references to these types outside of PropertyTrees, and force all access to go through PropertyTrees. To do this, I would like to convert the API surface of PropertyTrees and its subtypes to use something like these wrappers:
template <typename T> class ScopedConstValue;
template <typename T>
class ScopedValue {
template <typename T>
class ScopedValue {
STACK_ALLOCATED();
friend class ScopedConstValue<T>;
public:
void* operator new(size_t) = delete;
void* operator new(size_t, void*) = delete;
void* operator new[](size_t) = delete;
void* operator new[](size_t, void*) = delete;
explicit ScopedValue(T* value = nullptr) : value_(value) {}
explicit ScopedValue(const ScopedValue& sv) : value_(sv.value) {}
T* operator->() const { return value_; }
explicit operator bool() const { return value_ != nullptr; }
void operator=(T* value) { value_ = value; }
void operator=(const ScopedValue& other) { value_ = other.value_; }
private:
T* value_;
};
template <typename T>
class ScopedConstValue {
friend class ScopedConstValue<T>;
public:
void* operator new(size_t) = delete;
void* operator new(size_t, void*) = delete;
void* operator new[](size_t) = delete;
void* operator new[](size_t, void*) = delete;
explicit ScopedValue(T* value = nullptr) : value_(value) {}
explicit ScopedValue(const ScopedValue& sv) : value_(sv.value) {}
T* operator->() const { return value_; }
explicit operator bool() const { return value_ != nullptr; }
void operator=(T* value) { value_ = value; }
void operator=(const ScopedValue& other) { value_ = other.value_; }
private:
T* value_;
};
template <typename T>
class ScopedConstValue {
STACK_ALLOCATED();
public:
void* operator new(size_t) = delete;
void* operator new(size_t, void*) = delete;
void* operator new[](size_t) = delete;
void* operator new[](size_t, void*) = delete;
explicit ScopedConstValue(const T* value = nullptr) : value_(value) {}
explicit ScopedConstValue(const ScopedConstValue& scv) : value_(scv.value_) {}
explicit ScopedConstValue(const ScopedValue<T>& sv) : value_(sv.value_) {}
const T* operator->() const { return value_; }
explicit operator bool() const { return value_ != nullptr; }
void operator=(const T* value) { value_ = value; }
void operator=(const ScopedConstValue& scv) { value_ = scv.value_; }
void operator=(const ScopedValue<T>& sv) { value_ = sv.value_; }
private:
const T* value_;
};
void* operator new(size_t) = delete;
void* operator new(size_t, void*) = delete;
void* operator new[](size_t) = delete;
void* operator new[](size_t, void*) = delete;
explicit ScopedConstValue(const T* value = nullptr) : value_(value) {}
explicit ScopedConstValue(const ScopedConstValue& scv) : value_(scv.value_) {}
explicit ScopedConstValue(const ScopedValue<T>& sv) : value_(sv.value_) {}
const T* operator->() const { return value_; }
explicit operator bool() const { return value_ != nullptr; }
void operator=(const T* value) { value_ = value; }
void operator=(const ScopedConstValue& scv) { value_ = scv.value_; }
void operator=(const ScopedValue<T>& sv) { value_ = sv.value_; }
private:
const T* value_;
};
Because these wrappers provide no way to directly access the underlying T*, they should effectively prevent illegal persistent refs.
Stefan Zager
Jan 27, 2022, 1:47:55 PM1/27/22
to Stefan Zager, dan...@chromium.org, Chromium-dev
On Thu, Jan 27, 2022 at 10:34 AM Stefan Zager <sza...@chromium.org> wrote:
On Thu, Jan 27, 2022 at 10:19 AM <dan...@chromium.org> wrote:
PropertyTrees, and the types it contains, currently have virtually no protections against misuse. That is an obstacle to the project I am working on: Non-Blocking Commit.
To be a bit more explicit, I'd like to do this:
class PropertyTrees {
public:
// PropertyTrees is owned by synchronizer, and must not outlive it
PropertyTrees(const& ProtectedSequenceSynchronizer synchronizer) : synchronizer_(synchronizer) {}
void* operator new(size_t) = delete;
void* operator new(size_t, void*) = delete;
void* operator new[](size_t) = delete;
void* operator new[](size_t, void*) = delete;
void* operator new(size_t, void*) = delete;
void* operator new[](size_t) = delete;
void* operator new[](size_t, void*) = delete;
ScopedConstValue<TransformTree> transform_tree() {
return ScopedValue<TransformTree>(transform_tree_.Read(synchronizer_));
} ScopedValue<TransformTree> transform_tree_mutable() {
return ScopedValue<TransformTree>(transform_tree_.Write(synchronizer_));
}
private:
const ProtectedSequenceSynchronizer& synchronizer_;
TransformTree transform_tree_;
};
ProtectedSequenceSynchronizer is the main enforcement mechanism to ensure thread safety for non-blocking commit.
Stefan Zager
Jan 27, 2022, 1:49:29 PM1/27/22
to Stefan Zager, dan...@chromium.org, Chromium-dev
Small mistake in my code...
class PropertyTrees {
...
private:
const ProtectedSequenceSynchronizer& synchronizer_;
ProtectedSequenceReadable<TransformTree> transform_tree_;
};
dan...@chromium.org
Jan 27, 2022, 2:44:03 PM1/27/22
to Stefan Zager, Chromium-dev
Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?
Stefan Zager
Jan 27, 2022, 2:49:38 PM1/27/22
to dan...@chromium.org, Stefan Zager, Chromium-dev
On Thu, Jan 27, 2022 at 11:43 AM <dan...@chromium.org> wrote:
Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?
STACK_ALLOCATED() prevents this:
class Dangerous {
STACK_ALLOCATED();
};
class LongLived {
Dangerous dangerous_;
};
I'm not aware of another mechanism to prevent this; simply deleting 'operator new' is not sufficient.
Stefan Zager
Jan 27, 2022, 2:51:46 PM1/27/22
to Stefan Zager, dan...@chromium.org, Chromium-dev
On Thu, Jan 27, 2022 at 11:48 AM Stefan Zager <sza...@chromium.org> wrote:
On Thu, Jan 27, 2022 at 11:43 AM <dan...@chromium.org> wrote:Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?STACK_ALLOCATED() prevents this:class Dangerous {STACK_ALLOCATED();};class LongLived {Dangerous dangerous_;};I'm not aware of another mechanism to prevent this; simply deleting 'operator new' is not sufficient.
Here's how that is enforced by blink_gc_plugin:
dan...@chromium.org
Jan 27, 2022, 2:55:11 PM1/27/22
to Stefan Zager, Chromium-dev
On Thu, Jan 27, 2022 at 2:50 PM Stefan Zager <sza...@chromium.org> wrote:
On Thu, Jan 27, 2022 at 11:48 AM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:43 AM <dan...@chromium.org> wrote:Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?STACK_ALLOCATED() prevents this:class Dangerous {STACK_ALLOCATED();};class LongLived {Dangerous dangerous_;};I'm not aware of another mechanism to prevent this; simply deleting 'operator new' is not sufficient.Here's how that is enforced by blink_gc_plugin:
Thanks, I was thinking of ANNOTATE_STACK_ALLOCATED, it turns out.
So this prevents composition of STACK_ALLOCATED into non-STACK_ALLOCATED. There's nothing blink or gc specific about that, so moving it out of blink seems okay to me.
But what about ANNOTATE_STACK_ALLOCATED? There's no comments about these but do they need to go together?
Aside: Does it also prevent inheritance from STACK_ALLOCATED?
Stefan Zager
Jan 27, 2022, 3:27:00 PM1/27/22
to dan...@chromium.org, Stefan Zager, Chromium-dev
On Thu, Jan 27, 2022 at 11:54 AM <dan...@chromium.org> wrote:
On Thu, Jan 27, 2022 at 2:50 PM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:48 AM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:43 AM <dan...@chromium.org> wrote:Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?STACK_ALLOCATED() prevents this:class Dangerous {STACK_ALLOCATED();};class LongLived {Dangerous dangerous_;};I'm not aware of another mechanism to prevent this; simply deleting 'operator new' is not sufficient.Here's how that is enforced by blink_gc_plugin:Thanks, I was thinking of ANNOTATE_STACK_ALLOCATED, it turns out.So this prevents composition of STACK_ALLOCATED into non-STACK_ALLOCATED. There's nothing blink or gc specific about that, so moving it out of blink seems okay to me.But what about ANNOTATE_STACK_ALLOCATED? There's no comments about these but do they need to go together?
ANNOTATE_STACK_ALLOCATED exists only to support the enforcement of STACK_ALLOCATED, by applying a custom __attribute__ that blink_gc_plugin looks for. So yes, I think they need to move together. Presumably we'd choose a more generic label for the __attribute__, and modify blink_gc_plugin to search for the new label.
Aside: Does it also prevent inheritance from STACK_ALLOCATED?
I don't know if the current mechanism prevents that, but it seems like something that could be enforced by the plugin, if it's not already.
dan...@chromium.org
Jan 27, 2022, 4:03:02 PM1/27/22
to Stefan Zager, Chromium-dev
On Thu, Jan 27, 2022 at 3:23 PM Stefan Zager <sza...@chromium.org> wrote:
On Thu, Jan 27, 2022 at 11:54 AM <dan...@chromium.org> wrote:On Thu, Jan 27, 2022 at 2:50 PM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:48 AM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:43 AM <dan...@chromium.org> wrote:Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?STACK_ALLOCATED() prevents this:class Dangerous {STACK_ALLOCATED();};class LongLived {Dangerous dangerous_;};I'm not aware of another mechanism to prevent this; simply deleting 'operator new' is not sufficient.Here's how that is enforced by blink_gc_plugin:Thanks, I was thinking of ANNOTATE_STACK_ALLOCATED, it turns out.So this prevents composition of STACK_ALLOCATED into non-STACK_ALLOCATED. There's nothing blink or gc specific about that, so moving it out of blink seems okay to me.But what about ANNOTATE_STACK_ALLOCATED? There's no comments about these but do they need to go together?ANNOTATE_STACK_ALLOCATED exists only to support the enforcement of STACK_ALLOCATED, by applying a custom __attribute__ that blink_gc_plugin looks for. So yes, I think they need to move together. Presumably we'd choose a more generic label for the __attribute__, and modify blink_gc_plugin to search for the new label.
We may be getting lost in details, but STACK_ANNOTATED puts a type alias inside the class that the plugin looks for: https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/blink_gc_plugin/RecordInfo.cpp;drc=1044dd49d7d78fba1e11a1cdb27e4e0f6d193ef9;l=261
It looks for that annotation on methods, when the type alias isn't found: https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/blink_gc_plugin/RecordInfo.cpp;l=278
I now see STACK_ALLOCATED does both.. but I don't understand why.
Stefan Zager
Jan 27, 2022, 4:30:14 PM1/27/22
to dan...@chromium.org, Stefan Zager, Chromium-dev
On Thu, Jan 27, 2022 at 1:02 PM <dan...@chromium.org> wrote:
On Thu, Jan 27, 2022 at 3:23 PM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:54 AM <dan...@chromium.org> wrote:On Thu, Jan 27, 2022 at 2:50 PM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:48 AM Stefan Zager <sza...@chromium.org> wrote:On Thu, Jan 27, 2022 at 11:43 AM <dan...@chromium.org> wrote:Dodging the larger question about memory safety and lifetimes here, my understanding of the STACK_ALLOCATED macro is to tell the GC plugin that the class doesn't need a Trace() method. But the Trace() requirement is only needed for blink types, and these aren't blink types. Could you clarify what is preventing you from doing this without STACK_ALLOCATED?STACK_ALLOCATED() prevents this:class Dangerous {STACK_ALLOCATED();};class LongLived {Dangerous dangerous_;};I'm not aware of another mechanism to prevent this; simply deleting 'operator new' is not sufficient.Here's how that is enforced by blink_gc_plugin:Thanks, I was thinking of ANNOTATE_STACK_ALLOCATED, it turns out.So this prevents composition of STACK_ALLOCATED into non-STACK_ALLOCATED. There's nothing blink or gc specific about that, so moving it out of blink seems okay to me.But what about ANNOTATE_STACK_ALLOCATED? There's no comments about these but do they need to go together?ANNOTATE_STACK_ALLOCATED exists only to support the enforcement of STACK_ALLOCATED, by applying a custom __attribute__ that blink_gc_plugin looks for. So yes, I think they need to move together. Presumably we'd choose a more generic label for the __attribute__, and modify blink_gc_plugin to search for the new label.We may be getting lost in details, but STACK_ANNOTATED puts a type alias inside the class that the plugin looks for: https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/blink_gc_plugin/RecordInfo.cpp;drc=1044dd49d7d78fba1e11a1cdb27e4e0f6d193ef9;l=261It looks for that annotation on methods, when the type alias isn't found: https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/blink_gc_plugin/RecordInfo.cpp;l=278I now see STACK_ALLOCATED does both.. but I don't understand why.
Just a guess, but I think maybe IsStackAllocatedTypeMarker is for v8:
... and "blink_stack_allocated" is for blink.
Kentaro Hara
Jan 27, 2022, 7:43:12 PM1/27/22
to Chromium-dev, szager, Chromium-dev, danakj
+1 to moving STACK_ALLOCATED() to //base/.
However, the main benefit of STACK_ALLOCATED() is to prevent programming errors about Oilpan (though I agree that there are non-Oilpan-related benefits as Stefan pointed out). I think it's great to start using STACK_ALLOCATED() in some key classes in Chromium but I'm not sure if it's worth introducing STACK_ALLOCATED() throughout the existing codebase proactively.
Alan Cutter
Jan 27, 2022, 8:19:01 PM1/27/22
to Chromium-dev, Kentaro Hara, szager, Chromium-dev, danakj
The AppRegistryCache::ForOneApp(const std::string& app_id, FunctionType f) interface could use STACK_ALLOCATED() to protect against dangling references. This would enable it to return a value instead of taking a callback param.
Michael Lippautz
Jan 28, 2022, 3:14:46 AM1/28/22
to alanc...@chromium.org, Chromium-dev, Kentaro Hara, szager, danakj
On Fri, Jan 28, 2022 at 2:19 AM Alan Cutter <alanc...@chromium.org> wrote:
The AppRegistryCache::ForOneApp(const std::string& app_id, FunctionType f) interface could use STACK_ALLOCATED() to protect against dangling references. This would enable it to return a value instead of taking a callback param.On Friday, 28 January 2022 at 11:43:12 am UTC+11 Kentaro Hara wrote:+1 to moving STACK_ALLOCATED() to //base/.However, the main benefit of STACK_ALLOCATED() is to prevent programming errors about Oilpan (though I agree that there are non-Oilpan-related benefits as Stefan pointed out). I think it's great to start using STACK_ALLOCATED() in some key classes in Chromium but I'm not sure if it's worth introducing STACK_ALLOCATED() throughout the existing codebase proactively.
+1
I think it can be generally useful as already pointed out.
Caveat right now is that the macro is mostly used for checking some things around Oilpan. So it likely does not yet check/enforce the things you would want to see in general C++. This could obviously be improved though.
I'd suggest picking up the implementation based on type markers (there's also an implementation based on annotations) so that things can be also checked from regular C++ using a type trait.
--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/c33403a7-e157-404a-9d35-ba3274653604n%40chromium.org.
Avi Drissman
Feb 2, 2022, 4:13:59 PM2/2/22
to mlip...@chromium.org, Alan Cutter, Chromium-dev, Kentaro Hara, szager, danakj
FYI the Mac team has ScopedNSAutoreleasePool that must be strictly nested. We would want to enforce people only using it on the stack, but because we cannot do so, there are many uses of it as a member variable that are dangerous.
We would definitely adopt any STACK_ALLOCATED() solution that became available.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAH%2BmL5B61g%3DEnK2jtE3ucQ0n4uhBmSypy37M4NV%3DiA%3D57BTuqQ%40mail.gmail.com.
Reply all
Reply to author
Forward
0 new messages