1155176 - asm.js: Incorrect return types for many Atomic operations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

(Found this while fleshing out the test cases for bug 1131613.)

For asm.js only, AsmJSValidate chooses to interpret the return type of compareExchange as Signed even if the array is a SharedUint32Array.  This is wrong.  I may try to fix it as part of bug 1131613 where that code is changing anyway.

Comment 1

[Lars T Hansen [:lth]]

I think this is probably part of a deeper problem, where the atomic primitives do not conform to the asm.js LoadType / StoreType system, instead having some ad-hoc rules for result types.  This seems like it ought to be examined more generally.

Comment 2

[Lars T Hansen [:lth]]

A patch for the general problem has been offered on bug 1131613 (as part of the asm.js upper-level patch, see bug 1131613 comment 22).

Comment 3

[Lars T Hansen [:lth]]

Atomics.{load, store, compareExchange, add, sub, and, or, xor} currently all yield Type::Signed.  Apart from store, these all return the old value in the view, and so they really should have the same return type as a regular array load.  Regular array load, in turn, always returns intish for the integer types.

Consider the attached test case, which allows the result of compareExchange on a uint32 array to be observed as a signed value, this is incorrect (the code run as straight JS produces a different value).  If the result type of compareExchange was intish, the program would be forced to make an explicit interpretation.

Comment 4

[Lars T Hansen [:lth]]

Attachment: testBug1155176.js

Comment 5

[Lars T Hansen [:lth]]

Attachment: Correct return types + test cases

Return Type::Intish for everything that is like a load, and the rhsType for store.  This follows array load and store.

This is a simple change, and I think it's necessary, but it impacts existing code: It appears, from code I've gotten from Jukka, that Emscripten currently does not generate the necessary "casts" on the results of atomics, the way it does on heap accesses.  If these changes land then that has to change, and existing Emscripten-compiled code will be invalidated.  Don't know how much hardship that is.  Jukka?

Comment 6

[Jukka Jylänki]

No hardship at all, I can easily change the compiler to add in the casts. Invalidating existing code is not a problem either, we don't have any live pages that couldn't be rebuilt.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1155176-atomics-types.diff, v2

Accidentally left a comment and a couple of asserts off the test case, otherwise the same.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8623636 [details] [diff] [review]
bug1155176-atomics-types.diff, v2

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

I think the return type can be "int" instead: "int" means "a number [INT32_MIN, UINT32_MAX] or bool", "intish" means "something that must be coerced via ToInt32()/ToUint32() before use".  The reason the load type of a typed array is intish is b/c it can additionally return 'undefined' which requires the coercion to turn into a number.  In the case of Atomics, we throw, so that's not an option.  The main difference is that type "int" can be used directly w/o coercion by various operations that don't care about signedness (e.g. add).  r+ with that change (or intish, if you have a good reason I didn't realize).

Comment 9

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #8)
> Comment on attachment 8623636 [details] [diff] [review]
> bug1155176-atomics-types.diff, v2
> 
> Review of attachment 8623636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the return type can be "int" instead: "int" means "a number
> [INT32_MIN, UINT32_MAX] or bool", "intish" means "something that must be
> coerced via ToInt32()/ToUint32() before use".  The reason the load type of a
> typed array is intish is b/c it can additionally return 'undefined' which
> requires the coercion to turn into a number.  In the case of Atomics, we
> throw, so that's not an option.  The main difference is that type "int" can
> be used directly w/o coercion by various operations that don't care about
> signedness (e.g. add).  r+ with that change (or intish, if you have a good
> reason I didn't realize).

OK, noted.  I will land as is because the "good reason" is that we don't actually throw on out-of-bounds as of now (neither in spec nor code, though desired by all I think, at this point).  I will file a bug for that and note the change in return type that needs to be done.

Comment 10

[Lars T Hansen [:lth]]

Try run is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=203a706b70f4

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa71cf7b52b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fa71cf7b52b