Closed
Bug 1147353
Opened 9 years ago
Closed 9 years ago
Odin: simplify the masked index bounds check test.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: dougc, Assigned: dougc)
Details
Attachments
(1 file, 1 obsolete file)
1.93 KB,
patch
|
dougc
:
review+
|
Details | Diff | Splinter Review |
The test of a masked index against the known minimum heap length is overly complex and while the recent update fixes an issue it is still a little too conservative and misses some opportunities.
Assignee | ||
Comment 1•9 years ago
|
||
This code was way too complex, sorry. It just needed to compare the mask to the length and avoid negative indexes when the top bit of the mask is set.
Attachment #8583043 -
Flags: review?(sphink)
Comment 2•9 years ago
|
||
Comment on attachment 8583043 [details] [diff] [review] Simplify the masked index bounds check test. Review of attachment 8583043 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense to me. A slightly different way of stating it is that ANDing the mask with the index can only decrease the mask if it's unsigned, so mask ≤ max => (x & mask) ≤ max since (mask & x) ≤ mask ≤ max. Pretty much the same as what your comment says. And yet -- I'd rather luke stamp this too, if you don't mind. My paranoia knows no bounds. ::: js/src/asmjs/AsmJSValidate.cpp @@ +4444,5 @@ > uint32_t mask2; > if (IsLiteralOrConstInt(f, maskNode, &mask2)) { > // Flag the access to skip the bounds check if the mask ensures that an 'out of > + // bounds' access can not occur based on the current heap length constraint. The > + // maxium of a masked index is the mask itself so simply compare the mask to the *maximum
Attachment #8583043 -
Flags: review?(sphink)
Attachment #8583043 -
Flags: review?(luke)
Attachment #8583043 -
Flags: review+
Comment 3•9 years ago
|
||
(In reply to Steve Fink [:sfink, :s:] from comment #2) > My paranoia knows no bounds. NO_BOUNDS_CHECK, even?
Comment 4•9 years ago
|
||
Comment on attachment 8583043 [details] [diff] [review] Simplify the masked index bounds check test. Much simpler.
Attachment #8583043 -
Flags: review?(luke) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Fix the source comment. Carrying forward r+.
Attachment #8583043 -
Attachment is obsolete: true
Attachment #8583435 -
Flags: review+
Assignee | ||
Comment 6•9 years ago
|
||
Requesting checkin. Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0748011b8259
Keywords: checkin-needed
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4aab7d129f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7e4aab7d129f
Status: NEW → RESOLVED
Closed: 9 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
•