STACK_ALLOCATED() macro now available chromium-wide

1,360 views
Skip to first unread message

Stefan Zager

unread,
Mar 14, 2023, 12:33:58 AM3/14/23
to Chromium-dev
tl;dr The STACK_ALLOCATED() macro, previously limited to blink, can now be used anywhere in chromium. It is enforced at compile time via the chromium clang plugin.

This commit defines STACK_ALLOCATED() in //base/memory/stack_allocated.h.

The STACK_ALLOCATED() macro, when added to a class definition, asserts that instances of the class may not be created on the heap, nor used as a member variable in a class which is not itself STACK_ALLOCATED(). References (&) and pointers (dumb and smart) to a STACK_ALLOCATED() type are also forbidden as member variables of non-STACK_ALLOCATED() classes.

STACK_ALLOCATED() was originally added to blink to allow classes that are only ever instantiated on the call stack to safely contain raw pointers to garbage-collected objects, without the requirement of tracing the on-stack objects. However there are other scenarios, unrelated to garbage collection, where it may be useful. In particular, any situation where a value is only guaranteed to be valid during the scope of the currently-running task may benefit from being wrapped in a STACK_ALLOCATED() class instance.

cf. this thread for a lengthier discussion/explanation.

Example usage:

class EphemeralWrapper {
  STACK_ALLOCATED();
 public:
  EphemeralWrapper(Ephemeral* e) : e_(e) {}
  EphemeralWrapper(const EphemeralWrapper&) = default;
 private:
  Ephemeral* e_;  // Not guaranteed to be valid beyond scope of the current task.
};

Ephemeral* GetEphemeral() {
  // Sensitive lifetime management of Ephemerals happens in here.
}

void DoSomething(const EphemeralWrapper& ew) {
  EphemeralWrapper ew2(GetEphemeral());  // OK
  EphemeralWrapper ew3(ew);  // OK
  EphemeralWrapper* ew4 = new EphemeralWrapper(ew);  // ERROR
}

class ShortLived {
  STACK_ALLOCATED();
 private:
  EphemeralWrapper ew;  // OK, because ShortLived is STACK_ALLOCATED().
};

class LongLived {
 private:
  EphemeralWrapper ew_  // ERROR
  EphemeralWrapper* ew_p_;  // ERROR
  EphemeralWrapper& ew_ref_;  // ERROR
  scoped_refptr<EphemeralWrapper> ew_refptr_;  // ERROR
};


Noteworthy implementation details...

The enforcement of STACK_ALLOCATED() occurs in the chromium clang plugin at compile time. The implementation of the check is independent from the preexisting blink gc plugin, which has not changed. The two plugins do not share any code, and the chromium check is a bit stricter than the blink check.

STACK_ALLOCATED() is a voluntary restriction, and it's not immune to deliberate evasion, e.g., via reinterpret_cast<>. If it's necessary to use a STACK_ALLOCATED() class in a non-conformant way, there is a back door to allow for one-off exemptions from enforcement, via the IGNORE_STACK_ALLOCATED(bug_or_reason) macro.

Avi Drissman

unread,
Mar 14, 2023, 12:52:38 AM3/14/23
to Stefan Zager, Chromium-dev
To clarify, the exemption macro is STACK_ALLOCATED_IGNORE?

Stefan Zager

unread,
Mar 14, 2023, 12:57:55 AM3/14/23
to Avi Drissman, Stefan Zager, Chromium-dev


On Mon, Mar 13, 2023, 2:50 PM Avi Drissman <[email protected]> wrote:
To clarify, the exemption macro is STACK_ALLOCATED_IGNORE?

Right you are, my mistake.

Zhanbang He

unread,
Mar 14, 2023, 7:39:19 AM3/14/23
to Chromium-dev, Stefan Zager, Chromium-dev, Avi Drissman
Does this line of code have any significance?
`using IsStackAllocatedTypeMarker [[maybe_unused]] = int;`

Stefan Zager

unread,
Mar 14, 2023, 8:58:57 AM3/14/23
to Zhanbang He, Chromium-dev, Stefan Zager, Avi Drissman
On Mon, Mar 13, 2023 at 9:39 PM Zhanbang He <[email protected]> wrote:
Does this line of code have any significance?
`using IsStackAllocatedTypeMarker [[maybe_unused]] = int;`

Yes, that is the basis of the enforcement mechanism in the clang plugin.

Bartek Nowierski

