Closed Bug 1155176 Opened 5 years ago Closed 5 years ago

asm.js: Incorrect return types for many Atomic operations

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

(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.
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.
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).
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.
Attached file testBug1155176.js
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Summary: asm.js: Atomics.compareExchange() on uint32 array treats the loaded value as signed → asm.js: Incorrect return types for many Atomic operations
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?
Attachment #8623624 - Flags: review?(luke)
Attachment #8623624 - Flags: feedback?(jujjyl)
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.
Attachment #8623624 - Flags: feedback?(jujjyl) → feedback+
Accidentally left a comment and a couple of asserts off the test case, otherwise the same.
Attachment #8623624 - Attachment is obsolete: true
Attachment #8623624 - Flags: review?(luke)
Attachment #8623636 - Flags: review?(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).
Attachment #8623636 - Flags: review?(luke) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/5fa71cf7b52b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Target Milestone: mozilla41 → mozilla42
You need to log in before you can comment on or make changes to this bug.