Closed Bug 864600 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: luke, Assigned: luke)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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?
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.)
Attached 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.
Attached 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.
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)
Attached 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 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+
https://hg.mozilla.org/mozilla-central/rev/66964658a097
https://hg.mozilla.org/mozilla-central/rev/42bcb974cfdd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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.