unread,
Mar 17, 2023, 12:02:21 PM3/17/23
to Chromium-dev, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He
Drive-by comment... Apparently you can have a STACK_ALLOCATED class in std::vector, which I presume allocates those on the heap. Dunno if it's an issue.

Stefan Zager

unread,
Mar 17, 2023, 7:37:19 PM3/17/23
to Bartek Nowierski, Chromium-dev, Stefan Zager, Avi Drissman, Zhanbang He
On Fri, Mar 17, 2023 at 2:02 AM Bartek Nowierski <[email protected]> wrote:
Drive-by comment... Apparently you can have a STACK_ALLOCATED class in std::vector, which I presume allocates those on the heap. Dunno if it's an issue.

That's a bug! Thanks for finding it.

Bartek Nowierski

unread,
Apr 25, 2023, 6:18:48 AM4/25/23
to Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Bartek Nowierski, Keishi Hattori
Is it a bug with STACK_ALLOCATED or std::vector? I kinda wonder what (if anything) can be done about this?

Thanks,
Bartek

Xianzhu Wang

unread,
Mar 20, 2024, 11:28:42 PM3/20/24
to [email protected], Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori
Currently the concept of STACK_ALLOCATED() is about both life scope and memory allocation. The current clang plugin checks only for the former, which effectively makes STACK_ALLOCATED() actually means STACK_SCOPED(). With this interpretation, allowing std::vector<StackAllocatedType> is probably not a bug but a useful feature :)

However, cppgc (blink GC) does require STACK_ALLOCATED() to be allocated from the native stack for correct GC marking. Vector<StackAllocatedType> is forbidden by static_asserts in blink container types.

Thus there is a divergence between std::vector<StackAllocatedType> (allowed, bug or feature) and Vector<StackAllocatedType> (disallowed).

How about renaming STACK_ALLOCATED() to STACK_SCOPED() for chromium, while keeping the blink version of STACK_ALLOCATED() which still requires memory allocation from native stack?

I think one advantage of STACK_SCOPED() is the flexibility of algorithm implementation. For example, we have a recursive algorithm using some STACK_ALLOCATED() data. Someday we want to modify the algorithm to be non-recursive with a vector as the stack. If STACK_ALLOCATED() requires native stack allocation, we will have to remove STACK_ALLOCATED() from the data types and lose the constraints for short-livedness. In a bad scenario, the removal may require removal of STACK_ALLOCATED() from other types and adding raw_ptrs even though the lifespan of the raw pointers haven't changed. In contrast, STACK_SCOPED() will still work with the non-recursive algorithm.







--
--
Chromium Developers mailing list: [email protected]
View archives, change email options, or unsubscribe:
https://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 [email protected].
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAC3g16icBJ86S%3DKifT66n%3DipPgdp2JzVhKRQ2SeprQ8DUVps1w%40mail.gmail.com.

Stefan Zager

unread,
Mar 20, 2024, 11:46:25 PM3/20/24
to Xianzhu Wang, [email protected], Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori
+1, this is a good idea. For a while it seemed like the two use cases were isomorphic, but the vector<StackAllocatedType> example clearly shows where they diverge. STACK_ALLOCATED can focus on the GC use case, and STACK_SCOPED is useful in cases where the lifetime of a value should not extend beyond the currently running task.

Joe Mason

