Closed Bug 1246750 Opened 4 years ago Closed 4 years ago

Atomics.futexWakeOrRequeue() permutes the index2 and value arguments

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- unaffected
firefox45 --- unaffected
firefox46 + fixed
firefox47 + fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The spec says that the signature is (array, index1, count, index2, value) but SpiderMonkey uses (array, index2, count, value, index2).

It appears that the (trivial) test case I wrote tonight for futexWakeOrRequeue() is the first test case for that function, which is embarrassing - the function has been in Firefox for a year.
Spec text:
https://tc39.github.io/ecmascript_sharedmem/shmem.html#Atomics.futexWakeOrRequeue

Test case (specifically the call to futexWakeOrRequeue that passes "Idx" as the fourth parameter):
https://github.com/tc39/ecmascript_sharedmem/blob/master/test262/futex-range.js
(In reply to Lars T Hansen [:lth] from comment #0)
> The spec says that the signature is (array, index1, count, index2, value)
> but SpiderMonkey uses (array, index2, count, value, index2).
                                ^^^^^^^
                                index1, of course
Attachment #8717128 - Flags: review?(bbouvier)
Comment on attachment 8717128 [details] [diff] [review]
fix argument ordering to futexWakeOrRequeue

Review of attachment 8717128 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, but can we have tests this time, please? :-)
If the intent is to make the tc39 tests running, can we have a way to automatically import them at compile time on treeherder?
If the intent is to have them later when everything is working, maybe we could add subsets of the tests in the meanwhile?
Attachment #8717128 - Flags: review?(bbouvier) → review+
I can at least add the trivial/shallow test case that uncovered this.

The existing test cases for futex functionality live in tests/shell/futex.js and I *think* these are run on check-in.  They are adequate but incomplete.

In general bug 1135026 (DOM/HTML futex test cases) still needs attention, though I think it is probably best to address that via the tc39 test cases.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6b1b2a915774e8e2fd0b73b2a98a389bdc1318
Bug 1246750 - fix argument ordering to futexWakeOrRequeue + test cases. r=bbouvier
[Tracking Requested - why for this release]: Bad API bug that should be corrected before release
Comment on attachment 8717128 [details] [diff] [review]
fix argument ordering to futexWakeOrRequeue

Approval Request Comment
[Feature/regressing bug #]: Shared memory and atomics
[User impact if declined]: API not compatible with other browsers
[Describe test coverage new/current, TreeHerder]: Test cases in js/src/test/shell/futex-api.js
[Risks and why]: No risk, just fixed the ordering of arguments
[String/UUID change made/needed]: None
Attachment #8717128 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/4a6b1b2a9157
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Tracking for 46, can't tell if this is a regression or not but since we aim to uplift, I'd like to keep an eye on this bug.
Comment on attachment 8717128 [details] [diff] [review]
fix argument ordering to futexWakeOrRequeue

Fix for API, has test coverage, ok to uplift to aurora
Attachment #8717128 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.