Add checks for stack-allocated types and their uses in fields.

Add checks for stack-allocated types and their uses in fields.

- If A derives B, then A is stack allocated iff B is stack allocated.
- Fields to stack allocated types can only appear in stack allocated classes.

BUG=334149
R=ager@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258346

[zerny-chromium, 2014-03-20 09:57:36]

Blink side change: https://codereview.chromium.org/203963005/

These requirements entail a lot of Blink side changes because all classes in a
hierarchy of stack-allocated structures must each be annotated. This makes it
very clear to developers what classes are stack allocated.

An alternative would be to only require STACK_ALLOCATED at the base class.

What do you think?

[haraken, 2014-03-20 10:22:47]

On 2014/03/20 09:57:36, zerny-chromium wrote:
> Blink side change: https://codereview.chromium.org/203963005/
> 
> These requirements entail a lot of Blink side changes because all classes in a
> hierarchy of stack-allocated structures must each be annotated. This makes it
> very clear to developers what classes are stack allocated.
> 
> An alternative would be to only require STACK_ALLOCATED at the base class.
> 
> What do you think?

I guess there is no benefit for developers in making it clear what classes are
stack allocated. So I don't want to mess up the code base with such meaningless
macros. Wouldn't it be possible to force developers to add STACK_ALLOCATED to
only stack-allocated classes/structs that contain oilpan Members?

[Mads Ager (chromium), 2014-03-20 10:32:29]

The code looks good. After talking to Ian off-line we are now leaning towards
not requiring STACK_ALLOCATED in all subclasses. If we have Members in
stack-allocated objects we will get trace checking errors if we make something
that used to be stack allocated a part object or a dynamically allocated object.

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
tools/clang/blink_gc_plugin/RecordInfo.cpp:20: is_stack_allocated_(kMaybe),
Wasn't it called kNone in the enum? This is really a cache and maybe we should
name it accordingly?

enum CachedBool {
  kFalse = 0,
  kTrue = 1,
  kNotComputed = 2
};

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
File tools/clang/blink_gc_plugin/tests/heap/stubs.h (right):

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
tools/clang/blink_gc_plugin/tests/heap/stubs.h:75: void* operator new(size_t) =
delete;
Just to play it safe, maybe you should add it here as well so this mirrors the
code that we will have on trunk?

[zerny-chromium, 2014-03-20 10:34:24]

On 2014/03/20 10:22:47, haraken wrote:
> On 2014/03/20 09:57:36, zerny-chromium wrote:
> > Blink side change: https://codereview.chromium.org/203963005/
> > 
> > These requirements entail a lot of Blink side changes because all classes in
a
> > hierarchy of stack-allocated structures must each be annotated. This makes
it
> > very clear to developers what classes are stack allocated.
> > 
> > An alternative would be to only require STACK_ALLOCATED at the base class.
> > 
> > What do you think?
> 
> I guess there is no benefit for developers in making it clear what classes are
> stack allocated. So I don't want to mess up the code base with such
meaningless
> macros. Wouldn't it be possible to force developers to add STACK_ALLOCATED to
> only stack-allocated classes/structs that contain oilpan Members?

An even less intrusive requirement is that only the top-most class of a
stack-allocated hierarchy needs to be annotated as STACK_ALLOCATED. And only if
one of the classes in the hierarchy actually contains a member to a GC managed
object.

I think this is a better way to go, so I'll update the two CL to do that
instead.

[haraken, 2014-03-20 10:36:29]

> An even less intrusive requirement is that only the top-most class of a
> stack-allocated hierarchy needs to be annotated as STACK_ALLOCATED. And only
if
> one of the classes in the hierarchy actually contains a member to a GC managed
> object.
> 
> I think this is a better way to go, so I'll update the two CL to do that
> instead.

This sounds reasonable to me :)

[zerny-chromium, 2014-03-20 15:14:20]

This patch lets derived classes inherit the STACK_ALLOCATED property. PTAL

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
File tools/clang/blink_gc_plugin/RecordInfo.cpp (right):

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
tools/clang/blink_gc_plugin/RecordInfo.cpp:20: is_stack_allocated_(kMaybe),
On 2014/03/20 10:32:29, Mads Ager (chromium) wrote:
> Wasn't it called kNone in the enum? This is really a cache and maybe we should
> name it accordingly?
> 
> enum CachedBool {
>   kFalse = 0,
>   kTrue = 1,
>   kNotComputed = 2
> };

Done.

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
File tools/clang/blink_gc_plugin/tests/heap/stubs.h (right):

https://codereview.chromium.org/206123004/diff/1/tools/clang/blink_gc_plugin/...
tools/clang/blink_gc_plugin/tests/heap/stubs.h:75: void* operator new(size_t) =
delete;
On 2014/03/20 10:32:29, Mads Ager (chromium) wrote:
> Just to play it safe, maybe you should add it here as well so this mirrors the
> code that we will have on trunk?

Done.

[Mads Ager (chromium), 2014-03-20 15:25:59]

LGTM

[zerny-chromium, 2014-03-20 15:37:57]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-03-20 15:39:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/206123004/40001

[commit-bot: I haz the power, 2014-03-20 18:07:51]

Change committed as 258346