1625212 - Investigate giving GC cells an explicit header field to store the GC flags

Status: RESOLVED FIXED

(Core :: JavaScript: GC, task, P3)

Description

[Jon Coppeard (:jonco)]

The GC uses the bottom bits of the first word of a GC thing to store some flags, e.g. to tell if the thing has been forwarded. This is surprising to GC thing implementers who must take care not to clobber the flags when storing to this word. It is also possibly undefined behaviour, although I don't understand all the nuances around that.

To resolve this we could add a uintptr_t field to Cell (the ultimate base class of all GC things) and add accessors that allow derived classes to use the spare bits to store their data, with the appropriate assertions.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Add CellWithNonGCPointer base class for GC things whose first field is a non-GC pointer

The flags_ word is factored out of CellWithLengthAndFlags and renamed flagsAndData_ for clarity. ObjectGroup is udpated to use the new class.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Add CellWithTenuredGCPointer base class for GC things whose first word is a GC pointer

Shape is updated to use the new class.

Depends on D68420

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Update all other GC things to use the new base classes

Depends on D68421

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Move flagsAndData_ field to Cell base class

Depends on D68422

Comment 5

[Jon Coppeard (:jonco)]

Asking for feedback for whether you think this is sane and should land. I don't particularly like moving the first field out of the derived classes, but it's arguably still better than the status quo.

Comment 6

[Ted Campbell [:tcampbell]]

Another design angle here is to leave the field in the current place, but put wrapper types around the pointers to at least document it all. So for BaseScript, we would replace uint8_t* jitCodeRaw_ with CellHeaderNonGcPtr<uint8_t> jitCodeRaw_. I have no idea what to call that though..

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Ted Campbell [:tcampbell] from comment #6)
That works and the reason I didn't do that is because it doesn't enforce that first word of the cell actually uses one of these wrapper types. Perhaps there is a solution to that though.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Jon Coppeard (:jonco) from comment #7)

because it doesn't enforce that first word of the cell actually uses one of these wrapper types. Perhaps there is a solution to that though.

Maybe something like this?

template <typename T>
void Check(T* thing) {
  auto t = thing->firstWordRef(); // Every Cell type needs to implement this.
  static_assert decltype(t) is approved;
  MOZ_ASSERT(&t == thing, "must be first member");
}

Comment 9

[Jon Coppeard (:jonco)]

(In reply to Jan de Mooij [:jandem] from comment #8)
Yeah, exactly what I had in mind :)

Comment 10

[Ted Campbell [:tcampbell]]

I guess if we go this route we can consider making CellWithLengthAndFlags just a member instead of a base.

Comment 11

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

Sorry for not getting to this. At this point, I guess I'll just say that I do prefer not pulling out the field, since it seems like it's avoidable without losing the checking.

As for UB, specifically strict aliasing considerations, I don't think these changes are going to really affect anything. I've been reading up on aliasing rules, and I think we do have a problem already, but it's going to need to be fixed at the read/write sites; uintptr_t isn't going to help.

For example, I think https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/js/src/gc/Marking-inl.h#125 is writing a uintptr_t into a memory location that c++ says has DynamicType JitCode* or ObjectGroup* or whatever, and thus smells like a strict aliasing violation to me. IIUC, that should be doing a placement new. Same with https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/js/src/gc/Marking.cpp#3161,3163 . I think that if those things were done, https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/js/src/gc/RelocationOverlay.h#51 would become correct. (Or rather, that line is correct no matter what, since it's just returning a pointer. The UB would be when something dereferences it.)

Then there's https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/js/src/gc/Cell.h#171-172 which does the big bad thing of dereferencing a pointer with the wrong type. I think this would be the correct code:

  inline bool isForwarded() const {
    uintptr_t firstWord;
    memcpy(&firstWord, reinterpret_cast<void*>(this), sizeof(uintptr_t));
    return firstWord & FORWARD_BIT;
  }

(and supposedly that's supposed to compile down to the same thing; I haven't checked.) firstWord points to memory that is and always will be a uintptr_t. this points to memory that might have a bunch of different types, but memcpy is allowed to read from anything.

If it helps, and people don't tell me I'm barking mad, I could put together a patch for this stuff on top of whatever you come up with here, since it'll probably change some of this(?). (Or you can do it if you prefer.)

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #11)
The goal here is to replace underlying pointer field (e.g. the JitCode* or ObjectGroup*) with a uintptr_t in one way or another to avoid reading/writing the field with the wrong type. AIUI this solves the problem for the forwarding flag and address, but not the ReloationOverlay::next_ field. The possibility of using placement new for this was discussed.

Comment 13

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Introduce CellHeader base class and convert String and BigInt to use an explicit cell header field r?tcampbell

Comment 14

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Add CellHeaderWithNonGCPointer class and use it for ObjectGroup r?sfink

Depends on D68827

Comment 15

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Add CellHeaderWithTenuredGCPointer and use it for Shape r?sfink

Depends on D68830

Comment 16

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give JitCode a CellHeader r?jandem

Depends on D68831

Comment 17

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give JSScript a cell header r?tcampbell

Depends on D68832

Comment 18

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give Scope a cell header r?jandem

I had a compilation issue with making the cell header contain a scope pointer due to the Scope class not being fully defined at that point so I worked around it by reodering the fields.

