OdinMonkey: Confusing error message for SIMD import

RESOLVED FIXED in Firefox 34

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: bbouvier)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(firefox32 unaffected, firefox33 unaffected, firefox34 fixed, firefox35 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8485947 [details]
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

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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.
Flags: needinfo?(benj)
(Assignee)

Comment 3

4 years ago
Created attachment 8486490 [details] [diff] [review]
Use failName rather than failf when validating asm.js globals

Doh.
Attachment #8486490 - Flags: review?(luke)
(Assignee)

Updated

4 years ago
Assignee: nobody → benj
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
Created attachment 8486520 [details] [diff] [review]
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?
Attachment #8486520 - Flags: review?(luke)
(Assignee)

Updated

4 years ago
Flags: needinfo?(benj)

Comment 5

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

Oops!
Attachment #8486490 - Flags: review?(luke) → review+

Comment 6

4 years ago
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().)
Attachment #8486520 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/60219948cf27
https://hg.mozilla.org/mozilla-central/rev/989a7811d91b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1065883
(Assignee)

Comment 9

4 years ago
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
Attachment #8486490 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

4 years ago
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
Attachment #8486520 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 11

4 years ago
Created attachment 8489289 [details] [diff] [review]
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
Attachment #8486520 - Attachment is obsolete: true
Attachment #8486520 - Flags: approval-mozilla-aurora?
Attachment #8489289 - Flags: review+
Attachment #8489289 - Flags: approval-mozilla-aurora?
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → affected
status-firefox35: --- → fixed
Comment on attachment 8489289 [details] [diff] [review]
better-error-message.patch

Aurora+
Attachment #8489289 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8486490 [details] [diff] [review]
Use failName rather than failf when validating asm.js globals

Aurora+
Attachment #8486490 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.