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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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

print(g()());

normal gives 0
asm.js gives 5

Comment 1

[Jesse Ruderman]

I think this is a spec bug.  (signed % signed) is int, but should be intish, because you don't want to use NaN uncoerced.

Comment 2

[Luke Wagner [:luke]]

Attachment: fix and test

D'oh!

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 760078 [details] [diff] [review]
fix and test

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

Nice catch!

Comment 4

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/841ffd181e14

Comment 5

[Luke Wagner [:luke]]

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.

Comment 6

[Jesse Ruderman]

How long is "a while"?  I want to do another round of asm correctness fuzzing before we ship odin.

Comment 7

[Jesse Ruderman]

If this ends up being the last correctness bug in my list, I can apply the patch locally to fuzz with it.

Comment 8

[Luke Wagner [:luke]]

Mostly until unrealengine.com/html5 gets an updated build from an updated Emscripten.

Comment 9

[cscott]

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.

Comment 10

[cscott]

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

Comment 11

[Luke Wagner [:luke]]

Interesting idea.  What about unrealengine.com/html5?

Comment 12

[Jesse Ruderman]

Filed https://github.com/dherman/asm.js/issues/69 to fix the spec.

Comment 13

[cscott]

(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;

Comment 14

[cscott]

Another example from unrealengine:
  p = (c[f >> 2] | 0) % (q | 0);

Comment 15

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcb8cb1a868

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cbcb8cb1a868