Closed Bug 878433 Opened 8 years ago Closed 8 years ago

OdinMonkey: (signed % signed) should be intish, not int

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: luke)

References

Details

(Keywords: testcase)

Attachments

(1 file)

function g()
{
  "use asm"
  function f()
  {
    return (5 + (1 % 0))|0;
  }
  return f;
}

print(g()());

normal gives 0
asm.js gives 5
I think this is a spec bug.  (signed % signed) is int, but should be intish, because you don't want to use NaN uncoerced.
Attached patch fix and testSplinter Review
D'oh!
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #760078 - Flags: review?(bbouvier)
Attachment #760078 - Attachment is patch: true
Comment on attachment 760078 [details] [diff] [review]
fix and test

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

Nice catch!
Attachment #760078 - Flags: review?(bbouvier) → review+
Oops, I forgot to check whether this type system fix breaks anything and it does; everything.  So, for now, backing out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9786c829bf3c

This is a pretty corner case correctness issue, so I think we should just fix the issue in Emscripten, wait a while, and then land this fix later after uptake.
How long is "a while"?  I want to do another round of asm correctness fuzzing before we ship odin.
If this ends up being the last correctness bug in my list, I can apply the patch locally to fuzz with it.
Mostly until unrealengine.com/html5 gets an updated build from an updated Emscripten.
If you add a "% by a constant" rule, like * has, then you can keep the int type and avoid breaking things.  I suggest adding, in section 6.6.8 of the spec:

> A MultiplicativeExpression node
>
>   expr:MultiplicativeExpression % n:NumericLiteral
>
> validates as type int if n is nonzero and validates as a subtype of signed and expr 
> validates as a subtype of signed, or else if n is nonzero and validates as a subtype
> of unsigned and expr validates as a subtype of unsigned.
If you add the above rule, only 5 sites in bullet.js need to be fixed; the other asm.js benchmarks from arewefastyet typecheck without modification.  (In particular, zlib.js contains a large number of expressions ending in '% 65521' which would otherwise fail to typecheck.)
Interesting idea.  What about unrealengine.com/html5?
Filed https://github.com/dherman/asm.js/issues/69 to fix the spec.
(In reply to Luke Wagner [:luke] from comment #11)
> Interesting idea.  What about unrealengine.com/html5?

At least one failure, even with the workaround from comment 9.  For example:
  b = i - (i >>> 0) % (h >>> 0) | 0;
Another example from unrealengine:
  p = (c[f >> 2] | 0) % (q | 0);
https://hg.mozilla.org/mozilla-central/rev/cbcb8cb1a868
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.