Closed
Bug 1148232
Opened 8 years ago
Closed 8 years ago
OdinMonkey: OOB and related cleanup patches
Categories
(Core :: JavaScript Engine: JIT, enhancement)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: sunfish, Assigned: sunfish)
References
Details
Attachments
(6 files)
1.02 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
7.18 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
13.80 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.25 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
9.78 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
In preparation for bug 1145676, I made several relatively minor patches.
Assignee | ||
Comment 1•8 years ago
|
||
This just updates an asm.js error message.
Attachment #8584197 -
Flags: review?(luke)
Assignee | ||
Comment 2•8 years ago
|
||
Several tests were still using the legacy syntax for HEAP8 accesses without the shift. This patch adds comments mentioning this, and adds equivalent tests using the new syntax.
Attachment #8584199 -
Flags: review?(luke)
Assignee | ||
Comment 3•8 years ago
|
||
This patch harmonizes the legacy path in CheckArrayAccess and the main path, and updates the error messages to point to the more relevant expression node.
Attachment #8584201 -
Flags: review?(luke)
Assignee | ||
Comment 4•8 years ago
|
||
This patch splits out the index mask insertion code from CheckArrayAccess so that it can always be called immediately prior to the actual array access, instead of sometimes prior to the expansion of other operands. The PrepareArrayIndex function this patch introduces will also be useful for inserting bounds checks in later patches.
Attachment #8584208 -
Flags: review?(luke)
Assignee | ||
Comment 5•8 years ago
|
||
This patch just deletes some unused code.
Attachment #8584209 -
Flags: review?(luke)
Assignee | ||
Comment 6•8 years ago
|
||
This patch marks more of the disassembly code as cold, and uses more MOZ_RELEASE_ASSERTS so that more checks are included in release builds.
Attachment #8584211 -
Flags: review?(luke)
Assignee | ||
Updated•8 years ago
|
Attachment #8584208 -
Attachment description: asmjs → asmjs-put-mask-next-to-access.patch
![]() |
||
Updated•8 years ago
|
Attachment #8584197 -
Flags: review?(luke) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8584199 -
Flags: review?(luke) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8584201 -
Flags: review?(luke) → review+
![]() |
||
Comment 7•8 years ago
|
||
Comment on attachment 8584208 [details] [diff] [review] asmjs-put-mask-next-to-access.patch Review of attachment 8584208 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/AsmJSValidate.cpp @@ +4493,5 @@ > "change-heap function (0x%x - 0x%x)", > f.m().minHeapLength(), f.m().module().maxHeapLength()); > } > > + *mask = -1; // no mask How about define a 'static const int32_t NoMask = -1;' above? @@ +4536,5 @@ > // For legacy compatibility, accept Int8/Uint8 accesses with no shift. > if (TypedArrayShift(*viewType) != 0) > return f.fail(indexExpr, "index expression isn't shifted; must be an Int8/Uint8 access"); > > + MOZ_ASSERT(*mask == -1); Could be NoMask w/ abovementioned change. @@ +4567,5 @@ > +} > + > +static void > +PrepareArrayIndex(FunctionCompiler &f, MDefinition **def, > + NeedsBoundsCheck needsBoundsCheck, int32_t mask = -1) I think needsBoundsCheck can be on the previous line. Also, I'd be a bit happier without a default arg (so the choice to leave off the mask is explicit); with 'NoMask' this will be more readable. @@ +4573,3 @@ > // Don't generate the mask op if there is no need for it which could happen for > + // a shift of zero or a SIMD access. > + if (mask != -1) NoMask @@ +4901,5 @@ > static bool > CheckSharedArrayAtomicAccess(FunctionCompiler &f, ParseNode *viewName, ParseNode *indexExpr, > Scalar::Type *viewType, MDefinition** pointerDef, > + NeedsBoundsCheck *needsBoundsCheck, > + int32_t *mask) I think 'mask' can be on the previous line.
Attachment #8584208 -
Flags: review?(luke) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8584209 -
Flags: review?(luke) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8584211 -
Flags: review?(luke) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Suggested fixes made. Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/768fcbee6319 https://hg.mozilla.org/integration/mozilla-inbound/rev/f41fff953bf1 https://hg.mozilla.org/integration/mozilla-inbound/rev/da24bccfcaf0 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2abf02e8df8 https://hg.mozilla.org/integration/mozilla-inbound/rev/982ae8f096bd https://hg.mozilla.org/integration/mozilla-inbound/rev/46872ab97179
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/768fcbee6319 https://hg.mozilla.org/mozilla-central/rev/f41fff953bf1 https://hg.mozilla.org/mozilla-central/rev/da24bccfcaf0 https://hg.mozilla.org/mozilla-central/rev/d2abf02e8df8 https://hg.mozilla.org/mozilla-central/rev/982ae8f096bd https://hg.mozilla.org/mozilla-central/rev/46872ab97179
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•