Closed Bug 1088328 Opened 6 years ago Closed 6 years ago

OdinMonkey: accept (and ignore) non-semantic processing directives

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files)

For the reasons mentioned in
  http://discourse.specifiction.org/t/allow-arbitrary-processing-directives-in-asm-js-module-nested-functions
In particular, this would allow us to experiment later with AOT-compilation hints (which functions to compile with what optimization level).
This is a small tweak to fix a minor bug I found while testing the next patch.  Currently, if you have:
  function f() { "use asm"; function g() { "use strict" } return g }
it will fail to parse as asm.js (correctly) but it will not issue a warning.  The reason has to do with the way new directives are parsed: when we encounter "use strict" for the first time, the parser returns 'false' after setting strictMode in newDirectives.  The expectation is the caller restarts parsing (this time in strict mode).  However, in ParseFunction, we just assume 'return false' means "error", so we silently return false.
Attachment #8510636 - Flags: review?(benj)
Attached patch allow-directivesSplinter Review
Pretty simple patch.  The only annoying thing is that, at module top-level, we don't have ASTs, so we have to do it by token.
Attachment #8510637 - Flags: review?(benj)
Comment on attachment 8510636 [details] [diff] [review]
tweak-parser-function

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

Good catch.
Attachment #8510636 - Flags: review?(benj) → review+
Comment on attachment 8510637 [details] [diff] [review]
allow-directives

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

I can't wait for "use chaos"!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3892,5 @@
> +        if (!IsIgnoredDirectiveName(m.cx(), ts.currentToken().atom()))
> +            return m.fail(nullptr, "unsupported processing directive");
> +
> +        if (!ts.matchToken(TOK_SEMI))
> +            return m.fail(nullptr, "unexpected string literal");

This message doesn't seem inappropriate, as for instance this module will show this error:

function f(){"use asm"; "use asm".replace('asm', 'strict'); return {}}

What about "expected semicolon after string literal" or something like that?
Also, could you add a test for this particular path please?
Attachment #8510637 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
Good points, will do both.
You need to log in before you can comment on or make changes to this bug.