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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[James Long (:jlongster)]

Attachment: 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.

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[James Long (:jlongster)]

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!

Comment 3

[Luke Wagner [:luke]]

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

Comment 4

[Luke Wagner [:luke]]

Attachment: disallow in validation rules and test

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1e54662a84

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a1e54662a84

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Alex Keybl [:akeybl]]

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.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a2ac4c2639c

Comment 10

[Jesse Ruderman]

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