unread,
Mar 21, 2024, 6:40:56 PM3/21/24
to [email protected], Xianzhu Wang, [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori
Should we annotate base::FunctionRef with this?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAHOQ7J-nByVY3TYQXhHN_8aENrLb-9opvO_EyDAPouu-JoO4jg%40mail.gmail.com.

danakj

unread,
Mar 21, 2024, 9:51:09 PM3/21/24
to [email protected], [email protected], Xianzhu Wang, [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori
On Thu, Mar 21, 2024 at 11:40 AM 'Joe Mason' via Chromium-dev <[email protected]> wrote:
Should we annotate base::FunctionRef with this?

Shouldn't hurt.
 

Bartek Nowierski

unread,
Mar 24, 2024, 3:08:16 PM3/24/24
to Xianzhu Wang, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee, danakj, Bartek Nowierski
When we made an (unsuccessful) attempt to correlate raw_ptr/raw_ref related tools with STACK_ALLOCATED, we had two rules in mind:
1. Pointers *inside* STACK_ALLOCATED classes/structs shouldn't be rewritten as they're short-lived (which follows our "don't rewrite local variable and parameter pointers" rule).
2. Pointers *to* STACK_ALLOCATED classes/structs shouldn't be rewritten as the pointee isn't backed by PartitionAlloc, making raw_ptr/ref a dead balast (overhead with no protection).
This confirms that the current version of STACK_ALLOCATED has an overloaded meaning, which I'm in favor of untangling. But I think this wasn't the core of your proposal.

I am not sure about renaming STACK_ALLOCATED to STACK_SCOPED. It just feels weird to delete `new` and `deleted` for something labeled STACK_SCOPED.

Xianzhu Wang

unread,
Mar 24, 2024, 7:46:25 PM3/24/24
to Bartek Nowierski, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee, danakj
On Sun, Mar 24, 2024 at 5:06 AM Bartek Nowierski <[email protected]> wrote:
When we made an (unsuccessful) attempt to correlate raw_ptr/raw_ref related tools with STACK_ALLOCATED, we had two rules in mind:

Can you elaborate how the attempt is unsuccessful? It seems that the raw_ptr clang plugin can successfully skip STACK_ALLOCATED classes?
 
1. Pointers *inside* STACK_ALLOCATED classes/structs shouldn't be rewritten as they're short-lived (which follows our "don't rewrite local variable and parameter pointers" rule).
2. Pointers *to* STACK_ALLOCATED classes/structs shouldn't be rewritten as the pointee isn't backed by PartitionAlloc, making raw_ptr/ref a dead balast (overhead with no protection).
This confirms that the current version of STACK_ALLOCATED has an overloaded meaning, which I'm in favor of untangling. But I think this wasn't the core of your proposal.

I am not sure about renaming STACK_ALLOCATED to STACK_SCOPED. It just feels weird to delete `new` and `deleted` for something labeled STACK_SCOPED.

Fwiw, the renaming is to disambiguate from the blink version of STACK_ALLOCATED which will still check for stack memory allocation required by Oilpan.

Disallowing `new` may look over-restricted for STACK_SCOPED. For example, in theory it's valid to hold a stack-scoped object in a stack-scoped unique_ptr. However, allowing that would create hard-to-check situations for the clang plugin, e.g. the ownership of the unique_ptr is passed outside of the stack scope. Also in most cases, we should just use StackScopedType directly instead of unique_ptr<StackScopedType> in a stack scope. In unfortunate rare cases, we can just forfeit the benefit of STACK_SCOPED().

Bartek Nowierski

unread,
Mar 25, 2024, 4:40:23 AM3/25/24
to Xianzhu Wang, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee, danakj, Bartek Nowierski
Disallowing `new` may look over-restricted for STACK_SCOPED
That is precisely my worry that after renaming disallowing `new` will no longer fit the name and people will be tempted to remove it. How about renaming it to STACK_SCOPED_AND_DISALLOW_NEW (used by default) and having STACK_SCOPED for the exceptional cases.

Xianzhu Wang

unread,
Mar 25, 2024, 6:21:31 AM3/25/24
to Bartek Nowierski, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee, danakj
On Sun, Mar 24, 2024 at 6:39 PM Bartek Nowierski <[email protected]> wrote:
Disallowing `new` may look over-restricted for STACK_SCOPED
That is precisely my worry that after renaming disallowing `new` will no longer fit the name and people will be tempted to remove it. How about renaming it to STACK_SCOPED_AND_DISALLOW_NEW (used by default) and having STACK_SCOPED for the exceptional cases.

I think a comment will prevent people from removing the constraint. The theoretically perfect STACK_SCOPED() seems hard to enforce (thus error-prone) and not very useful. The shorter name might attract people to use it instead of the better STACK_SCOPED_AND_DISALLOW_NEW().

danakj

unread,
Mar 25, 2024, 5:01:10 PM3/25/24
to Xianzhu Wang, Bartek Nowierski, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee
On Sun, Mar 24, 2024 at 11:19 PM Xianzhu Wang <[email protected]> wrote:
On Sun, Mar 24, 2024 at 6:39 PM Bartek Nowierski <[email protected]> wrote:
Disallowing `new` may look over-restricted for STACK_SCOPED
That is precisely my worry that after renaming disallowing `new` will no longer fit the name and people will be tempted to remove it. How about renaming it to STACK_SCOPED_AND_DISALLOW_NEW (used by default) and having STACK_SCOPED for the exceptional cases.

I think a comment will prevent people from removing the constraint. The theoretically perfect STACK_SCOPED() seems hard to enforce (thus error-prone) and not very useful. The shorter name might attract people to use it instead of the better STACK_SCOPED_AND_DISALLOW_NEW().

A stack-restricted unique_ptr sounds more like optional. So I double checked, and deleting new does still allow the type to be used in optional, because optional makes use of the (new-in-c++20) std::construct_at, which does not use placement `new`. Use of (now kinda legacy) placement new will fail with such types. https://godbolt.org/z/63f8YhM3W

So I think this shows a bit of evidence that deleting new isn't too limiting for stack use as of c++20.

Bartek Nowierski

unread,
Apr 2, 2024, 6:16:19 PM4/2/24
to Xianzhu Wang, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee, danakj, Bartek Nowierski
Actually, std::vector<StackAllocated> will compile even if the vector is on heap or a global, so I don't know if renaming to STACK_SCOPED will accomplish anything.

Xianzhu Wang

unread,
Apr 2, 2024, 7:16:47 PM4/2/24
to Bartek Nowierski, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee, danakj
Allowing std::vector<StackAllocated> is a loophole and might be fixed in the future. The fix would be like the fix of the loophole that allowed WTF::Vector<StackAllocated> but is more difficult. Before the fix, we had some usage of WTF::Vector<StackAllocated> which had to be removed along with the fix.

With the rename:
- all STACK_ALLOCATED in non-blink code will become STACK_SCOPED, and std::vector<StackScoped> will be legal instead of exploiting a loophole.
- STACK_ALLOCATED will be for blink only to meet the requirement of Oilpan. std::vector<StackAllocated> will be a lesser problem because std::vector is not allowed in blink renderer code by default.

danakj

unread,
Apr 2, 2024, 7:59:23 PM4/2/24
to Xianzhu Wang, Bartek Nowierski, [email protected], [email protected], Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, [email protected], Kalvin Lee
On Tue, Apr 2, 2024 at 12:14 PM Xianzhu Wang <[email protected]> wrote:
Allowing std::vector<StackAllocated> is a loophole and might be fixed in the future.

IIRC the reason is that vector uses `std::construct_at(&p)` instead of `new (&p) T()`, and the former does not get blocked by StackAllocated. Is that right? If so, this is also the reason why StackAllocated works in optional (and other union types, like variant). Or would we somehow just ban specific template instantiations?

Xianzhu Wang

unread,
Apr 2, 2024, 8:44:25 PM4/2/24
to danakj, [email protected], Omer Katz, Bartek Nowierski, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee
(-memory-safe-dev@ because it's a closed group)

Actually std::optional<StackAllocated> is a good thing and Oilpan works well with it. I'm not sure about other union types.  +Omer Katz


Bartek Nowierski

unread,
Apr 3, 2024, 4:39:18 AM4/3/24
to Xianzhu Wang, danakj, [email protected], Omer Katz, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee

- all STACK_ALLOCATED in non-blink code will become STACK_SCOPED, and std::vector<StackScoped> will be legal instead of exploiting a loophole.
My point is that std::vector<StackScoped> can be a global or heap-allocated (due to the same loophole), so STACK_SCOPED will be equally inaccurate of a name as STACK_ALLOCATED.

Xianzhu Wang

unread,
Apr 3, 2024, 7:56:47 AM4/3/24
to Bartek Nowierski, danakj, [email protected], Omer Katz, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee
On Tue, Apr 2, 2024 at 6:37 PM Bartek Nowierski <[email protected]> wrote:

- all STACK_ALLOCATED in non-blink code will become STACK_SCOPED, and std::vector<StackScoped> will be legal instead of exploiting a loophole.
My point is that std::vector<StackScoped> can be a global or heap-allocated (due to the same loophole), so STACK_SCOPED will be equally inaccurate of a name as STACK_ALLOCATED.

Sorry I misunderstood your point. Filed crbug.com/332583996.

The two names having a common subset of false positives doesn't necessarily make them equally inaccurate. The new name makes the desired case of stack-scoped std::vector<StackScoped> no longer a false positive, and disambiguates from the blink version of STACK_ALLOCATED.

For further discussions, I created a globally editable empty doc here. I haven't got time to fill it with content yet. Feel free to add anything related into the doc.

Bartek Nowierski

unread,
Apr 4, 2024, 6:08:02 PM4/4/24
to Xianzhu Wang, v8-garbage-collection, danakj, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee, Omer Katz, Bartek Nowierski
The new name makes the desired case of stack-scoped std::vector<StackScoped> no longer a false positive
You have a point here. But it's a bit more complicated than that:

STACK_ALLOCATEDSTACK_SCOPED
T local;compiles, expectedcompiles, expected
auto local = std::make_unique<T>();doesn't compile, expecteddoesn't compile, not expected
field_ = std::make_unique<T>();doesn't compile, expecteddoesn't compile, expected
std::optional<T> local;compiles, expectedcompiles, expected
auto local = std::make_unique<std::optional<T>>();compiles, not expectedcompiles, expected
field_ = std::make_unique<std::optional<T>>();compiles, not expectedcompiles, not expected
std::vector<T> local;compiles, not expectedcompiles, expected
auto local = std::make_unique<std::vector<T>>();compiles, not expectedcompiles, expected
field_ = std::make_unique<std::vector<T>>();compiles, not expectedcompiles, not expected

I guess you can make an argument that STACK_SCOPE has more green, so it's a better choice. But not quite sure if it's worth disrupting the codebase. And one thing that still bugs me is that people will have to remember that STACK_SCOPED also implies DISALLOW_NEW which is an important information in some use cases, but not clear from the name at all.


The more I'm staring at the code, the more I would love to have two separate things: STACK_SCOPED_ON_STACK_ONLY and STACK_SCOPED_ON_HEAP_OK (terrible naming, I know). There are legitimate cases where we want something to be a pointer for polymorphism purposes but we want it to be stack-scoped (example 1, example 2). We can skip raw_ptr<> inside STACK_SCOPED_ON_HEAP_OK. For a different reason, we can skip raw_ptr<> pointing to STACK_SCOPED_ON_STACK_ONLY (I guess we can live with the std::vector false negative).

Now, how to enforce this is a whole different story. Maybe a clang plugin that does something like this:
1. An object of STACK_SCOPED_* has to be either a local variable, or a field (or base) of another class, that class has to be STACK_SCOPED* (it's ok for _ON_HEAP_OK to be embedded in _ON_STACK_ONLY but not vice-versa)
2. An exception to the above is when the embedding class is outside of our control, e.g. std::optional<> or 3p code, in which case the plugin would implicitly assume the embedder is "infected" with STACK_SCOPED_* and apply rule #1 to it.
3. STACK_SCOPED_ON_STACK_ONLY can't be a target of a pointer.
4. If STACK_SCOPED_ON_HEAP_OK class is a pointer target, that pointer has to follow rule #1.

That doesn't quite solve the std::vector case, but perhaps we can hardcode some known types.

Does this sound doable?


Cheers,
Bartek


On Wed, Apr 3, 2024 at 5:44 PM Omer Katz <[email protected]> wrote:
On Wed, Apr 3, 2024 at 6:54 AM Xianzhu Wang <[email protected]> wrote:

On Tue, Apr 2, 2024 at 6:37 PM Bartek Nowierski <[email protected]> wrote:

- all STACK_ALLOCATED in non-blink code will become STACK_SCOPED, and std::vector<StackScoped> will be legal instead of exploiting a loophole.
My point is that std::vector<StackScoped> can be a global or heap-allocated (due to the same loophole), so STACK_SCOPED will be equally inaccurate of a name as STACK_ALLOCATED.

Sorry I misunderstood your point. Filed crbug.com/332583996.

The two names having a common subset of false positives doesn't necessarily make them equally inaccurate. The new name makes the desired case of stack-scoped std::vector<StackScoped> no longer a false positive, and disambiguates from the blink version of STACK_ALLOCATED.

For further discussions, I created a globally editable empty doc here. I haven't got time to fill it with content yet. Feel free to add anything related into the doc.

On Wed, Apr 3, 2024 at 2:41 AM Xianzhu Wang <[email protected]> wrote:
(-memory-safe-dev@ because it's a closed group)

Actually std::optional<StackAllocated> is a good thing and Oilpan works well with it. I'm not sure about other union types.  +Omer Katz

For Oilpan, it's mostly the memory allocation that matters. Life scope is less of a concern.
As long as StackAllocated types are allocated on the native stack, Oilpan can find them using conservative stack scanning and handle them conservatively.
std::vector<StackAllocated>, even if the vector is stack allocated, is a problem for Oilpan because the vector's backing is allocated off the stack and Oilpan will have no way of observing it and finding the objects in the vector.
On-stack std::optional on the other hand is fine because the underlying object in this case will be allocated on the native stack. The same would generally apply to on-stack std::variant and unions (even though neither are gc-safe off-stack).


--

Omer Katz

V8, Software Engineer


Google Germany GmbH

Erika-Mann-Straße 33

80636 München


Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg


Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.



Bartek Nowierski

unread,
Apr 5, 2024, 3:37:17 AM4/5/24
to Omer Katz, Xianzhu Wang, v8-garbage-collection, danakj, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee
Yes, we should hardcode std::vector<T> et al. to ban STACK_SCOPED_ON_STACK_ONLY (or STACK_ALLOCATED, if we don't go through with the rename). std::optional<T> should be allowed though. It's really about the on-heap backing, which vector<> has but optional<> doesn't.

Independently, std::vector<T> and std::optional<T> should acquire T's annotation for the purpose of on-stack enforcement analysis.


On Fri, Apr 5, 2024 at 5:33 AM Omer Katz <[email protected]> wrote:


Why can't the plugin look at all fields of type std::vector<T>, std::optional<T>, etc. and check that T is not STACK_SCOPES_*?
It should be possible as an ASTMatcher query imo. We do something similar in the blink gc plugin to detect std::vector<T>, std::optional<T> and such where T is GCed (here).
Our checks for whether T is GCed or Traceable rely on it having a specific base class or a trace method. Assuming STACK_SCOPED_* contains some marker, the query for whether T is stack scopes should be as simple as this example.

danakj

unread,
Apr 5, 2024, 5:40:04 PM4/5/24
to Bartek Nowierski, Xianzhu Wang, v8-garbage-collection, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee, Omer Katz
I was surprised that `auto local = std::make_unique<T>();` is marked as expected to compile for STACK_SCOPED. While the unique_ptr is a local variable, it owns a heap allocation, and it can be moved in ways that cause it to outlive the current stack frame. So I would not expect us to want STACK_SCOPED in a unique_ptr, unless we have some dataflow analysis proving it does not escape the stack frame as well.

Stefan Zager

unread,
Apr 5, 2024, 6:46:17 PM4/5/24
to danakj, Bartek Nowierski, Xianzhu Wang, v8-garbage-collection, Joe Mason, Stefan Zager, Chromium-dev, Avi Drissman, Zhanbang He, Mikihito Matsuura, Keishi Hattori, Kalvin Lee, Omer Katz
On Fri, Apr 5, 2024 at 7:38 AM danakj <[email protected]> wrote:

I was surprised that `auto local = std::make_unique<T>();` is marked as expected to compile for STACK_SCOPED. While the unique_ptr is a local variable, it owns a heap allocation, and it can be moved in ways that cause it to outlive the current stack frame. So I would not expect us to want STACK_SCOPED in a unique_ptr, unless we have some dataflow analysis proving it does not escape the stack frame as well.

+1 -- it may be possible to determine via static analysis that this is safe, but it would have to be done case-by-case and it's much more complex than the current checks. STACK_SCOPED is ultimately a voluntary opt-in safeguard, which comes with some restrictions. This one doesn't seem onerous.

Bartek Nowierski

unread,
Apr 6, 2024, 2:08:44 AM4/6/24
to Stefan Zager, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Mikihito Matsuura, Omer Katz, Xianzhu Wang, Zhanbang He, danakj, v8-garbage-collection
The only places where the pointer can be moved to are also STACK_SCOOED. see rule #4 of my proposed algorithm.

Bartek Nowierski

unread,
Apr 8, 2024, 4:07:59 PM4/8/24
to Mikihito Matsuura, Stefan Zager, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Xianzhu Wang, Zhanbang He, danakj, v8-garbage-collection, Bartek Nowierski
Re: Idea 1: If it takes care of std::vector<StackAllocated>, we should do it. I assume it doesn't affect std::optional<StackAllocated>?
Re: Idea 2: I don't see an issue with calling it stack-scoped, if we can enforce that it can be only assigned a local or a field in another stack-scoped object.
Re: question: Hmmm... I was close to agreeing with you, but then I thought that this will be just inviting UaFs as the pointer will likely outlive the pointee (so it's a different reason).

I also updated spreadsheet based on your comments. Good catch that the plugin wouldn't allow templated fields SomeTemplate<StackAllocated> in non-STACK_ALLOCATED classes. That pretty much takes care of the algorithm I proposed for STACK_SCOPED.


On Mon, Apr 8, 2024 at 7:33 PM Mikihito Matsuura <[email protected]> wrote:
I have two ideas and one question.

Idea 1: We may just ban instantiation of `std::allocator<StackAllocated>` in the clang plugin to achieve a more strict `STACK_ALLOCATED` (i.e. no `std::vector<StackAllocated>`).
Idea 2: If we want to move forward with the `STACK_SCOPED` option, how about naming it like `STACK_OWNED`? I feel it's weird that we can return `std::unique_ptr<StackScoped>` from a function, as the object outlives the "scope".

Question: WDYT about pointers pointing to `STACK_*` object?

Currently, the plugin bans even `T*` and `T&` in member fields if `T` is `STACK_ALLOCATED`. However, in theory, it should be possible to use stack-allocated objects in the non-`STACK_ALLOCATED` class as a pointer/reference.
The same for `STACK_SCOPED`. Should we ban pointers to `STACK_SCOPED` in class member fields?
Honestly, I'm not particularly concerned about this point, but if we think that the name should match the reality, we need to consider this point as well.

Stefan Zager

unread,
Apr 8, 2024, 8:43:04 PM4/8/24
to Bartek Nowierski, Mikihito Matsuura, Stefan Zager, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Xianzhu Wang, Zhanbang He, danakj, v8-garbage-collection
It might be helpful to restate the premise and motivation for STACK_SCOPED: it is a voluntary opt-in feature intended to prevent well-intended developers from holding references to objects that are not safe beyond the scope of the current call stack or task. I think the priority should be on making sure it's not superficially leaky, even if that comes at the price of arbitrary restrictions such as forbidding `std::make_unique<StackScoped>()` altogether. Even if static code analysis can prove that construct to be safe on a case-by-case basis, it doesn't seem to me to be an onerous restriction. If it is truly too restrictive, we can consider whether it's worth the effort to implement the static analysis (which I suspect will be more complicated than Bartek suggests); but also the developer is free to not use `STACK_SCOPED` if it's too burdensome.

Bartek Nowierski

unread,
Apr 9, 2024, 4:01:04 AM4/9/24
to Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Xianzhu Wang, Zhanbang He, danakj, v8-garbage-collection, Bartek Nowierski
such as forbidding `std::make_unique<StackScoped>()` altogether
I actually have a specific use case where I want to apply STACK_SCOPED, but std::make_unique<StackScoped>() (assigned to a local variable) is preventing me. But from the looks of it, preventing it from escaping the scope of the current call stack/task should be easy.




Xianzhu Wang

unread,
Apr 9, 2024, 7:19:59 AM4/9/24
to Bartek Nowierski, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, danakj, v8-garbage-collection
On Mon, Apr 8, 2024 at 5:58 PM Bartek Nowierski <[email protected]> wrote:
such as forbidding `std::make_unique<StackScoped>()` altogether
I actually have a specific use case where I want to apply STACK_SCOPED, but std::make_unique<StackScoped>() (assigned to a local variable) is preventing me. But from the looks of it, preventing it from escaping the scope of the current call stack/task should be easy.

Can the local variable be std::optional<StackScoped>? 

Bartek Nowierski

unread,
Apr 9, 2024, 7:38:30 AM4/9/24
to Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, danakj, v8-garbage-collection
100%

Mikihito Matsuura

unread,
Apr 10, 2024, 3:06:33 AM4/10/24
to Bartek Nowierski, Stefan Zager, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Xianzhu Wang, Zhanbang He, danakj, v8-garbage-collection
I have two ideas and one question.

Idea 1: We may just ban instantiation of `std::allocator<StackAllocated>` in the clang plugin to achieve a more strict `STACK_ALLOCATED` (i.e. no `std::vector<StackAllocated>`).
Idea 2: If we want to move forward with the `STACK_SCOPED` option, how about naming it like `STACK_OWNED`? I feel it's weird that we can return `std::unique_ptr<StackScoped>` from a function, as the object outlives the "scope".

Question: WDYT about pointers pointing to `STACK_*` object?

Currently, the plugin bans even `T*` and `T&` in member fields if `T` is `STACK_ALLOCATED`. However, in theory, it should be possible to use stack-allocated objects in the non-`STACK_ALLOCATED` class as a pointer/reference.
The same for `STACK_SCOPED`. Should we ban pointers to `STACK_SCOPED` in class member fields?
Honestly, I'm not particularly concerned about this point, but if we think that the name should match the reality, we need to consider this point as well.


Xianzhu Wang

unread,
Apr 11, 2024, 7:41:37 PM4/11/24
to Bartek Nowierski, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, danakj, v8-garbage-collection
On Mon, Apr 8, 2024 at 9:36 PM Bartek Nowierski <[email protected]> wrote:
100%

My point (perhaps also others') is: We should discourage std::unique_ptr<StackScoped>, in favor of std::optional<StackScoped>, which makes deleting 'new' not a (big) problem for STACK_SCOPED(). Have you seen other cases needing std::unique_ptr<StackScoped>?

Bartek Nowierski

unread,
Apr 11, 2024, 7:51:04 PM4/11/24
to Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, danakj, v8-garbage-collection, Bartek Nowierski
I totally agree with you that this is the preferred option, where possible. The couple cases I pointed out before (example 1, example 2), however, use polymorphism so need to be a pointer. I didn't find a way to work around this :-(

danakj

unread,
Apr 11, 2024, 7:51:45 PM4/11/24
to Bartek Nowierski, Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection
On Thu, Apr 11, 2024 at 12:48 PM Bartek Nowierski <[email protected]> wrote:
I totally agree with you that this is the preferred option, where possible. The couple cases I pointed out before (example 1, example 2), however, use polymorphism so need to be a pointer. I didn't find a way to work around this :-(

You only need a heap pointer if you want to extend its lifetime (move it). Putting a polymorphic type on the stack and giving out pointers to that is fine.

Bartek Nowierski

unread,
Apr 11, 2024, 7:56:52 PM4/11/24
to danakj, Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection

danakj

unread,
Apr 11, 2024, 8:28:31 PM4/11/24
to Bartek Nowierski, Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection
On Thu, Apr 11, 2024 at 12:55 PM Bartek Nowierski <[email protected]> wrote:
Since it's stack scoped, it should be returned by value, in which case RVO happens and it's constructed in place on the stack. Or those functions should be turned into constructors.

The idea of returning an object by unique_ptr seems incompatible/at-odds-with being stack scoped.

Bartek Nowierski

unread,
Apr 11, 2024, 9:13:16 PM4/11/24
to danakj, Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection
It can't be returned by value, because of polymorphism. It's stack scoped nonetheless.

danakj

unread,
Apr 11, 2024, 9:16:21 PM4/11/24
to Bartek Nowierski, Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection
On Thu, Apr 11, 2024 at 2:11 PM Bartek Nowierski <[email protected]> wrote:
It can't be returned by value, because of polymorphism. It's stack scoped nonetheless.

Right, I mean it can if the caller is the one who decides and knows the final type instead of hiding that inside a function that type-erases. That is what would be required to put it on the stack, which seems fair to me.. you need that to know its size. Otherwise it has to go on the heap and then it's not stack-scoped anymore.

Stefan Zager

unread,
Apr 11, 2024, 9:24:09 PM4/11/24
to Bartek Nowierski, danakj, Xianzhu Wang, Stefan Zager, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection
Would this work:

class RuleIterator {
  STACK_SCOPED();
 public:
  RuleIterator(
      ProviderInterface& interface,
      ContentSettingsType content_type,
      bool off_the_record,
      const PartitionKey& partition_key)
    : impl_(interface.GetRuleIteratorImpl(content_type, off_the_record, partition_key) {}
  bool HasNext() const { return impl_->HasNext(); }
  std::unique_ptr<Rule> Next() { return std::move(impl_->Next()); }
 private:
  std::unique_ptr<RuleIteratorImpl> impl_;
};

class ProviderInterface {
 private:
  friend class RuleIterator;
  virtual std::unique_ptr<RuleIterator> GetRuleIteratorImpl(...);
}

?

Bartek Nowierski

unread,
Apr 12, 2024, 5:28:21 PM4/12/24
to Stefan Zager, danakj, Xianzhu Wang, Mikihito Matsuura, Avi Drissman, Chromium-dev, Joe Mason, Kalvin Lee, Keishi Hattori, Omer Katz, Zhanbang He, v8-garbage-collection, Bartek Nowierski
I am not sure if I understand your idea. If we go with the STACK_SCOPED that I'm proposing (i.e. with not deleted `new`), then we can simply add STACK_SCOPED() to both RuleIterator and RuleIteratorImpl without any need for refactoring. If we want `new` to be deleted, then this will fail on `std::unique_ptr<RuleIterator> GetRuleIteratorImpl(...)`


Reply all
Reply to author
Forward
0 new messages