Closed Bug 1088328 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: luke, Assigned: luke)



(2 files)

For the reasons mentioned in
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]

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

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

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

I can't wait for "use chaos"!

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3892,5 @@
> +        if (!IsIgnoredDirectiveName(, ts.currentToken().atom()))
> +            return, "unsupported processing directive");
> +
> +        if (!ts.matchToken(TOK_SEMI))
> +            return, "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.