Closed
Bug 1260835
Opened 9 years ago
Closed 9 years ago
Remove Atomics.{OK,NOTEQUAL,TIMEDOUT}, return strings from Atomics.futexWait instead
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
(deleted),
patch
|
jolesen
:
review+
|
Details | Diff | Splinter Review |
This is a spec change (see https://github.com/tc39/ecmascript_sharedmem/issues/69#issuecomment-200410800 for details).
Jukka, Alon - this will affect you. A credible check that makes code run on both older and upcoming Firefox is that if Atomics.OK === undefined then you must expect string values to be returned from futexWait.
Flags: needinfo?(jujjyl)
Flags: needinfo?(azakai)
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 1•9 years ago
|
||
Thanks for the heads up! Proposing the following kind of fix: https://github.com/kripken/emscripten/pull/4212 . Is that kosher as well?
Flags: needinfo?(jujjyl)
Assignee | ||
Comment 2•9 years ago
|
||
That should be fine, Atomics is just a normal object.
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8737022 -
Flags: review?(jolesen)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Comment 5•9 years ago
|
||
Comment on attachment 8737022 [details] [diff] [review]
Atomics.wait returns strings + remove symbolic constants
Review of attachment 8737022 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/builtin/AtomicsObject.cpp
@@ +818,5 @@
> + break;
> + case AtomicsObject::FutexTimedOut:
> + r.setString(cx->names().futexTimedOut);
> + break;
> + }
Perhaps add a default case to this switch to crash on unexpected result codes?
::: js/src/builtin/AtomicsObject.h
@@ +23,5 @@
> // The error values must be negative because APIs such as futexWaitOrRequeue
> // return a value that is either the number of tasks woken or an error code.
> + enum FutexWaitResult {
> + FutexOK,
> + FutexTimedOut
It's a bit confusing that this enumeration doesn't cover all the possible wait result codes, even if you don't need them all here.
Attachment #8737022 -
Flags: review?(jolesen) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Jakob Stoklund Olesen [:jolesen] from comment #5)
> Comment on attachment 8737022 [details] [diff] [review]
> Atomics.wait returns strings + remove symbolic constants
>
>
> It's a bit confusing that this enumeration doesn't cover all the possible
> wait result codes, even if you don't need them all here.
An excellent point. The actual bug is that the result codes no longer belong in AtomicsObject, but in FutexRuntime, which can only return OK or TimedOut.
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3ca174abe074fbde07dd3fb7efe1d5f5da477a
Bug 1260835 - Atomics.wait returns strings + remove symbolic constants. r=jolesen
Comment 9•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 10•9 years ago
|
||
Updated
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/futexWait
Not going to add any compatibility note as this is an experimental API anyway and we are shipping the old version of this for 2 releases only (will this be uplifted?).
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 11•9 years ago
|
||
Will let the changes ride the train with 48, given everything else that changes at the same time.
You need to log in
before you can comment on or make changes to this bug.
Description
•