Closed Bug 1178793 Opened 4 years ago Closed 4 years ago

Atomics operations should throw on oob access

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: lth, Assigned: lth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

The atomics accesses have been following the "regular" accesses by returning undefined on OOB (coerced to 0 in asm.js), which the regular accesses do because they were designed to be temperamentally compatible with other JS behavior.  Everyone recognizes it's a pretty silly idea though.  The spec for atomic accesses should change, and the code should change too.

When the code changes, the return types for atomic accesses in AsmJSValidate.cpp should change from intish to int, because intish is required only to allow undefined to be in the result set.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1155176#c8.
Update: I don't know if this behavior has changed over the last 8 months or so, but in any case, there are some variant behaviors in final ES6.  In strict mode, OOB [[Set]] will throw, the check is performed in PutValue which throws if the setter returns false (oob) if the strict reference flag is set, for now assume the [[Get]] does the same thing.

The atomics operations should throw in non-strict mode as well, being new and shiny with no backward compatibility baggage.
Oh, and this is all asm.js - for regular JS we use standard patterns that ought to do the right thing.  Should verify that.  Note that Waldo says that we may not properly be implementing standard behavior yet.
Update: the draft spec has changed, atomic accesses now throw on OOB in strict and non-strict code alike.  Anyone's guess what TC39 will think but this really is the best behavior, so might as well go ahead and change the code.

(Should refactor bounds checking code when we do this, most of the back-ends are getting pretty messy.)
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
For what it's worth, the same choices were made for OOB on SIMD loads/stores, with the explanation that SIMD should be the fastest (i.e. no undefined value on OOB read). TC39 seemed to agree with this argument.
Attachment #8654871 - Flags: review?(luke)
Attached patch OdinSplinter Review
Luke, maybe you can review overall logic, x86, and x64.
Sean, maybe you can review the ARM code, turned out to be a small change.
Attachment #8654873 - Flags: review?(sstangl)
Attachment #8654873 - Flags: review?(luke)
A tiny fix - the bulk of this patch removes redundant casts from the test cases.
Attachment #8654874 - Flags: review?(luke)
Attachment #8654871 - Flags: review?(luke) → review+
Comment on attachment 8654873 [details] [diff] [review]
Odin

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

mmm, throw-on-oob semantics are so much nicer

::: js/src/jit/MIRGraph.cpp
@@ +123,4 @@
>  #if defined(ASMJS_MAY_USE_SIGNAL_HANDLERS_FOR_OOB)
> +    if (usesSignalHandlersForAsmJSOOB_) {
> +        if (access->isAtomicAccess())
> +            return access->needsBoundsCheck();

How about 'if (usesSignalHandlersForAsmJSOOB_ && !access->isAtomicAcces())' ?
Attachment #8654873 - Flags: review?(luke) → review+
Attachment #8654874 - Flags: review?(luke) → review+
Comment on attachment 8654873 [details] [diff] [review]
Odin

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

::: js/src/jit/MIR.h
@@ +13180,2 @@
>        : offset_(0), accessType_(accessType),
> +        needsBoundsCheck_(needsBoundsCheck), numSimdElems_(numSimdElems),

nit: move numSimdElems_ to separate line
Attachment #8654873 - Flags: review?(sstangl) → review+
https://hg.mozilla.org/mozilla-central/rev/51f25ea57b74
https://hg.mozilla.org/mozilla-central/rev/3e0f851d4443
https://hg.mozilla.org/mozilla-central/rev/9dc70a671d84
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
See Also: → 1204468
You need to log in before you can comment on or make changes to this bug.