Closed Bug 854396 Opened 9 years ago Closed 9 years ago

[OdinMonkey] Unsigned conversion is lost when passing to external functions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- fixed
firefox23 --- fixed

People

(Reporter: jlong, Assigned: luke)

Details

Attachments

(2 files)

Attached file test case
I might be missing something, but when using asm.js I would expect to see 4294967295 and not -1 in the attached test case. It appears the the unsigned conversion (-1 >>> 0) is lost if it's passed directly to an external function. It works fine if you assign it to a variable first.
OS: Mac OS X → All
Hardware: x86 → All
Summary: [asm.js] Unsigned conversion is lost when passing to external functions → [OdinMonkey] Unsigned conversion is lost when passing to external functions
Great find!  The problem here is that we're unconditionally treating int arguments as signed.  I'm actually surprised that this works if you assign to a variable first; that shouldn't work any different.  E.g.:

  function g() {
    var i = 4294967295;
    foo(i >>> 0);
  }

prints -1 for me (incorrectly).  Perhaps you were accidentally generating non-validating asm.js (thereby avoiding Odin and the bug)?

Anyhow, we'll get this fixed.
Oh, yes, I think you're right. I must have done `var x = -1 >>> 0` which is invalid, so it wasn't running under asm.js.

Thanks for confirming!
Right now, you can only return 'signed' and 'double', so, after talking with Alon and Dave, I think we should just apply the symmetric rule to arguments and remove 'unsigned' as a subtype of 'extern'.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #731986 - Flags: review?(sstangl)
Attachment #731986 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/0a1e54662a84
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 731986 [details] [diff] [review]
disallow in validation rules and test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 840282
User impact if declined: incorrect asm.js evaluation
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low
Attachment #731986 - Flags: approval-mozilla-aurora?
Comment on attachment 731986 [details] [diff] [review]
disallow in validation rules and test

Given the risk evaluation and targeted asm.js support in 22, approving.
Attachment #731986 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Luke Wagner [:luke] from comment #3)
> Right now, you can only return 'signed' and 'double', so, after talking with
> Alon and Dave, I think we should just apply the symmetric rule to arguments
> and remove 'unsigned' as a subtype of 'extern'.

Filed https://github.com/dherman/asm.js/issues/56 for making a corresponding change to the spec.
You need to log in before you can comment on or make changes to this bug.