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

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Sorry, not Atomics.exchange - it returns the old value of the cell, of course.
(Assignee)

Comment 2

2 years ago
Actually the JIT gets it wrong too, but in a different way: it returns the input value unmodified.
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Blocks: 1225039
(Assignee)

Comment 3

2 years ago
Created attachment 8807501 [details] [diff] [review]
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.)
Attachment #8807501 - Flags: review?(hv1989)
(Assignee)

Comment 4

2 years ago
Created attachment 8807502 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Attachment #8807502 - Flags: review?(hv1989)
(Assignee)

Comment 5

2 years ago
Created attachment 8807503 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

2 years ago
(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'?
(Assignee)

Comment 12

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7fe9d1a41f3
https://hg.mozilla.org/mozilla-central/rev/e66184c7eb45
https://hg.mozilla.org/mozilla-central/rev/1e287fe42147
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.