1178793 - Atomics operations should throw on oob access

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Lars T Hansen [:lth]]

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.

Comment 2

[Lars T Hansen [:lth]]

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.

Comment 3

[Lars T Hansen [:lth]]

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

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 5

[Lars T Hansen [:lth]]

Attachment: Interpreter and Ion

Comment 6

[Lars T Hansen [:lth]]

Attachment: Odin

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.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Correct return types + test cases

A tiny fix - the bulk of this patch removes redundant casts from the test cases.

Comment 8

[Lars T Hansen [:lth]]

Try is green, so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7bc4f63d947

Comment 9

[Luke Wagner [:luke]]

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())' ?

Comment 10

[Sean Stangl [:sstangl]]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51f25ea57b74
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0f851d4443
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc70a671d84

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51f25ea57b74
https://hg.mozilla.org/mozilla-central/rev/3e0f851d4443
https://hg.mozilla.org/mozilla-central/rev/9dc70a671d84