Closed Bug 1255772 Opened 4 years ago Closed 4 years ago

wasm: implement Unreachable

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file)

Unreachable is pretty simple: we just generate a jump to an Unreachable trap stub which calls into the function generating the JSError.
Attachment #8729497 - Flags: review?(sunfish)
Comment on attachment 8729497 [details] [diff] [review]
unreachable.patch

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

::: js/src/js.msg
@@ +348,5 @@
>  MSG_DEF(JSMSG_WASM_TEXT_FAIL,          1, JSEXN_SYNTAXERR, "wasm text error: {0}")
>  MSG_DEF(JSMSG_WASM_BAD_IND_CALL,       0, JSEXN_ERR,     "wasm indirect call signature mismatch")
>  MSG_DEF(JSMSG_WASM_BAD_BUF_ARG,        0, JSEXN_TYPEERR, "first argument must be a typed array")
>  MSG_DEF(JSMSG_WASM_BAD_IMPORT_ARG,     0, JSEXN_TYPEERR, "second argument, if present, must be an object")
> +MSG_DEF(JSMSG_WASM_UNREACHABLE,        0, JSEXN_ERR,     "hit unreachable trap")

Optional bikeshed: "reached unreachable trap"?
Attachment #8729497 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/e3a45833901b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
AstSemantics.md and ml-proto both seem to name unreachable "unreachable", not "trap".  Is there a reason it's "trap"?
(In reply to Luke Wagner [:luke] from comment #4)
> AstSemantics.md and ml-proto both seem to name unreachable "unreachable",
> not "trap".  Is there a reason it's "trap"?

It's a mix of personal taste and liberty we've taken with the (still unofficial, unless I missed some announcement) text format:
- the name is shorter
- trap is both a verb and a noun; we don't seem to have many adjectives in the text format. Thinking about it as a verb makes it clearer that it has an effect (and provokes a side-effect), in my opinion.
- it's different from what it's in AstSemantics.md, the same way we renamed "if_else" to just "if" (the markdown files still refers to "if_else").

Of course, it's just a name in the *temporary* text format, so it sounds fine to change it if you want to.
It's unofficial, but we try to stick to the format parsed by ml-proto so we sexpr-wasm-prototype can all share .wast files.  The 'if'/'if_else' thing is because if_else is going away soon in the spec ('if' just has an optional 'else' child) so this is ml-proto just being a bit ahead of the game.  The patch which switches to postorder will make the change for us.
Would be nice to fix this to comply with the spec, this breaks building and running e.g. the skinning benchmark (binaryen emits according to the spec, then sm fails to parse it).
You need to log in before you can comment on or make changes to this bug.