1064493 - OdinMonkey: Confusing error message for SIMD import

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: test.js

On the attached testcase, the asm.js validator says this:

test.js:5:14 warning: asm.js type error: expecting m.*:
test.js:5:14 warning:   var i4add = global.SIMD.int32x4.add;
test.js:5:14 warning: ..............^

It's unclear what "m.*" is referring to.

Also, it's unclear whether this line should be a warning at all, since similar code:

  var i4 = global.SIMD.int32x4;
  var add = i4.add;

compiles without complaint.

Comment 1

[Luke Wagner [:luke]]

Well, the latter is the form we picked as the way to import SIMD ops.  We could allow the other too, if you felt like this was important.  Definitely should improve the error message, though.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for this bug! Indeed the error message is really not talkative at all. Regarding the forms, we've chosen the latter as it is shorter (in terms of code size) to reuse i4 (which is needed either in coercive form or in constructor form for having typex4 at all and thus using typex4.add). It could be added if necessary, but compilers-to-asmjs would probably prefer reducing code size.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Use failName rather than failf when validating asm.js globals

Doh.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Better wording for globals validation errors in asm.js

Attempt at a better explanation of what's really going on. Here, we should
never have more than 3 accessors uses (which is the case in the above test
case), so if we consider the particular case of global being a DOT, we can show
a better error message.

This error makes it look like an implementation detail,
although it's more a fact that can be deduced from the spec. Any ideas for a
better wording?

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8486490 [details] [diff] [review]
Use failName rather than failf when validating asm.js globals

Oops!

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8486520 [details] [diff] [review]
Better wording for globals validation errors in asm.js

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

::: js/src/asmjs/AsmJSValidate.cpp
@@ +3595,5 @@
>          PropertyName *mathOrSimd = DotMember(base);
>  
> +        if (!IsUseOfName(global, m.module().globalArgumentName())) {
> +            if (global->isKind(PNK_DOT))
> +                return m.fail(base, "maximum subscript depth should be not greater than 3 names");

How about: "imports can have at most two dot accesses (e.g., %s.Math.sin)" ?  (With failName and m.module().globalArgumentName().)

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/60219948cf27
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/989a7811d91b

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60219948cf27
https://hg.mozilla.org/mozilla-central/rev/989a7811d91b

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8486490 [details] [diff] [review]
Use failName rather than failf when validating asm.js globals

Approval Request Comment
[Feature/regressing bug #]: bug 992267
[User impact if declined]: unexpected behaviour (best case: confusing error messages, worst case: crashes)
[Describe test coverage new/current, TBPL]: all tests have been passing, locally and on TBPL, for a few days
[Risks and why]: no risk -- need uplift of patch in bug 1065883 as well
[String/UUID change made/needed]: n/a

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8486520 [details] [diff] [review]
Better wording for globals validation errors in asm.js

Approval Request Comment
[Feature/regressing bug #]: same bug
[User impact if declined]: worse error message
[Describe test coverage new/current, TBPL]: all green
[Risks and why]: no risk -- would help uplifting the patch in the dependent bug without having to rebase.
[String/UUID change made/needed]: n/a

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: better-error-message.patch

Approval Request Comment
[Feature/regressing bug #]: same bug
[User impact if declined]: worse error message in case code isn't valid asm.js
[Describe test coverage new/current, TBPL]: all tests green
[Risks and why]: no risk -- would help uplifting in the dependent bug
[String/UUID change made/needed]: n/a

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8489289 [details] [diff] [review]
better-error-message.patch

Aurora+

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8486490 [details] [diff] [review]
Use failName rather than failf when validating asm.js globals

Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19a3efe721ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/785d8cb2da69