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

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: luke)

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla24
x86_64
Mac OS X
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

print(g()());

normal gives 0
asm.js gives 5
(Reporter)

Comment 1

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

Comment 2

4 years ago
Created attachment 760078 [details] [diff] [review]
fix and test

D'oh!
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #760078 - Flags: review?(bbouvier)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/841ffd181e14
(Assignee)

Comment 5

4 years ago
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.
(Reporter)

Comment 6

4 years ago
How long is "a while"?  I want to do another round of asm correctness fuzzing before we ship odin.
(Reporter)

Comment 7

4 years ago
If this ends up being the last correctness bug in my list, I can apply the patch locally to fuzz with it.
(Assignee)

Comment 8

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

Comment 9

4 years ago
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

4 years ago
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.)
(Assignee)

Comment 11

4 years ago
Interesting idea.  What about unrealengine.com/html5?
(Reporter)

Comment 12

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

Comment 13

4 years ago
(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

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

Comment 15

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcb8cb1a868
https://hg.mozilla.org/mozilla-central/rev/cbcb8cb1a868
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.