1246140 - Atomics.store returns the truncated value, not the number value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Lars T Hansen [:lth]]

This is on Mac OS X 64-bit x86_64.  Found by v8 test suite.

By the spec, the value returned by Atomics.store (and Atomics.exchange) should be the ToNumber result of the input value, not the further truncation of that value to an integer.  The JIT appears to get this right, but the code in builtin/AtomicsObject.cpp does not - it returns the truncated value.

Comment 1

[Lars T Hansen [:lth]]

Sorry, not Atomics.exchange - it returns the old value of the cell, of course.

Comment 2

[Lars T Hansen [:lth]]

Actually the JIT gets it wrong too, but in a different way: it returns the input value unmodified.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1246140-test-cases.patch

I could fold these into basic-tests.js in the same directory, but that file is becoming large already.  (Also, perhaps setting ion.warmup.trigger is desirable? I couldn't tell, this seems to work OK.)

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1246140-interpreter.patch

Fix the interpreter path: break the conversion into two steps and return the result of the first step, as required by the spec.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1246140-jit.patch

Instead of trying to return the result of the ToInteger conversion, instead inline only when the result is unused or the input is already Int32.  This will apply to most uses and should not represent a performance problem in practice.  Programs that use Uint32 values outside the Int32 range *and* capture the return value are left out in the cold for now, but I'll file a followup bug to cover more cases properly.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8807501 [details] [diff] [review]
bug1246140-test-cases.patch

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

::: js/src/jit-test/tests/atomics/store-does-not-truncate-returnval.js
@@ +37,5 @@
> +    assertEq(Atomics.store(ia, 0, { valueOf: () => 3.7 }), 3);
> +    assertEq(ia[0], 3);
> +}
> +
> +for ( var i=0 ; i < 1000 ; i++ )

I think with we can do 10 instead of 1000 here?
That should trigger it on "--ion-eager --no-threads". Can you confirm?

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8807502 [details] [diff] [review]
bug1246140-interpreter.patch

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

The devil is in the details. r+

::: js/src/builtin/AtomicsObject.cpp
@@ +344,4 @@
>          return false;
>  
>      bool badType = false;
> +    int32_t result = ExchangeOrStore<op>(view->type(), JS::ToInt32(integerValue),

I think we can loose the "JS::"

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8807503 [details] [diff] [review]
bug1246140-jit.patch

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

::: js/src/jit/MCallOptimize.cpp
@@ +3090,2 @@
>      MDefinition* value = callInfo.getArg(2);
> +    if (!(BytecodeIsPopped(pc) || value->type() == MIRType::Int32)) {

can you transform !(x || y) to !x && !y

Comment 9

[Lars T Hansen [:lth]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> Comment on attachment 8807502 [details] [diff] [review]
> bug1246140-interpreter.patch
> 
> Review of attachment 8807502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +    int32_t result = ExchangeOrStore<op>(view->type(), JS::ToInt32(integerValue),
> 
> I think we can loose the "JS::"

You might think so, and I might too, but we'd both be wrong:

> AtomicsObject.cpp:347:56: error: use of undeclared identifier 'ToInt32'; did you mean 'JS::ToInt32'?

Comment 10

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1566da9ea3f

Comment 11

[Lars T Hansen [:lth]]

Try build is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95505b97467d

Comment 12

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fe9d1a41f3ae291e4926de88559411ea0f0cd1
Bug 1246140 - test for Atomics.store return value. r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/e66184c7eb45ae566a7e2bce62c9b109e945d405
Bug 1246140 - Return correct value from Atomics.store (interpreter implementation). r=h4writer

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e287fe4214720717b3def013366494fddd1e8ed
Bug 1246140 - Return correct value from Atomics.store (jit implementation). r=h4writer

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f7fe9d1a41f3
https://hg.mozilla.org/mozilla-central/rev/e66184c7eb45
https://hg.mozilla.org/mozilla-central/rev/1e287fe42147