wasm: Implement anyref support for globals

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

(Blocks 1 bug)

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

Attachments

(4 attachments, 18 obsolete attachments)

5.62 KB, patch
lth
: review+
Details | Diff | Splinter Review
52.54 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
52.73 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
46.46 KB, patch
luke
: review+
jonco
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
Detaching from the initial anyref bug to not block it, since it's a bit more complicated than forecast (b/o barriers).
(Assignee)

Comment 1

a year ago
Posted patch globals.wip.patch (obsolete) — Splinter Review
(wip)
(Assignee)

Comment 2

a year ago
Posted patch wip-globals.patch (obsolete) — Splinter Review
(rebased)
Attachment #8963945 - Attachment is obsolete: true
Priority: -- → P1
Severity: normal → enhancement
Priority: P1 → P2
(Assignee)

Comment 3

a year ago
Posted patch wip.patch (obsolete) — Splinter Review
Updated wip, which introduces a missing-barrier crash in the final test in wasm/gc/anyref.js.
Attachment #8966660 - Attachment is obsolete: true
(Assignee)

Comment 4

11 months ago
Posted patch wip.patch (obsolete) — Splinter Review
Now with tracing support for globals which don't get a WebAssembly.Globals!

(Since I couldn't find to make test cases that were needing barriers, I made a very small domain-specific fuzzer that generates objects, in JS and in wasm, plays around with them and subfields, and triggers GCs. Now I've got a test case for prebarriers too)
Attachment #8971633 - Attachment is obsolete: true
(Assignee)

Comment 5

11 months ago
Posted patch wip.patch (obsolete) — Splinter Review
Attachment #8976234 - Attachment is obsolete: true
(Assignee)

Comment 6

11 months ago
Posted patch wip.patch (obsolete) — Splinter Review
wip patch, now with prebarrier and postbarrier for wasm globals, with test cases failing if we don't have those barriers.

Now, since this is an initial implementation, checks and calls to C++ (for full barriers) are done inline, just before/after the set_global. This might generate a lot of code, so I'm trying to think of alternatives:

- use OOL paths for the calls (will only affect performance, not code size)
- parametrize the code sequence by registers, and have one thunk per function (one for the prebarrier, one for the postbarrier)
- or one thunk per module, even

Or maybe we can defer the code size considerations until they prove being an actual issue and just ship something that works in the meanwhile.
Attachment #8976528 - Attachment is obsolete: true
(Assignee)

Comment 7

11 months ago
Posted patch anyref-globals.patch (obsolete) — Splinter Review
Patch is ready for review, I think. I've factored out the prebarrier/postbarrier code so it's also useful for future usage, including an untested case that should make your life easier (postbarrier when the holder object might be in the nursery).

I've slightly over-engineered the postbarrier in a way that will also work for anyref in tables while limiting the number of different C++ postbarrier functions we'd need. I think this design is sufficient, or might need to be slightly extended to support pointer-sized values when we have typed refs.

The only interesting feature missing is the stack information when we're in the pre-barrier code. (Stack info is there for the post-barrier code, because it's a simple symbolic address call)

Since the pre-barrier calls into a stub generated by the jit runtime, if we wanted to have stack info there, we'd need to make a proper wasm stub for this. This would be a bit inefficient, because the pre-barrier stub preserves all registers by itself; but having a wasm stub would mean we'd spill registers too before calling into the stub. So we'd do double register spilling work there. At the moment, the stack is just dropped.

Alternatives:
- just deal with it, it's a POC anyway.
- implement our own pre-barrier code, duplicating the one in the jit runtime, removing all the register spilling; then add a wasm stub for the pre-barrier.
- add a wasm stub for the pre-barrier, just spill the minimal amount of registers so the wasm stub can clobber them without fear (that would be ugly).
- [other ideas]
Attachment #8979690 - Attachment is obsolete: true
Attachment #8980040 - Flags: review?(lhansen)

Comment 8

11 months ago
Comment on attachment 8980040 [details] [diff] [review]
anyref-globals.patch

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

Nice work.  A number of nits here, and then a suggestion about how to make the barriers more efficient, but my main concern is actually that I don't believe the way you've extended Val is correct.  We should talk.  Either way I think another round is appropriate, so clearing review now.

::: js/src/jit-test/tests/wasm/gc/anyref-global-object.js
@@ +5,5 @@
> +function Baguette(calories) {
> +    this.calories = calories;
> +}
> +
> +var g = new WebAssembly.Global({ type: "anyref", value: null, mutable: true });

This should take the value as a second argument.  And, "type" => "value" soon.

@@ +7,5 @@
> +}
> +
> +var g = new WebAssembly.Global({ type: "anyref", value: null, mutable: true });
> +
> +gczeal(2, 1);

Comment about what this zeal mode does.

::: js/src/jit-test/tests/wasm/gc/anyref-global-postbarrier.js
@@ +42,5 @@
> +
> +test(exportsPlain);
> +test(exportsObj);
> +
> +// Test stacks reported in profiling mode in a separate way, to not perturbate

"perturbate" => "perturb"

