OdinMonkey: change asm.js validation to require explicit coercion of all call expressions

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Right now, only FFI calls require coercion, but a single-pass asm.js parser wants the result of all calls (whose return value is used) to be coerced.  Apparently Emscripten already does this, so existing code *should* continue to validate (but we'll see).

Comment 1

4 years ago
I noticed while verifying this issue that emscripten only coerces arguments into library calls, not other asm functions,

  otherAsmFunc($x, $y);
  libraryFunc($x|0, +$y);

But I think we do not need this for a single-pass parser, since you know during the very beginning which are library calls and which are not, and know the local variable types anyhow?
(Assignee)

Comment 2

4 years ago
That's right, we should know the type of the actual argument expressions; FFI calls only need the coercions because they can observe signed/unsigned.  (The coercion isn't necessary for double args, btw, double <: extern.)
(Assignee)

Comment 3

4 years ago
Created attachment 741095 [details] [diff] [review]
coerce calls (WIP)

This patch makes the change.  I still need to fix up all the unit tests to validate with the new rules.

Alon: much Emscripten-generated asm.js validates with these new rules (bullet, zlib, skinning), but the big apps seem to hit a few missing corner cases (involving assignment).  If you attach the patch, you can see the type errors.
(Assignee)

Comment 4

4 years ago
Created attachment 741097 [details] [diff] [review]
tweak doublish rules (WIP)

This patch restricts ~~ (double-to-int conversion) and binary - to only take double.  The reasoning is that, for any expression, only one of {intish,doublish} should be accepted.

Comment 5

4 years ago
I fixed the few small missing coercions on the emscripten test suite that popped up with these two patches applied.
(Assignee)

Comment 6

4 years ago
Created attachment 742640 [details] [diff] [review]
tweak doublish rules

The spec change here is to change doublish to double in the signature of ~~ and binary -.  The reasoning is that there must be only one *ish type flowing into these operator signatures or else it is ambiguous, if the operand is a call, what the implied return type is.
Attachment #741097 - Attachment is obsolete: true
Attachment #742640 - Flags: review?(sstangl)
(Assignee)

Comment 7

4 years ago
Created attachment 742641 [details] [diff] [review]
coerce calls

See the big comment.
Attachment #741095 - Attachment is obsolete: true
Attachment #742640 - Attachment is obsolete: true
Attachment #742640 - Flags: review?(sstangl)
Attachment #742641 - Flags: review?(sstangl)
(Assignee)

Comment 8

4 years ago
Comment on attachment 742640 [details] [diff] [review]
tweak doublish rules

Oops, did not mean to obsolete this one.
Attachment #742640 - Attachment is obsolete: false
Attachment #742640 - Flags: review?(sstangl)

Updated

4 years ago
Attachment #742641 - Flags: review?(sstangl) → review+

Updated

4 years ago
Attachment #742640 - Flags: review?(sstangl) → review+

Comment 9

4 years ago
https://hg.mozilla.org/mozilla-central/rev/66964658a097
https://hg.mozilla.org/mozilla-central/rev/42bcb974cfdd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 10

4 years ago
This change doesn't seem to have made it into the spec at http://asmjs.org/spec/latest/ yet?  (And that spec could use a changelog...)
You need to log in before you can comment on or make changes to this bug.