Re: [chromium-dev] Re: STACK_ALLOCATED() macro now available chromium-wide

283 views
Skip to first unread message

Bartek Nowierski

unread,
Mar 24, 2024, 3:06:52 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.




On Fri, Mar 22, 2024 at 3:49 AM danakj <[email protected]> wrote:
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.
 

On Wed, Mar 20, 2024 at 4:45 PM Stefan Zager <[email protected]> wrote:
+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.

On Wed, Mar 20, 2024 at 1:26 PM Xianzhu Wang <[email protected]> wrote:
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.







On Mon, Apr 24, 2023 at 8:18 PM 'Bartek Nowierski' via Chromium-dev <[email protected]> wrote:
Is it a bug with STACK_ALLOCATED or std::vector? I kinda wonder what (if anything) can be done about this?

Thanks,
Bartek


On Sat, Mar 18, 2023 at 1:35 AM Stefan Zager <[email protected]> wrote:
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.
 


On Tuesday, March 14, 2023 at 2:58:57 PM UTC+9 Stefan Zager wrote:
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.
 

On Tuesday, March 14, 2023 at 5:57:55 AM UTC+8 Stefan Zager wrote:


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.


On Mon, Mar 13, 2023 at 5:32 PM Stefan Zager <[email protected]> wrote:
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.

--
--
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.

--
--
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/CAHOQ7J-nByVY3TYQXhHN_8aENrLb-9opvO_EyDAPouu-JoO4jg%40mail.gmail.com.

--
--
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/CAH%3DT95T%3DD-0z2LNdkSzmqgx5FFcGL8RaRCrRuenUeHvmb1mZBQ%40mail.gmail.com.

Bartek Nowierski

unread,
Mar 25, 2024, 4:39:11 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.


On Mon, Mar 25, 2024 at 1:44 AM Xianzhu Wang <[email protected]> wrote:
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().

danakj

unread,
Mar 25, 2024, 4:59:16 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:14:40 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.

danakj

unread,
Apr 2, 2024, 7:57:39 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?
 
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.
Reply all
Reply to author
Forward
0 new messages