::: js/src/jit-test/tests/wasm/gc/anyref-global-prebarrier.js
@@ +10,5 @@
> +)`).exports;
> +
> +let obj = { field: null };
> +
> +gczeal(4, 1);

A comment about what this zeal setting does would be helpful here.

::: js/src/jit-test/tests/wasm/gc/anyref.js
@@ +402,5 @@
> +// Globals.
> +
> +// WebAssembly.Global object.
> +
> +assertEq(new WebAssembly.Global({type: "anyref"}) instanceof WebAssembly.Global, true);

The "type" field is about to change its name to "value", that probably lands today.

@@ +419,5 @@
> +    assertErrorMessage(() => g.value = 42, TypeError, /immutable global/);
> +
> +    let obj = {};
> +    g = new WebAssembly.Global({type: "anyref"}, obj);
> +    assertEq(g.value, obj);

So, what's the behavior here if the initializing value is eg an integer or a primitive string?  I believe we currently coerce to Object, and then store, right?  We should test this.

@@ +469,5 @@
> +    (global $g_imp_mut_null   (import "constants" "mut_null") (mut anyref))
> +    (global $g_imp_mut_bread  (import "constants" "mut_bread") (mut anyref))
> +
> +    (global $g_imm_null     anyref (ref.null anyref))
> +    (global $g_imm_getglob  anyref (get_global $g_imp_imm_bread))

Nice :)

@@ +509,5 @@
> +assertEq(exports.get_mut(), null);
> +let glutenFreeBaguette = new Baguette("calories-free bread");
> +exports.set_mut(glutenFreeBaguette);
> +assertEq(exports.get_mut(), glutenFreeBaguette);
> +assertEq(exports.get_mut().calories, "calories-free bread");

There's one more test you may want to make, which is that importing an anyref global to a non-anyref global (and vice versa) should fail with a link error.  Right now they do not, due to a bug that Julian is about to fix.  But we should check.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +4363,5 @@
>      // Global variable access.
>  
>      Address addressOfGlobalVar(const GlobalDesc& global, RegI32 tmp)
>      {
> +        masm.loadWasmTlsRegFromFrame(tmp);

Please undo this change.

@@ +5693,5 @@
> +#ifdef ENABLE_WASM_GC
> +    // This emits a GC pre-write barrier. Note that this is needed when we
> +    // replace an member field with a new value, and the previous field value
> +    // might have no other references. Note the field might belong to an
> +    // object or be a stack slot or a register or a heap allocated value.

In previous paragraph, "an member" => "a member", "references" => "referents", and "Note that" and "Note" can be removed.

@@ +5698,5 @@
> +    //
> +    // let obj = { field: previousValue };
> +    // obj.field = newValue; // previousValue must be marked with a pre-barrier.
> +    //
> +    // valueAddr must contain the effective address of the previous value we're

"effective address" => "address"; "previous value" => "field"

@@ +5707,5 @@
> +        MOZ_ASSERT(valueAddr == PreBarrierReg);
> +
> +        Label skipBarrier;
> +
> +        // If the previous value is null, we don't need the barrier.

The previous-value check is only interesting if there's an incremental gc going, and that is the unlikely case, so we should do this test after first testing for incremental gc.  And in that case we won't need PreBarrierReg until after that test, so it should be computed later than it currently is (also see comments below).  That suggests that emitPreBarrier might be split into two phases, one that tests whether incremental gc is going and then another that checks for null and calls the barrier code, and the PreBarrierReg setup only need happen if the first test does not skip the barrier.

@@ +5741,5 @@
> +        masm.branchTestPtr(Assembler::Zero, setValue, setValue, &skipBarrier);
> +
> +        // If the set value is tenured, no barrier.
> +        RegPtr scratch = needRef();
> +        masm.branchPtrInNurseryChunk(Assembler::NotEqual, setValue, scratch, &skipBarrier);

It looks like branchPtrInNurseryChunk will be false for null pointers.  Since the set value being in the nursery is the common case - we hope that most null assignments are part of a special initialization sequence, not expressed by normal assignment - we should move the test for a null pointer after the test for a nursery pointer, to reduce the amount of testing.

@@ +8397,5 @@
>          break;
>        }
> +#ifdef ENABLE_WASM_GC
> +      case ValType::AnyRef: {
> +        RegPtr valueAddr{PreBarrierReg};

I know the braces are the new hotness, but since all other code in the file is using "... = PreBarrierReg" or similar, please do so here too.

@@ +8401,5 @@
> +        RegPtr valueAddr{PreBarrierReg};
> +        needRef(valueAddr);
> +        {
> +            ScratchI32 tmp(*this);
> +            masm.computeEffectiveAddress(addressOfGlobalVar(global, tmp), valueAddr);

I think this value is only needed *after* we pass the test in emitPreBarrier() for whether an incremental GC is already running, so it would be nice if we don't compute it until we need it.  (Also nice if we don't force the use of the PreBarrierReg until then, since that may cause us to spill.)

::: js/src/wasm/WasmJS.cpp
@@ +2138,5 @@
> +WasmGlobalObject::trace(JSTracer* trc, JSObject* obj)
> +{
> +    WasmGlobalObject* global = reinterpret_cast<WasmGlobalObject*>(obj);
> +    switch (global->type()) {
> +      case ValType::AnyRef: {

Braces on case blocks not needed here, I think.

::: js/src/wasm/WasmTypes.h
@@ +491,5 @@
>          I8x16 i8x16_;
>          I16x8 i16x8_;
>          I32x4 i32x4_;
>          F32x4 f32x4_;
> +        intptr_t ptr_;

Is this right?  This basically hides a pointer from the GC.  The GC may then move the object that the pointer points to at any time it gets to perform GC work, but this pointer will not be updated.  You can argue that if we guarantee no GC work while the Val is live then it won't be an issue but there's nothing that really prevents that.

@@ +2085,5 @@
> +        Last = Global
> +    };
> +
> +  private:
> +    uint32_t raw_;

Could we use bit fields or do we run up against portability issues with packing order?
Attachment #8980040 - Flags: review?(lhansen)
(Assignee)

Comment 9

11 months ago
Thanks for the review.

(In reply to Lars T Hansen [:lth] from comment #8)
> Comment on attachment 8980040 [details] [diff] [review]
> anyref-globals.patch
> 
> Review of attachment 8980040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +419,5 @@
> > +    assertErrorMessage(() => g.value = 42, TypeError, /immutable global/);
> > +
> > +    let obj = {};
> > +    g = new WebAssembly.Global({type: "anyref"}, obj);
> > +    assertEq(g.value, obj);
> 
> So, what's the behavior here if the initializing value is eg an integer or a
> primitive string?  I believe we currently coerce to Object, and then store,
> right?  We should test this.

That's right. I've added tests for all the primitive types, and one other important test that was missing was the explicit usage of undefined as the last argument, which throws, contrary to the value argument missing, which is silently coerced to nullptr. Not sure what's the best behavior should be there.

> @@ +509,5 @@
> > +assertEq(exports.get_mut(), null);
> > +let glutenFreeBaguette = new Baguette("calories-free bread");
> > +exports.set_mut(glutenFreeBaguette);
> > +assertEq(exports.get_mut(), glutenFreeBaguette);
> > +assertEq(exports.get_mut().calories, "calories-free bread");
> 
> There's one more test you may want to make, which is that importing an
> anyref global to a non-anyref global (and vice versa) should fail with a
> link error.  Right now they do not, due to a bug that Julian is about to
> fix.  But we should check.

Added the test. It passes as expected on my branch, so maybe I fixed this too in this patch, because of the changes I had to add.

> 
> ::: js/src/wasm/WasmBaselineCompile.cpp
> @@ +4363,5 @@
> >      // Global variable access.
> >  
> >      Address addressOfGlobalVar(const GlobalDesc& global, RegI32 tmp)
> >      {
> > +        masm.loadWasmTlsRegFromFrame(tmp);
> 
> Please undo this change.

Fair enough :) Was trying to put the definition of globalToTlsOffset closer to its uses, but that might have been overzealous (... no pun intended).

> @@ +5741,5 @@
> > +        masm.branchTestPtr(Assembler::Zero, setValue, setValue, &skipBarrier);
> > +
> > +        // If the set value is tenured, no barrier.
> > +        RegPtr scratch = needRef();
> > +        masm.branchPtrInNurseryChunk(Assembler::NotEqual, setValue, scratch, &skipBarrier);
> 
> It looks like branchPtrInNurseryChunk will be false for null pointers. 
> Since the set value being in the nursery is the common case - we hope that
> most null assignments are part of a special initialization sequence, not
> expressed by normal assignment - we should move the test for a null pointer
> after the test for a nursery pointer, to reduce the amount of testing.

It's subtle, but branchPtrInNurseryChunk will actually segfault if the pointer is null, see https://searchfox.org/mozilla-central/source/js/src/jit/x64/MacroAssembler-x64.cpp#472-473 for instance on x64, where the Address' base will be the mask if the original pointer is zero.

> @@ +8401,5 @@
> > +        RegPtr valueAddr{PreBarrierReg};
> > +        needRef(valueAddr);
> > +        {
> > +            ScratchI32 tmp(*this);
> > +            masm.computeEffectiveAddress(addressOfGlobalVar(global, tmp), valueAddr);
> 
> I think this value is only needed *after* we pass the test in
> emitPreBarrier() for whether an incremental GC is already running, so it
> would be nice if we don't compute it until we need it.  (Also nice if we
> don't force the use of the PreBarrierReg until then, since that may cause us
> to spill.)

(combined with other comment) Good idea! Will make the prebarriers a bit cheaper, hopefully. Thanks for the suggestion.

> ::: js/src/wasm/WasmTypes.h
> @@ +491,5 @@
> >          I8x16 i8x16_;
> >          I16x8 i16x8_;
> >          I32x4 i32x4_;
> >          F32x4 f32x4_;
> > +        intptr_t ptr_;
> 
> Is this right?  This basically hides a pointer from the GC.  The GC may then
> move the object that the pointer points to at any time it gets to perform GC
> work, but this pointer will not be updated.  You can argue that if we
> guarantee no GC work while the Val is live then it won't be an issue but
> there's nothing that really prevents that.

Great catch, indeed this is completely hidden from GC, and it was not intended. Will figure out a better solution (maybe using a GCPointer here, and root the Val at the few places where it can be a ref)

> @@ +2085,5 @@
> > +        Last = Global
> > +    };
> > +
> > +  private:
> > +    uint32_t raw_;
> 
> Could we use bit fields or do we run up against portability issues with
> packing order?

I tried this first but couldn't figure out the right packing order, neither how to make it explicit. Will try harder.
(Assignee)

Updated

11 months ago
Blocks: 1464157
(Assignee)

Comment 10

11 months ago
Posted patch 1.anyref-in-global.patch (obsolete) — Splinter Review
This addresses all the comments from your previous round, except for the Val rooting issue, which goes into a different patch.
Attachment #8980040 - Attachment is obsolete: true
Attachment #8980682 - Flags: review?(lhansen)
(Assignee)

Comment 11

11 months ago
Posted patch 2.any-wip.patch (obsolete) — Splinter Review
Unfinished patch, but I'm going on PTO for a long time, so setting feedback?. Please steal this patch and the previous time any time, if that can be helpful for general typed references.

The idea is that a new class is introduced, called Any, which really is a Val-or-a-ref-type-that-needs-gc, as discussed with Luke. It is going to get traced (and already implements the trace() method, which is trivial), so we get Rooted/Handle/MutableHandle Any, as well as GCVector to store them in several locations.

The patch doesn't compile yet, but I'm running out of time. There are places where the C++ compiler tries to use the copy ctor (HandleAny.set(Any(42)) for instance), and Any contains a GCPtrObject (which I think is correct, since it needs both barriers) and it's not copiable, so it doesn't get one. I implemented the move ctor there already, but not sure this is correct either. Anyway, more fun with C++ and JSAPI GC types involved here.

Also added a test case that crashes with gc zeal 14 (always compact), which I *think* tests the correctness of this patch to be implemented. (with the previous patch only, this test crashes when trying to touch an address that was stored as a raw pointer in an anyref Val)
Attachment #8980684 - Flags: feedback?(luke)
Comment on attachment 8980682 [details] [diff] [review]
1.anyref-in-global.patch

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

Only checked the barrier changes, but this looks fine.

::: js/src/wasm/WasmBaselineCompile.cpp
@@ +5692,5 @@
>  
> +#ifdef ENABLE_WASM_GC
> +    // The following couple of functions emit a GC pre-write barrier.
> +    // This is needed when we replace a
> +    // member field with a new value, and the previous field value might have

Nit: comment fill.

@@ +5714,5 @@
> +        ScratchPtr scratch(*this);
> +        masm.loadWasmTlsRegFromFrame(scratch);
> +        masm.loadPtr(Address(scratch, offsetof(TlsData, addressOfNeedsIncrementalBarrier)), scratch);
> +        masm.loadPtr(Address(scratch, 0), scratch);
> +        masm.branchTest32(Assembler::Zero, scratch, Imm32(0x1), skipBarrier);

Nit: you should be able to combine these last two instructions since there is a branchTest32 form that takes an Address and an immediate.

@@ +5754,5 @@
> +        if (object) {
> +            // If the object value isn't tenured, no barrier.
> +            masm.branchPtrInNurseryChunk(Assembler::Equal, *object, scratch, &skipBarrier);
> +        }
> +

Nit: if it were me I would probably switch the order of the previous two tests because I would expect that assigning into a nursery object is pretty common; hence we would filter more pointers quickly.  But I haven't asked around to see if we have data on that.
Attachment #8980682 - Flags: review?(lhansen) → review+
(Assignee)

Updated

10 months ago
Component: JavaScript Engine: JIT → Javascript: Web Assembly

Comment 13

10 months ago
Comment on attachment 8980684 [details] [diff] [review]
2.any-wip.patch

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

Sorry for the terrible delay in reviewing.  We've both discussed this issue in the meantime and iirc the current plan is:
 - changing Val to LitVal to represent just "literal values" which is mostly constants of which the only interesting one, for reference types, is null
 - for the few places that do need a full wasm value, having wasm::Val be a new type that can be used in Rooted<> and Handle<> templates
which is somewhat different than what's in this patch.
Attachment #8980684 - Flags: feedback?(luke)

Updated

10 months ago
Blocks: 1467632
(Assignee)

Comment 14

10 months ago
Posted patch 2.rename-val-to-litval.patch (obsolete) — Splinter Review
Splitting patch 2 into smaller parts.

This just does the Val -> LitVal renaming.
Attachment #8980684 - Attachment is obsolete: true
Attachment #8986745 - Flags: review?(lhansen)
(Assignee)

Comment 15

10 months ago
Posted patch 3.simplify.patch (obsolete) — Splinter Review
Two simplification opportunities.
Attachment #8986746 - Flags: review?(lhansen)
(Assignee)

Comment 16

10 months ago
Noticed we can go even further and remove one argument.
Attachment #8986746 - Attachment is obsolete: true
Attachment #8986746 - Flags: review?(lhansen)
Attachment #8986750 - Flags: review?(lhansen)
(Assignee)

Comment 17

10 months ago
Posted patch wip.patch (obsolete) — Splinter Review

Updated

10 months ago
Attachment #8986745 - Flags: review?(lhansen) → review+

Updated

10 months ago
Attachment #8986750 - Flags: review?(lhansen) → review+
(Assignee)

Comment 18

10 months ago
Posted patch 4.val.patch (obsolete) — Splinter Review
Attachment #8986840 - Attachment is obsolete: true
(Assignee)

Comment 19

10 months ago
Posted patch 4.val.patch (obsolete) — Splinter Review
Asking Jon for review for the rooting and GC APIs usage (in WasmTypes.h; looking at other files is appreciated but not strictly required), Luke for general review.

Some rationale about the used structures:
- Val may have stack lifetime in the worst case, so it needs to store JSObject* in a HeapPtr<JSObject*>, so that the store buffer entry (if it exists) is cleared when the Val is deleted when it gets out of scope.
- Cell (in WasmGlobalObject) has its owner's lifetime, which has GC lifetime, so I think it only needs GCPtr<JSObject*> to store the pointee.
Attachment #8987060 - Attachment is obsolete: true
Attachment #8987109 - Flags: review?(luke)
Attachment #8987109 - Flags: review?(jcoppeard)
Comment on attachment 8987109 [details] [diff] [review]
4.val.patch

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

::: js/src/wasm/WasmTypes.cpp
@@ +134,5 @@
>        case ValType::AnyRef:
> +        // No need for a pre-barrier here: the memory was uninitialized until
> +        // now.
> +        // No need for a post-barrier either, because the owning Instance or
> +        // Global object will always trace the memory that is written to.

We do need a post barrier here if the reference could point into the nursery. We don't trace through tenured things when we do a minor GC.

::: js/src/wasm/WasmTypes.h
@@ +710,5 @@
> +        I8x16              i8x16_;
> +        I16x8              i16x8_;
> +        I32x4              i32x4_;
> +        F32x4              f32x4_;
> +        HeapPtr<JSObject*> ptr_;

We don't need HeapPtr here if Val is going to be used inside Rooted<>.  Rooted things are always traced and don't need barriers.

The best thing to do is have the raw pointer in the Val and use it as Rooted<Val> or HeapPtr<Val> as appropriate.
Attachment #8987109 - Flags: review?(jcoppeard)
(Assignee)

Updated

10 months ago
Attachment #8987109 - Flags: review?(luke)

Comment 21

10 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a1643bd46e
Make To{WebAssembly,JS}Value private and inline GetGlobalExport; r=lth
(Assignee)

Updated

10 months ago
Keywords: leave-open
(Assignee)

Comment 22

10 months ago
Posted patch 4.val.patch (obsolete) — Splinter Review
It was actually much less work than expected. The rooting analysis did find two other real issues with the patch, and that makes me happy that we can have ~nice things.
Attachment #8987109 - Attachment is obsolete: true
Attachment #8987586 - Flags: feedback?(jcoppeard)
(Assignee)

Comment 24

10 months ago
Posted patch 4.val.patch (obsolete) — Splinter Review
Attachment #8987586 - Attachment is obsolete: true
Attachment #8987586 - Flags: feedback?(jcoppeard)
Attachment #8987632 - Flags: feedback?(jcoppeard)
Comment on attachment 8987632 [details] [diff] [review]
4.val.patch

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

Looks good, just a few questions and suggestions.

::: js/src/wasm/WasmJS.cpp
@@ +2155,5 @@
>      WasmGlobalObject* global = reinterpret_cast<WasmGlobalObject*>(obj);
> +
> +    // Manually call the non-trivial destructor for GC pointers.
> +    if (IsRefType(global->type()))
> +        global->cell()->ptr.~GCPtrObject();

This looks kinda strange to me.  Where do we initialise this GCPtr?  I'm not sure what the rules are for initialising different types in C++ unions though.

@@ +2161,5 @@
>      js_delete(global->cell());
>  }
>  
>  /* static */ WasmGlobalObject*
> +WasmGlobalObject::create(JSContext* cx, const Val& val, bool isMutable)

It's a hazard to hold val live over the call to NewObjectWithGivenProto which can GC.  You can pass Handle<Val> instead.

::: js/src/wasm/WasmTypes.cpp
@@ +101,5 @@
>          memcpy(dst, &u, jit::Simd128DataSize);
>          return;
>        case ValType::AnyRef:
> +        if (!u.ptr_)
> +            return;

Don't you need to write nullptr in this case?  Oh, maybe the memory is zero initialised.

@@ +103,5 @@
>        case ValType::AnyRef:
> +        if (!u.ptr_)
> +            return;
> +        // No need for a pre-barrier here: the memory was uninitialized until
> +        // now.

Consider asserting this.

@@ +108,5 @@
> +        memcpy(dst, &u.ptr_, sizeof(JSObject*));
> +        // The location needs to be post-barriered, because a nursery
> +        // allocation won't know about the instance's or global's memory, since
> +        // it's allocated on the non GC-heap or tenured.
> +        JS::HeapObjectPostBarrier((JSObject**)dst, nullptr, u.ptr_);

Does the location we write to ever get moved?
Attachment #8987632 - Flags: feedback?(jcoppeard) → feedback+
(Assignee)

Comment 26

10 months ago
(In reply to Jon Coppeard (:jonco) from comment #25)
> Comment on attachment 8987632 [details] [diff] [review]
> 4.val.patch
> 
> Review of attachment 8987632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, just a few questions and suggestions.
> 
> ::: js/src/wasm/WasmJS.cpp
> @@ +2155,5 @@
> >      WasmGlobalObject* global = reinterpret_cast<WasmGlobalObject*>(obj);
> > +
> > +    // Manually call the non-trivial destructor for GC pointers.
> > +    if (IsRefType(global->type()))
> > +        global->cell()->ptr.~GCPtrObject();
> 
> This looks kinda strange to me.  Where do we initialise this GCPtr?  I'm not
> sure what the rules are for initialising different types in C++ unions
> though.

This is initialized in WasmJS.cpp, in WasmGlobalObject::create. It's a good question, it's probably safer that I use a ctor there too, instead of using init() on the thing (it's probably uninitialized memory and even though the effect might be the same, one can't be sure).

> 
> @@ +2161,5 @@
> >      js_delete(global->cell());
> >  }
> >  
> >  /* static */ WasmGlobalObject*
> > +WasmGlobalObject::create(JSContext* cx, const Val& val, bool isMutable)
> 
> It's a hazard to hold val live over the call to NewObjectWithGivenProto
> which can GC.  You can pass Handle<Val> instead.

Good catch, will do. (I've been over-zealously trying to avoid unnecessary rooting)

> ::: js/src/wasm/WasmTypes.cpp
> @@ +101,5 @@
> >          memcpy(dst, &u, jit::Simd128DataSize);
> >          return;
> >        case ValType::AnyRef:
> > +        if (!u.ptr_)
> > +            return;
> 
> Don't you need to write nullptr in this case?  Oh, maybe the memory is zero
> initialised.

hmm, probably yes, just to be safe.

> 
> @@ +103,5 @@
> >        case ValType::AnyRef:
> > +        if (!u.ptr_)
> > +            return;
> > +        // No need for a pre-barrier here: the memory was uninitialized until
> > +        // now.
> 
> Consider asserting this.

Good idea.

> 
> @@ +108,5 @@
> > +        memcpy(dst, &u.ptr_, sizeof(JSObject*));
> > +        // The location needs to be post-barriered, because a nursery
> > +        // allocation won't know about the instance's or global's memory, since
> > +        // it's allocated on the non GC-heap or tenured.
> > +        JS::HeapObjectPostBarrier((JSObject**)dst, nullptr, u.ptr_);
> 
> Does the location we write to ever get moved?

No, it doesn't.

As discussed on IRC, there's nothing prevent cross-compartments pointers in here; will add code that makes sure we throw when trying to store a cross-compartment pointer, or read it out of the global.
(Assignee)

Comment 27

10 months ago
Posted patch 4.val.patch (obsolete) — Splinter Review
(not handled cross compartment pointers yet)

So to properly address the hazard you've spotted, I had to let go of the Maybe<Val> solution to the deferred initialization problem, and instead opted in for having a default, empty Val (with an invalid type) and implement Val::operator=(const Val&) instead.

The thing is... I think that now this function wants a post-write barrier after we've written the ptr in the current Val read from the other Val. But if I do a manual call to PostBarrierObject, then:
- the JSObject* location is put into the store buffer
- I think the Val gets out of scope, i.e. destructed
- the GC crashes somewhere.

See comment in the patch (grep TODO) where this happens. What do I need to do differently there?
Attachment #8987632 - Attachment is obsolete: true
Attachment #8987909 - Flags: feedback?(jcoppeard)

Comment 28

10 months ago
Comment on attachment 8987632 [details] [diff] [review]
4.val.patch

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

Driveby feedback:

::: js/src/wasm/WasmJS.h
@@ +158,3 @@
>  
>      wasm::ValType type() const;
> +    wasm::Val val() const;

Although technically the caller might do the Right Thing, this is a hazard, returning a Val by value.  For this reason, NativeObject::getSlot() (which does the analogous thing for JS objects) returns a const Value& and I think we should do that here too.
(Assignee)

Comment 29

10 months ago
Comment on attachment 8987909 [details] [diff] [review]
4.val.patch

We discussed it on irc, it should be good enough to not have a post barrier here, since the Val is only a stack value, or in a stack GCVector.
Attachment #8987909 - Flags: feedback?(jcoppeard)
Comment on attachment 8987909 [details] [diff] [review]
4.val.patch

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

::: js/src/wasm/WasmTypes.cpp
@@ +103,5 @@
> +        // it seems that the ptr address could end up in the store buffer, but the
> +        // Val would die before the ptr address is removed from the store buffer,
> +        // causing a crash later in IsCellPointerValid.
> +        if (val.ptr())
> +            JS::HeapObjectPostBarrier(&u.ptr_, nullptr, val.ptr());

You don't need a post barrier if Vals are always used Rooted on the stack.

If you did need a post barrier, the way to do this is something like TaggedPtr where you implement InternalBarrierMehods<Val> and then use HeapPtr<Val> or similar.

@@ +138,5 @@
> +        // The location needs to be post-barriered, because a nursery
> +        // allocation won't know about the instance's or global's memory, since
> +        // it's allocated on the non GC-heap or tenured.
> +        if (u.ptr_)
> +            JS::HeapObjectPostBarrier((JSObject**)dst, nullptr, u.ptr_);

Is this writing to a Cell?  If so, you want to cast the destination to a Cell& and assign cell.ptr, and that will do the barrier for you.

::: js/src/wasm/WasmTypes.h
@@ +738,5 @@
> +typedef Rooted<Val> RootedVal;
> +typedef Handle<Val> HandleVal;
> +typedef MutableHandle<Val> MutableHandleVal;
> +
> +typedef GCVector<Val, 0, SystemAllocPolicy> GCVectorRootedVal;

This should just be called GCVectorVal since it's not rooted.
(Assignee)

Comment 31

10 months ago
> Although technically the caller might do the Right Thing, this is a hazard, returning a Val by value.  For this reason, NativeObject::getSlot() (which does the analogous thing for JS objects) returns a const Value& and I think we should do that here too.

That's WasmGlobalObject::val(), for which the value is stored in a Cell, not in a Val. So we can't return const&, since we're creating the Val on the fly. I'll use a MutableHandleVal outparam instead.

> Is this writing to a Cell?  If so, you want to cast the destination to a Cell& and assign cell.ptr, and that will do the barrier for you.

It is used in two paths: one in which the written location is a Cell (for the case of the global belonging to a WasmGlobalObject) or on the non-gc heap (when the global belongs to the module and hasn't been imported/exported).
(Assignee)

Comment 32

10 months ago
Attachment #8980682 - Attachment is obsolete: true
Attachment #8988254 - Flags: review+
(Assignee)

Comment 33

10 months ago
Attachment #8986745 - Attachment is obsolete: true
Attachment #8988255 - Flags: review+
(Assignee)

Comment 34

10 months ago
Posted patch 4.val.patch (obsolete) — Splinter Review
I hope I have addressed all the feedback. No more crashes are happening, and many are doubting that we need any form of special treatment for COW at the moment (in any case, fuzzers may approve or disapprove).
Attachment #8987909 - Attachment is obsolete: true
Attachment #8988259 - Flags: review?(luke)
Attachment #8988259 - Flags: review?(jcoppeard)
(In reply to Luke Wagner [:luke] from comment #28)
> Although technically the caller might do the Right Thing, this is a hazard,
> returning a Val by value.

Returning raw GC thing pointers what we do normally when we allocate.  I don't think there's a problem with this.

(In reply to Benjamin Bouvier [:bbouvier] from comment #31)
> It is used in two paths: one in which the written location is a Cell (for
> the case of the global belonging to a WasmGlobalObject) or on the non-gc
> heap (when the global belongs to the module and hasn't been
> imported/exported).

So in one case it's a Cell and in the other it's not?  That seems like a bad idea.  What type is it in the other case?
(Assignee)

Comment 36

10 months ago
Posted patch 4.val.patchSplinter Review
What I hope to be the last version: replaces the GCPtrObject to a JSObject* (with manual barriers explicitly done) to avoid breaking encapsulation at the jit code level, as suggested and preferred by Jon.
Attachment #8988259 - Attachment is obsolete: true
Attachment #8988259 - Flags: review?(luke)
Attachment #8988259 - Flags: review?(jcoppeard)
Attachment #8988507 - Flags: review?(luke)
Attachment #8988507 - Flags: review?(jcoppeard)
Comment on attachment 8988507 [details] [diff] [review]
4.val.patch

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

This looks good to me.  Thanks for making all the updates.
Attachment #8988507 - Flags: review?(jcoppeard) → review+

Comment 38

10 months ago
Comment on attachment 8988507 [details] [diff] [review]
4.val.patch

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

Great work!  r+ with a few tweaks:

::: js/src/wasm/WasmJS.cpp
@@ +2148,1 @@
>          break;

pre-existing: but it seems like, with the coming reference types, it'd be beneficial if all these WasmGlobalObject `switch (type().code())`s didn't use `default` so that we were sure to include the new reference types.  Really, it's just those darn SIMD types that are a pain.  Maybe we could have a macro "CRASH_IF_SIMD_TYPES" that is just a list of all the SIMD cases with a MOZ_CRASH() and use that everywhere?

::: js/src/wasm/WasmModule.cpp
@@ +1009,5 @@
>  
>      return true;
>  }
>  
> +static Val

For general rooting hygiene, I think this method should return its value via MutableHandleVal, not by copy.  For the same reason, JS::Value is marked MOZ_NON_PARAM (although I have no idea how to make the analysis that checks this run locally...) so we should probably put that on wasm::Val.

::: js/src/wasm/WasmTypes.cpp
@@ +74,2 @@
>  void
> +Val::writePayload(uint8_t* dst) const

Given the write barrier which makes assumptions about what 'dst' points to, can we rename this method to writePayloadIntoInstanceGlobalData()?

@@ +96,5 @@
> +        MOZ_ASSERT(*(JSObject**)dst == nullptr, "should be null so no need for a pre-barrier");
> +        memcpy(dst, &u.ptr_, sizeof(JSObject*));
> +        // The location needs to be post-barriered, because a nursery
> +        // allocation won't know about the instance's or global's memory, since
> +        // it's allocated on the non GC-heap or tenured.

nit: I'm not sure that's exactly the reason we need to post-barrier here.  Rather, I'd say: "WasmInstanceObjects are always tenured and u.ptr_ may point to a nursery object, so we need a post-barrier since the global data of an instance is effectively a field of the WasmInstanceObject."

::: js/src/wasm/WasmTypes.h
@@ +603,5 @@
>      False,
>      True
>  };
>  
> +#define VAL_ACCESSORS                                                               \

As a non-macro way to avoid code duplication, how about:
 - adding a JSObject* to the LitVal union, storing nullptr into it for LitVal refs
 - making LitVal fields protected, not private
 - having Val derive LitVal, using the JSObject* to store non-null values

@@ +652,3 @@
>      } u;
>  
> +    friend class Val;

With above and sufficient `protected`, can we remove this friendship?

@@ +686,5 @@
>      bool isSimd() const { return IsSimdType(type()); }
>      static constexpr size_t sizeofLargestValue() { return sizeof(u); }
>  
> +    VAL_ACCESSORS;
> +    JSObject* ptr() const { MOZ_ASSERT(IsRefType(type_)); return nullptr; }

Could this method be removed?  It seems like any code that would call this would really want a Val.
Attachment #8988507 - Flags: review?(luke) → review+
(Assignee)

Comment 39

10 months ago
Thanks for the reviews!

> Given the write barrier which makes assumptions about what 'dst' points to, can we rename this method to writePayloadIntoInstanceGlobalData()?

This would be a misnomer, this the Variable GetGlobal case for indirect globals uses this method. The location which is written to has a uniform representation, per Jon's ask: so the Cell contains a JSObject* and the Val too, both of which are manually barriered.

> nit: I'm not sure that's exactly the reason we need to post-barrier here.  Rather, I'd say: "WasmInstanceObjects are always tenured and u.ptr_ may point to a nursery object, so we need a post-barrier since the global data of an instance is effectively a field of the WasmInstanceObject."

Right, I can reformulate my comment, but per the above answer, we also need to consider the case where the location written to is a Cell, in which case it's allocated on the non-gc heap, which is non-visible during a nursery collection, thus needs the post barrier.kjjk

>  - having Val derive LitVal, using the JSObject* to store non-null values

So I've done this at some point, and reverted it, because I was afraid the hazard analysis would see the JSObject* member in the union, and say LitVal may be unrooted, although LitVal may only have nullptr there (hazard analysis can't know this!). But maybe the hazard analysis only cares about classes that have been Rooted<> at least once; if so, that would be fine. Let's see what a try build thinks...
(Assignee)

Updated

10 months ago
Keywords: leave-open

Comment 40

10 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1478fb0f82de
Implement support of anyref in wasm globals; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddbbcea98cde
Rename Val to LitVal; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b05573ae795a
Implement Val, a rooted LitVal; r=luke, r=jonco
(Assignee)

Comment 41

10 months ago
As expected, putting the JSObject* within the union in LitVal introduced a benign hazard, in a case looking like this:

LitVal something;
if (!FuncThatActuallyInitializes(&something))
  return false;

so the fix was to use a Maybe<LitVal> there and use emplace within the function.
(Assignee)

Comment 43

10 months ago
The CGC bug was real: a Val shouldn't be traced if its type is invalid, which can happen in a few places, just before it gets really initialized. Fixed this with an additional check that the Val was initialized.

The MSAN situation is pretty terrible (there's a number of "maximum known test failures"), and this patch introduces one, which isn't related to wasm but to (JS) baseline. Sfink has opened bug 1472202 and I've made a patch there to disable MSAN from being tier 1 until it's fixed. In the meanwhile, I've bumped the count of known test failures.

Comment 44

10 months ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffe4a57033c
Implement support of anyref in wasm globals; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/631a97e399a0
Rename Val to LitVal; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/50d6babb2ec7
Implement Val, a rooted LitVal; r=luke, r=jonco

Comment 45

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ffe4a57033c
https://hg.mozilla.org/mozilla-central/rev/631a97e399a0
https://hg.mozilla.org/mozilla-central/rev/50d6babb2ec7
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

9 months ago
Depends on: 1473956
Depends on: 1475943
You need to log in before you can comment on or make changes to this bug.