1459225 - Enable asm.js and wasm when running TSan

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

It seems to work nowadays, except for an OOM on wasm/atomic.js and a race that Luke says is an actual problem (\o/), so we should try to enable this.

Comment 1

[Luke Wagner [:luke]]

Attachment: init-before-register

The bug TSAN found is https://pastebin.com/BaG9QWPx.  What's happening here is that a tier-2 CodeSegment is registered in the process-wide processCodeSegmentMap_ before it and it's CodeTier are fully initialized (in particular, the back-pointers to their containing CodeTier and Code, resp).  Once a CodeSegment is in the processCodeSegmentMap_, any other process can racily access it via LookupCodeSegment() while the CodeSegment is finishing being initialized.

The fix is to delay the RegisterCodeSegment() call until after the CodeSegment/CodeTier are fully initialized.  The current logic for this "follow-up initialization" makes it hard to feel confident that we have a non-future-hazardous fix and so this patch gives CodeSegment/CodeTier/Code distinct "construct" and "initialize" steps and an assertion that the "initialize" step is finished for all 3 when a CodeSegment is registered.  This refactoring (while making the patch a bit bigger) helped point out the lazy stubs case

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8973336 [details] [diff] [review]
init-before-register

Oops, more cases to think about with lazy stub segments.

Comment 3

[Luke Wagner [:luke]]

Attachment: fix-init-tier-race

Better fix that tweaks lazy stub creation.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix-init-tier-race

Oops, didn't qref.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Add gc() to wasm/atomic.js test

I tried to split this test in 2 smaller tests but it didn't help - it's really this part of the test that's causing the OOM. This change makes the test go from perma-fail to always green.

Comment 6

[Luke Wagner [:luke]]

Attachment: fix-init-tier-race

The last patch was a WIP that I try'd but still needed to double-check my lazy-stub+tiering logic with bbouvier.  Here's an updated patch ready for review.

So with this patch, Code+CodeTier+CodeSegment have a clear two-pass creation scheme: pass 1 is construction which happens bottom up (CodeSegment first), pass 2 is initialize() and happens top down (Code first).  This is what we need since initialize() needs its owner and CodeSegment::initialize() does RegisterCodeSegment() which needs to happen last, after everything is fully initialized (for reasons given above).

Thus, most of the patch is just moving init stuff into constructors.  The deserialize() changes follow from wanting to construct bottom-up and I think simplify the code.

For the Module::finishTier2() changes, the constraints I'm trying to satisfy here are:
 * RegisterCodeSegment() now asserts that the given CodeSegment and its containing CodeTier and Code have been initialized.
 * When LazyStubSegment::create() is called normally, its containing CodeTier has already been initialized (b/c lazy compilation); so the right time to call CodeSegment::initialize() is immediately after construction, during create().
 * The corner case is in Module::finishTier2(), when the CodeTier is newly-created.  If LazyStubSegment::create() calls CodeSegment::initialize() the RegisterCodeSegment() assert mentioned above fires b/c the CodeTier isn't initialized.

So the change in Module::finishTier2() satisfies these constraints by switching the order so that the CodeTier is initialized first (by Code::setTier2(CodeTier&)), *then* the LazyStubSegments are create()d.  This gives the nice invariant in CodeTier::initialize() that lazyStubs_ is empty.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44c76dad59b6
Add a gc() call to wasm/atomic.js test so TSan builds don't OOM. r=lth

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/44c76dad59b6

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8973722 [details] [diff] [review]
fix-init-tier-race

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

Nice! I remember there's been a few changes in the architecture when the lazy stubs patch was almost ready to land, leading to complicated code. Happy to see it cleaned up (especially deserialization code). Thanks for the patch!

::: js/src/wasm/WasmCode.cpp
@@ +971,5 @@
> +{
> +    MOZ_ASSERT(!initialized());
> +    code_ = &code;
> +
> +    // See comments in CodeSegment::initialize() for why this must be last.

nit: maybe move this comment down one line?

@@ +972,5 @@
> +    MOZ_ASSERT(!initialized());
> +    code_ = &code;
> +
> +    // See comments in CodeSegment::initialize() for why this must be last.
> +    MOZ_ASSERT(lazyStubs_.lock()->empty());

also, there can't be a dead lock situation (in this assert) because this happens before we even:
- start tier 2 compilation
- finish module generation (thus before we give access to exports)
Is that right?

@@ +1112,5 @@
> +
> +    if (!tier2->initialize(*this, bytecode, linkData, *metadata_))
> +        return false;
> +
> +    tier2_ = Move(tier2);

For perfect symmetry with Code::initialize, maybe MOZ_ASSERT(hasTier2()) at the end?

::: js/src/wasm/WasmModule.cpp
@@ +258,5 @@
> +
> +    // Before we can make tier-2 live, we need to compile tier2 versions of any
> +    // extant tier1 lazy stubs (otherwise, tiering would have a delazifying
> +    // effect, breaking the assumption that any extant exported wasm function
> +    // has had a lazy entry stub compiled for it).

It's actually worse than being delazified, which sounds like a process that can be safely reverted: here, it'll even crash if we call into an entry that was lazily generated and for which we didn't make a tier2 entry.

Nice to see this written down.

@@ +284,5 @@
>                  return false;
>          }
>  
>          HasGcTypes gcTypesEnabled = code().metadata().temporaryHasGcTypes;
> +        const CodeTier& tier2 = code().codeTier(Tier::Ion);

nit: it's unfortunate that this name aliases the parameter's name. Maybe we could change the latter to something else? (or change this one)

Comment 10

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
Thanks, good points!

> > +    MOZ_ASSERT(lazyStubs_.lock()->empty());
> 
> also, there can't be a dead lock situation (in this assert) because this
> happens before we even:
> - start tier 2 compilation
> - finish module generation (thus before we give access to exports)
> Is that right?

I might be missing the particular case you're thinking about, but the lack of deadlock should follow from the (asserted) ascending order of lock-taking within the thread.  One of the two cases where CodeTier::initialize() is called is indeed (indirectly) from within Module::finishTier2() (via Code::setTier2()), so both bullets are false, but this is from a helper thread where no higher lock is held.

Comment 11

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #9)
> > +    tier2_ = Move(tier2);
> 
> For perfect symmetry with Code::initialize, maybe MOZ_ASSERT(hasTier2()) at
> the end?

Oops, can't assert this here since hasTier2() is only true after commitTier2(); but I added it there.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12581fbf4008
Baldr: fix race in tier-2 initialization (r=bbouvier)

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/12581fbf4008

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Enable asm.js/wasm with TSan

This is green now.

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Enable asm.js/wasm with TSan

Also fixes the comment.

Comment 16

[Luke Wagner [:luke]]

(In reply to Jan de Mooij [:jandem] from comment #14)
> This is green now.

\o/ Thanks for revisiting this Jan; this is going to help a lot going forward.

Comment 17

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

Comment on attachment 8974968 [details] [diff] [review]
Enable asm.js/wasm with TSan

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

\o/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3023c7efe312
Enable asm.js and wasm when running TSan in automation. r=sfink

Comment 19

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3023c7efe312