Depends on D68833

Comment 19

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give JSObject a cell header r?jandem

Depends on D68834

Comment 20

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give RegExpShared a cell header r?sfink

I had to reorder the fields here to make so the first field was simple to replace.

Depends on D68836

Comment 21

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give Symbol a cell header r?tcampbell

Depends on D68837

Comment 22

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Give BaseShape a cell header r?tcampbell

Depends on D68838

Comment 23

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Check that all cells have a cell header and that it is at the start of the cell r?sfink

Depends on D68839

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4c240f9d3e1
Introduce CellHeader base class and convert String and BigInt to use an explicit cell header field r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/e1350f5a444c
Add CellHeaderWithNonGCPointer class and use it for ObjectGroup r=sfink
https://hg.mozilla.org/integration/autoland/rev/b8850cec94f9
Add CellHeaderWithTenuredGCPointer and use it for Shape r=sfink
https://hg.mozilla.org/integration/autoland/rev/5c1db656c7a6
Give JitCode a CellHeader r=jandem,tcampbell
https://hg.mozilla.org/integration/autoland/rev/d02c04d34ed9
Give JSScript a cell header r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/e38411544a17
Give Scope a cell header r=jandem
https://hg.mozilla.org/integration/autoland/rev/629b473b36f2
Give JSObject a cell header r=jandem
https://hg.mozilla.org/integration/autoland/rev/51a9e5d36bfb
Give RegExpShared a cell header r=sfink
https://hg.mozilla.org/integration/autoland/rev/bf0a5676af9f
Give Symbol a cell header r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/a255c4445189
Give BaseShape a cell header r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/9ac72a058642
Check that all cells have a cell header and that it is at the start of the cell r=sfink

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a07f96b0b41
Backed out 11 changesets for mochitest failures at Cell.h on a CLOSED TREE.

Comment 26

[Oana Pop-Rus]

Backed out 11 changesets (bug 1625212) for mochitest failures at Cell.h

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&revision=9ac72a05864283683d1aebd688f7e48a6b54142d&selectedJob=295762094

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=247746196&repo=autoland&lineNumber=2480

Backout: https://hg.mozilla.org/integration/autoland/rev/0a07f96b0b411e4867fa58a91738d542e1c3fce7

Comment 27

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Update TSAN supression list now a Shape's base shape is not marked directly r?sfink

Previously this ended up with MovingTracer doing the update itself, but now the pointer is stored in a CellHeader it's done through an overload of TraceEdge.

The TSAN stack doesn't appear to have the class for the unsafeSetPtr method that actually does the update so rather than make this apply to all uses of this method I added a supression for the next function up in the stack.

Comment 28

[Jon Coppeard (:jonco)]

Attachment: Bug 1625212 - Update gdb pretty printers in line with CellHeader changes r?sfink

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd3c63762d2c
Introduce CellHeader base class and convert String and BigInt to use an explicit cell header field r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/90a41b1fa6e0
Add CellHeaderWithNonGCPointer class and use it for ObjectGroup r=sfink
https://hg.mozilla.org/integration/autoland/rev/8fecdcf14ec5
Add CellHeaderWithTenuredGCPointer and use it for Shape r=sfink
https://hg.mozilla.org/integration/autoland/rev/219873fae1dc
Give JitCode a CellHeader r=jandem,tcampbell
https://hg.mozilla.org/integration/autoland/rev/c846189c425c
Give JSScript a cell header r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/d12afb36d200
Give Scope a cell header r=jandem
https://hg.mozilla.org/integration/autoland/rev/a0531e0c35f4
Give JSObject a cell header r=jandem
https://hg.mozilla.org/integration/autoland/rev/42aacd19ea36
Give RegExpShared a cell header r=sfink
https://hg.mozilla.org/integration/autoland/rev/366364315e61
Give Symbol a cell header r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/a1cc6e73479e
Give BaseShape a cell header r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/6de2c3884377
Check that all cells have a cell header and that it is at the start of the cell r=sfink
https://hg.mozilla.org/integration/autoland/rev/ea5efdc9ae7a
Update gdb pretty printers in line with CellHeader changes r=sfink
https://hg.mozilla.org/integration/autoland/rev/cfa64a6b5a87
Update TSAN supression list now a Shape's base shape is not marked directly r=sfink

Comment 30

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/dd3c63762d2c
https://hg.mozilla.org/mozilla-central/rev/90a41b1fa6e0
https://hg.mozilla.org/mozilla-central/rev/8fecdcf14ec5
https://hg.mozilla.org/mozilla-central/rev/219873fae1dc
https://hg.mozilla.org/mozilla-central/rev/c846189c425c
https://hg.mozilla.org/mozilla-central/rev/d12afb36d200
https://hg.mozilla.org/mozilla-central/rev/a0531e0c35f4
https://hg.mozilla.org/mozilla-central/rev/42aacd19ea36
https://hg.mozilla.org/mozilla-central/rev/366364315e61
https://hg.mozilla.org/mozilla-central/rev/a1cc6e73479e
https://hg.mozilla.org/mozilla-central/rev/6de2c3884377
https://hg.mozilla.org/mozilla-central/rev/ea5efdc9ae7a
https://hg.mozilla.org/mozilla-central/rev/cfa64a6b5a87