Closed Bug 1246140 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
Sorry, not Atomics.exchange - it returns the old value of the cell, of course.
Actually the JIT gets it wrong too, but in a different way: it returns the input value unmodified.
Priority: -- → P2
Blocks: 1225039
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.)
Attachment #8807501 - Flags: review?(hv1989)
Fix the interpreter path: break the conversion into two steps and return the result of the first step, as required by the spec.
Attachment #8807502 - Flags: review?(hv1989)
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.
Attachment #8807503 - Flags: review?(hv1989)
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?
Attachment #8807501 - Flags: review?(hv1989) → review+
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::"
Attachment #8807502 - Flags: review?(hv1989) → review+
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
Attachment #8807503 - Flags: review?(hv1989) → review+
(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'?
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
You need to log in before you can comment on or make changes to this bug.