Add a pref that makes asm.js validation failure throw an error?

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jukka Jylänki, Assigned: bbouvier)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
It would be very useful to have a developer pref javascript.options.throw_on_asmjs_validation_error, which, if enabled, would cause an exception to be thrown is asm.js validation fails. This would make it easier in Emscripten and pthreads suites to catch situations where due to some bug validation has failed, by installing a window.onerror() handler to catch the validation error and report a failure back to the harness.

In Emscripten, we do validate asm.js on command line with SpiderMonkey before executing, however that is not quite good enough, since it uses a separately compiled executable, and sometimes a disparity happens between current FF Nightly vs whatever compiled SpiderMonkey one happens to have set up locally. If there was such a pref, we could be certain that validation tests are done in the same engine that we are actually running in.

Comment 1

2 years ago
Sounds fine to me
(Assignee)

Comment 2

2 years ago
Created attachment 8666640 [details] [diff] [review]
1.throw_on_asmjs_validation_failure.patch

This adds an option to throw on validation failures (preconditions errors (e.g. the asm.js module function is a generator) and actual validation failures). Note that it only applies to validation failures, not link failures or directive failures (which are handled in several other places).

The suggested name is a bit long ("throw_on_asmjs_validation_failure"), but I didn't see how to make it shorter without making it less explicit.
Assignee: nobody → benj
Status: NEW → ASSIGNED
Attachment #8666640 - Flags: review?(luke)
(Assignee)

Comment 3

2 years ago
Created attachment 8666651 [details] [diff] [review]
2.add.about-config.prefs.patch

This creates a boolean pref for enabling "throw_on_asmjs_validation_failure", which is specified in the previous comment and implemented in the first patch.
Attachment #8666651 - Flags: review?(bzbarsky)

Comment 4

2 years ago
Comment on attachment 8666640 [details] [diff] [review]
1.throw_on_asmjs_validation_failure.patch

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

Nice!
Attachment #8666640 - Flags: review?(luke) → review+
(Assignee)

Comment 5

2 years ago
Thinking about it, another alternative is just to set javascript.options.werror to true (it must be a boolean value). Even though asm.js compilation also warns when it succeeds, the error message could be pattern-matched and the successful compilation case could be put aside. Would that be enough?
Flags: needinfo?(jujjyl)
(Reporter)

Comment 6

2 years ago
Oh, interesting. I did not know such an option already existed. Looks like the pref javascript.options.werror is hidden and must be added manually. With brief testing, it does look like it is usable to catch asm.js validation error in window.onerror(e), but the error it receives is a string "Script error." which is not ideal, but could work.

Is javascript.options.werror documented somewhere? What does it do exactly? Could it have unintended consequences?
Flags: needinfo?(jujjyl)
werror could do whatever it wants, since nothing really defines when the JS engine will do a warning.  It's particularly bad in "strict warnings" mode....
Comment on attachment 8666651 [details] [diff] [review]
2.add.about-config.prefs.patch

You need to add this to all.js to have it show up in about:config, right?

r=me with that
Attachment #8666651 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/b96ac06ea355
https://hg.mozilla.org/mozilla-central/rev/9bae4db3f286
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Reporter)

Comment 11

2 years ago
Thanks for doing this :bbouvier! Integrated the feature to emrun at https://github.com/juj/emrun/commit/ca9aa1767cb60d9e717ff5574b09c556ecfbe83e and Emscripten unit test suites at http://clb.demon.fi:8112/waterfall .
You need to log in before you can comment on or make changes to this bug.