Closed Bug 1257929 Opened 4 years ago Closed 4 years ago

Ensure that entry resume point have all their operands before the safeInsertTop location.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(3 files)

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)
Status: NEW → ASSIGNED
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)
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)
Attachment #8732984 - Flags: review?(bhackett1024) → review+
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+
(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.
(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)
Flags: needinfo?(nicolas.b.pierron)
Keywords: sec-want
Group: javascript-core-security
https://hg.mozilla.org/mozilla-central/rev/f379f47e538f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
re-opening this bug to land an extra patch to fix the false-positive assertion failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Attachment #8734395 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/ac258e975ea4
https://hg.mozilla.org/mozilla-central/rev/8180fae38b38
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.