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

RESOLVED FIXED in mozilla23



6 years ago
6 years ago


(Reporter: luke, Assigned: luke)


(Blocks: 1 bug)


Firefox Tracking Flags

(Not tracked)



(2 attachments, 2 obsolete attachments)



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

Comment 2

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

Comment 3

6 years ago
Posted patch coerce calls (WIP) (obsolete) — Splinter Review
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.

Comment 4

6 years ago
Posted patch tweak doublish rules (WIP) (obsolete) — Splinter Review
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.
I fixed the few small missing coercions on the emscripten test suite that popped up with these two patches applied.

Comment 6

6 years ago
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)

Comment 7

6 years ago
Posted patch coerce callsSplinter Review
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)

Comment 8

6 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)
Attachment #742641 - Flags: review?(sstangl) → review+
Attachment #742640 - Flags: review?(sstangl) → review+
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Comment 10

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