Closed
Bug 1178793
Opened 9 years ago
Closed 9 years ago
Atomics operations should throw on oob access
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
sstangl
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8654871 -
Flags: review?(luke)
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
A tiny fix - the bulk of this patch removes redundant casts from the test cases.
Attachment #8654874 -
Flags: review?(luke)
Assignee | ||
Comment 8•9 years ago
|
||
Try is green, so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7bc4f63d947
Updated•9 years ago
|
Attachment #8654871 -
Flags: review?(luke) → review+
Comment 9•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8654874 -
Flags: review?(luke) → review+
Comment 10•9 years ago
|
||
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+
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
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: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•