Closed Bug 1260835 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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)
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)
That should be fine, Atomics is just a normal object.
Thanks for the heads up from me as well!
Flags: needinfo?(azakai)
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/fd3ca174abe0
Status: ASSIGNED → RESOLVED
Closed: 4 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?).
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.