Odin: put asm.js Atomics/SAB supporg behind wasmTestMode

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments)

Posted patch sab-test-modeSplinter Review
After some initial discussion last week, I think we should keep the asm.js Atomics/SharedArrayBuffer support behind a flag instead of having it ship at the same time as SAB ships.

The main reason is that asm.js+SAB won't be an actual expected shipping configuration for most (if not all) current asm.js users:
 - the entire asm.js module's code is duplicated in each worker
 - asm.js+SAB isn't supported by any of the other asm.js-optimizing engines and might not ever be

Keeping it behind a flag will help us keep testing the current support (so the code doesn't rot) but still allow us to break things as wasm+SAB integration is implemented.  A flag will also avoid adding a new source of s-s bugs, especially since this code is churning.

As you see in the patch, shell tests can toggle via `setJitCompilerOption('wasm.test-mode', 1)`.  The setting can also be set globally for the shell or browser by setting the env var JIT_OPTION_wasmTestMode=true.
Attachment #8786596 - Flags: review?(lhansen)
Comment on attachment 8786596 [details] [diff] [review]
sab-test-mode

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

I did look at this briefly, but this is pretty much a rubber stamp.
Attachment #8786596 - Flags: review?(lhansen) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0599e881d7a4
Odin: put asm.js Atomics/SAB support behind wasmTestMode (r=lth)
https://hg.mozilla.org/mozilla-central/rev/0599e881d7a4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Does this mean that handwritten JS will be able to access SAB+Atomics, and SAB+Atomics will proceed to DevEdition thru Stable, but SAB+Atomics accessed from within asm.js won't? 

So removing "use asm" will allow otherwise validating asm.js modules code to run with SAB?
Flags: needinfo?(luke)
Correct.  Technically, the asm.js will have a link-time validation error and then get re-parsed and re-run (and I think probably JIT-compiled normally) so it'll work even if you don't remove "use asm", but you waste the AOT compilation time.
Flags: needinfo?(luke)
That makes me feel surprised about how much this can achieve in providing security for breaking things since the handwritten JS side already defines the API.

Also, why an environment variable and not a pref? Prefs are easy to set up once, env. vars are generally not idiomatic way to configure these kind of experimental features.
(In reply to Jukka Jylänki from comment #6)
> That makes me feel surprised about how much this can achieve in providing
> security for breaking things since the handwritten JS side already defines
> the API.

But internally there are very different code paths and ones that will change a lot with wasm+threads.

> Also, why an environment variable and not a pref? Prefs are easy to set up
> once, env. vars are generally not idiomatic way to configure these kind of
> experimental features.

This mostly just fell out of using a JitOption.  It takes extra plumbing into Gecko to get a full about:config pref and I wasn't sure it'd be worth it.  I actually expected an env var might be *easier* (at least for testing purposes) since it's easier to set programmatically.
(In reply to Luke Wagner [:luke] from comment #7)
> (In reply to Jukka Jylänki from comment #6)
> > That makes me feel surprised about how much this can achieve in providing
> > security for breaking things since the handwritten JS side already defines
> > the API.
> 
> But internally there are very different code paths and ones that will change
> a lot with wasm+threads.

(By definition users shouldn't see internal changes, since they are internal?)

> > Also, why an environment variable and not a pref? Prefs are easy to set up
> > once, env. vars are generally not idiomatic way to configure these kind of
> > experimental features.
> 
> This mostly just fell out of using a JitOption.  It takes extra plumbing
> into Gecko to get a full about:config pref and I wasn't sure it'd be worth
> it.  I actually expected an env var might be *easier* (at least for testing
> purposes) since it's easier to set programmatically.

Could we do this a pref to accommodate, given that a notable partner commented it would be easier for them to do the opposite?
(In reply to Jukka Jylänki from comment #8)
> > > That makes me feel surprised about how much this can achieve in providing
> > > security for breaking things since the handwritten JS side already defines
> > > the API.
> > 
> > But internally there are very different code paths and ones that will change
> > a lot with wasm+threads.
> 
> (By definition users shouldn't see internal changes, since they are
> internal?)

Yes, but you are asking initially about security and security *does* involve implementation.

> Could we do this a pref to accommodate, given that a notable partner
> commented it would be easier for them to do the opposite?

That was assuming this would ship to end users; the solution for them is to not use asm.js+SAB (for multiple reasons).
If one enables JIT_OPTION_wasmTestMode=true environment variable to allow SAB in asm.js, are there side effects to what this does to the execution environment? (performance etc?) The env. var was not named JIT_OPTION_enable_sab_in_wasm=true, so I expect it's paired with enabling other features as well?
Actually that's a good point.  It doesn't hurt performance, but wasmTestMode does box up NaNs (so their uncanonicalized payloads can be tested) which could actually break asm.js code that wasn't expecting this.

So yeah, I'll write a patch to give SAB-in-asm.js its own JitOption.
Posted patch sab-test-modeSplinter Review
Attachment #8791605 - Flags: review?(lhansen)
With this patch, you'd set JIT_OPTION_asmJSAtomicsEnable=true
Comment on attachment 8791605 [details] [diff] [review]
sab-test-mode

rubberstamp over irc
Attachment #8791605 - Flags: review?(lhansen) → review+
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98d2d447a7a1
Odin: split out separate JitOptions.asmJSAtomicsEnable (rs=lth)
You need to log in before you can comment on or make changes to this bug.