Closed
Bug 1246140
Opened 9 years ago
Closed 8 years ago
Atomics.store returns the truncated value, not the number value
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
1.61 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
2.81 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Sorry, not Atomics.exchange - it returns the old value of the cell, of course.
Assignee | ||
Comment 2•9 years ago
|
||
Actually the JIT gets it wrong too, but in a different way: it returns the input value unmodified.
Assignee | ||
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 3•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
Attachment #8807502 -
Flags: review?(hv1989)
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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•8 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 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Try build is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=95505b97467d
Assignee | ||
Comment 12•8 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•8 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
Closed: 8 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.
Description
•