Remove Atomics.{OK,NOTEQUAL,TIMEDOUT}, return strings from Atomics.futexWait instead

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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)

Comment 1

3 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

3 years ago
That should be fine, Atomics is just a normal object.
Thanks for the heads up from me as well!
Flags: needinfo?(azakai)
Assignee

Updated

3 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
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

3 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 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd3ca174abe074fbde07dd3fb7efe1d5f5da477a
Bug 1260835 - Atomics.wait returns strings + remove symbolic constants. r=jolesen

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd3ca174abe0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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?).
Assignee

Comment 11

3 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.