Closed
Bug 1257929
Opened 9 years ago
Closed 9 years ago
Ensure that entry resume point have all their operands before the safeInsertTop location.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: nbp, Assigned: nbp)
References
Details
Attachments
(3 files)
(deleted),
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
As described in Bug 1247909 comment 5, some optimizations rely on the safeInsertTop function to either remove (--ion-pgo=on) or add instructions after the set of instructions required by the resume points.
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•9 years ago
|
||
This patch adds extra assertions which verifies that the entry resume point of basic blocks do not have any operands which are below the safeInsertTop location. This is needed by the Branch Pruning optimization which uses the safeInsertTop instruction as a starting point for removing instructions from the basic blocks. Thus causing issues similar to Bug 1247909.
Attachment #8732982 -
Flags: review?(hv1989)
Assignee | ||
Comment 2•9 years ago
|
||
This patch changes Instruction Reorder phase to prevent moving instructions above the safeInsertTop location. The safeInsertTop location is used for producing bailout which are not dependent on any fallible execution. Moving instructions above the safeInsertTop instruction implies that a fallible instruction can sneak in and cause a bailout which has incomplete information.
Attachment #8732984 -
Flags: review?(bhackett1024)
Updated•9 years ago
|
Attachment #8732984 -
Flags: review?(bhackett1024) → review+
Comment 3•9 years ago
|
||
Comment on attachment 8732982 [details] [diff] [review] Add assertions to ensure the safety of entry resume point encoding. Review of attachment 8732982 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jit/IonAnalysis.cpp @@ +2285,5 @@ > +// To properly encode entry resume points, we have to ensure that all the > +// operands of the entry resume point are located before the safeInsertTop > +// location. > +static void > +AssertOperandsBeforeSafeInsertTop(MBasicBlock* block, MResumePoint* resume) It doesn't make sense to provide the argument "block" and "resume". block->entryResumePoint() == resume and resume->block() == block and it doesn't make sense to do this on an arbitrary resumepoint and arbitrary block. Plz remove one of the arguments. I think it makes most sense to keep the "block" argument. @@ +2293,5 @@ > + MInstruction* stop = block->safeInsertTop(); > + for (size_t i = 0, e = resume->numOperands(); i < e; ++i) { > + MDefinition* def = resume->getOperand(i); > + if (def->block() != block) > + continue; I think we can make this even: if (def->block() != block) { MOZ_ASSERT(def->block()->id() < block->id()); continue; } @@ +2675,5 @@ > } > > // Verify that the block resume points are dominated by their operands. > if (MResumePoint* resume = block->entryResumePoint()) { > + AssertOperandsBeforeSafeInsertTop(*block, resume); No need to do this in "AssertBasicGraphCoherency" and "AssertExtendedGraphCoherency". Since AssertExtendedGraphCoherency calls AssertBasicGraphCoherency. I.e. No need need to do it in AssertExtendedGraphCoherency.
Attachment #8732982 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 4•9 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #3) > @@ +2293,5 @@ > > + MInstruction* stop = block->safeInsertTop(); > > + for (size_t i = 0, e = resume->numOperands(); i < e; ++i) { > > + MDefinition* def = resume->getOperand(i); > > + if (def->block() != block) > > + continue; > > I think we can make this even: > > if (def->block() != block) { > MOZ_ASSERT(def->block()->id() < block->id()); > continue; > } This is already done by AssertResumePointDominatedByOperands.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #3) > Comment on attachment 8732982 [details] [diff] [review] > > +AssertOperandsBeforeSafeInsertTop(MBasicBlock* block, MResumePoint* resume) > > Plz remove one of the arguments. I think it makes most sense to keep the > "block" argument. I will keep the resume point argument, as the naming of the function sounds better with its signature. AssertOperandsBeforeSafeInsertTop(MBasicBlock* block) // What operands? AssertOperandsBeforeSafeInsertTop(MResumePoint* resume)
Assignee | ||
Updated•9 years ago
|
Group: javascript-core-security
https://hg.mozilla.org/integration/mozilla-inbound/rev/af93bc155267 https://hg.mozilla.org/integration/mozilla-inbound/rev/f379f47e538f
Backed out the added assertions in https://hg.mozilla.org/integration/mozilla-inbound/rev/dc60c84a3b2f because they were failing like https://treeherder.mozilla.org/logviewer.html#?job_id=24454331&repo=mozilla-inbound
Flags: needinfo?(nicolas.b.pierron)
Comment 8•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f379f47e538f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 9•9 years ago
|
||
re-opening this bug to land an extra patch to fix the false-positive assertion failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•9 years ago
|
||
This patch special case the osr block in range analysis, as the alternative would be to add 5 extra conditions in safe-insert-top while loop, which would only be dedicated to the OSR block instructions. Note this phase only add instruction if there is a Range information, which is not the case for any instruction above the MStart, and thus should not be concerned by safe-insert-top.
Attachment #8734395 -
Flags: review?(hv1989)
Updated•9 years ago
|
Attachment #8734395 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e90a5a2969f0c5f0bfe558939c50ba174da15701
Flags: needinfo?(nicolas.b.pierron)
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac258e975ea4 https://hg.mozilla.org/integration/mozilla-inbound/rev/8180fae38b38
Comment 13•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac258e975ea4 https://hg.mozilla.org/mozilla-central/rev/8180fae38b38
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•