Closed Bug 1459225 Opened Last year Closed Last year

Enable asm.js and wasm when running TSan

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(3 files, 4 obsolete files)

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.
Attached patch init-before-register (obsolete) — Splinter Review
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
Assignee: nobody → luke
Attachment #8973336 - Flags: review?(lhansen)
Comment on attachment 8973336 [details] [diff] [review]
init-before-register

Oops, more cases to think about with lazy stub segments.
Attachment #8973336 - Attachment is obsolete: true
Attachment #8973336 - Flags: review?(lhansen)
Attached patch fix-init-tier-race (obsolete) — Splinter Review
Better fix that tweaks lazy stub creation.
Attached patch fix-init-tier-race (obsolete) — Splinter Review
Oops, didn't qref.
Attachment #8973362 - Attachment is obsolete: true
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.
Attachment #8973427 - Flags: review?(lhansen)
Attachment #8973363 - Flags: review?(lhansen)
Attachment #8973427 - Flags: review?(lhansen) → review+
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.
Attachment #8973363 - Attachment is obsolete: true
Attachment #8973363 - Flags: review?(lhansen)
Attachment #8973722 - Flags: review?(bbouvier)
Pushed by jandemooij@gmail.com:
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
Keywords: leave-open
Priority: -- → P2
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)
Attachment #8973722 - Flags: review?(bbouvier) → review+
(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.
(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.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12581fbf4008
Baldr: fix race in tier-2 initialization (r=bbouvier)
Attached patch Enable asm.js/wasm with TSan (obsolete) — Splinter Review
This is green now.
Assignee: luke → jdemooij
Status: NEW → ASSIGNED
Attachment #8974967 - Flags: review?(sphink)
Also fixes the comment.
Attachment #8974967 - Attachment is obsolete: true
Attachment #8974967 - Flags: review?(sphink)
Attachment #8974968 - Flags: review?(sphink)
(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 on attachment 8974968 [details] [diff] [review]
Enable asm.js/wasm with TSan

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

\o/
Attachment #8974968 - Flags: review?(sphink) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3023c7efe312
Enable asm.js and wasm when running TSan in automation. r=sfink
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/3023c7efe312
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.