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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

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).

Comment 1

[Luke Wagner [:luke]]

Attachment: tweak-parser-function

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.

Comment 2

[Luke Wagner [:luke]]

Attachment: allow-directives

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.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8510636 [details] [diff] [review]
tweak-parser-function

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

Good catch.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 5

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
Good points, will do both.

Comment 6

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1090f3645eb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcd719ea9990

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1090f3645eb3
https://hg.mozilla.org/mozilla-central/rev/fcd719ea9990