1195957 - mozilla::Swap allocates heap type JS::Heap<T> on the stack in temporary

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

../../dist/include/mozilla/Move.h:231:3: error: variable of type 'mozilla::AlignedStorage2<js::HashMapEntry<JSObject *, JS::Heap<JSObject *> > >' only valid on the heap
  T tmp(Move(aX));
  ^
../../dist/include/mozilla/Move.h:231:3: note: value incorrectly allocated in an automatic variable
../../dist/include/mozilla/Alignment.h:126:56: note: 'mozilla::AlignedStorage2<js::HashMapEntry<JSObject *, JS::Heap<JSObject *> > >' is a heap type because it has a template argument heap type 'js::HashMapEntry<JSObject *, JS::Heap<JSObject *> >'
struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2
                                                       ^
../../dist/include/js/HashTable.h:650:11: note: 'js::HashMapEntry<JSObject *, JS::Heap<JSObject *> >' is a heap type because member 'value_' is a heap type 'JS::Heap<JSObject *>'
    Value value_;
          ^

Comment 1

[Nika Layzell [:nika] (ni? for response)]

I'm not sure what the best way to fix this would be. In this case the stack allocation is probably OK (because there is no way that the collector could run in this particular case), but it definitely isn't OK in the general case (as the destructor could, for example, trigger a collection). Maybe a specific overload for Swap on js::HashMapEntry?

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Implement a Swap for Heap<T> that does not put temporaries on the stack

I'm thinking we need a partial specialization of Move<Heap<T>> that swaps the contained pointer instead of the whole Heap<T>.

mystor: Heap<T> isn't about triggering collections, btw. It's about triggering post-write barriers to maintain certain invariants for generational GC. Specifically, all tenured -> nursery heap edges must be recorded in a side table that records the address of all such pointers. Swapping two Heap<T>s must update this table. (And yes, this is Too Much Information.)

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8649454 [details] [diff] [review]
Implement a Swap for Heap<T> that does not put temporaries on the stack

Review of attachment 8649454 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9498ca2f09a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9498ca2f09a