863961 - Assertion failure: !info.argsObjAliasesFormals(), at ion/IonAnalysis.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

x = new Array
Object.defineProperty(x, 1, {
    get: (function(j) {
        if (j) {
            for (v of y) {
                a = arguments
            }
        }
    })
})
Array.prototype.slice(x, 4)

asserts js debug shell on m-c changeset dd03d42b01b1 with --ion-eager at Assertion failure: !info.argsObjAliasesFormals(), at ion/IonAnalysis.cpp

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   129263:d12788533ab7
user:        Kannan Vijayan
date:        Thu Apr 18 16:47:25 2013 -0400
summary:     Bug 860145 - Allow Ion to compile functions which require heavyweight arguments-object construction. r=jandem r=nbp

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Fix.

The assert that arguments are never phis in argsobj-heavy functions is not legitimate.  In particular, when for loops are processed, MBasicBlock::NewPendingLoopHeader will create phis for all stack slots (see MBasicBlock::inherit, around |kind_ == PENDING_LOOP_HEADER|).

I thought about changing the logic here to deal with argsobj-heavy functions, but it doesn't seem like that would be the right way.  We would need to leak the |needsArgsObj| state into this code, which is not very clean.

The assert in |isPhiObservable| is just a bad assert.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 739979 [details] [diff] [review]
Fix.

Review of attachment 739979 [details] [diff] [review]:
-----------------------------------------------------------------

Can you try modifying the condition and keeping this assertion, as we should not expect to have any Phi on the formal arguments when we have an argument object.

::: js/src/ion/IonAnalysis.cpp
@@ +203,5 @@
>      // If the Phi is one of the formal argument, and we are using an argument
>      // object in the function. The phi might be observable after a bailout.
>      // For inlined frames this is not needed, as they are captured in the inlineResumePoint.
>      if (info.fun() && info.hasArguments()) {
>          uint32_t first = info.firstActualArgSlot();

I did not notice that you renamed this function in the previous patch.  Remove Actual, because the term Actual is attached to the number of argument given at call time.  So snapshots / resume points and anything which handle a static state is dealing with formal arguments, not actual arguments.

@@ -206,5 @@
>      if (info.fun() && info.hasArguments()) {
>          uint32_t first = info.firstActualArgSlot();
> -        if (first <= slot && slot - first < info.nargs()) {
> -            // If arguments obj aliases formals, then no arguments slots should ever be phis.
> -            JS_ASSERT(!info.argsObjAliasesFormals());

I think this is true if the previous if condition were:
  info.hasArguments() && !info.needsArgsObj()

because if we have an argument object, then the formals are always aliased and manipulated through the argument object.  On the other hand, if we don't have an argument object, we still accept OSR and setters on formal arguments and all formal arguments are potentially observable.

I think the assertion IsPhiObservable still makes sense in such cases, that is the inherent cost of having an indirect pointer to the formal arguments.

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Can you try modifying the condition and keeping this assertion, as we should
> not expect to have any Phi on the formal arguments when we have an argument
> object.

I don't think this is possible, creating a loopPreHeader _always_ adds a Phi node for all slots which are tracked by IonBuilder, including local and argument slots.  This means that the original "dummy" value for the argument slot (inserted in the start block) will get phi-ed with itself.

Avoiding this would mean adding a special-case check to NOT do these phis for argument slots when doing a loop pre-header block construction on scripts with arguments-objects.  That would mean threading |needsArgsObj| state from IonBuilder into the MBasicBlock constructor, and then adding some logic which is parameterized on that condition, which is not a good idea.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Kannan Vijayan [:djvj] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > Can you try modifying the condition and keeping this assertion, as we should
> > not expect to have any Phi on the formal arguments when we have an argument
> > object.
> 
> I don't think this is possible, creating a loopPreHeader _always_ adds a Phi
> node for all slots which are tracked by IonBuilder, including local and
> argument slots.  This means that the original "dummy" value for the argument
> slot (inserted in the start block) will get phi-ed with itself.

I don't see why we would need a loopPreHeader knowing that we have no OSR when we have an argument object, and that Phi node are fine when we have a lazy argument vector, as there is no setter allowed.

What is done for the lazy argument magic value, is to fold the constants on the Phi, such as we use the one defined in the first basic block.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1150403342b2).

Comment 6

[Kannan Vijayan [:djvj]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Kannan Vijayan [:djvj] from comment #3)
> > (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > > Can you try modifying the condition and keeping this assertion, as we should
> > > not expect to have any Phi on the formal arguments when we have an argument
> > > object.
> > 
> > I don't think this is possible, creating a loopPreHeader _always_ adds a Phi
> > node for all slots which are tracked by IonBuilder, including local and
> > argument slots.  This means that the original "dummy" value for the argument
> > slot (inserted in the start block) will get phi-ed with itself.
> 
> I don't see why we would need a loopPreHeader knowing that we have no OSR
> when we have an argument object, and that Phi node are fine when we have a
> lazy argument vector, as there is no setter allowed.
> 
> What is done for the lazy argument magic value, is to fold the constants on
> the Phi, such as we use the one defined in the first basic block.

Sorry, I meant to say PendingLoopHeader, not loopPreHeader.

Comment 7

[Kannan Vijayan [:djvj]]

Explicitly taking this bug since Gary wasn't sure if I was still on it.  Will get back to this and other argsobj patches as soon as the scriptanalysis stuff is in good shape.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: Fix.

New fix.  Renamed firstActualArgSlot to just firstArgSlot.  Removed assert.  Changed IsPhiObservable to always indicate that argument slots are unobservable for scripts which needsArgsObj.

Running in try:
https://tbpl.mozilla.org/?tree=Try&rev=54ce59fe7e5f

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 743781 [details] [diff] [review]
Fix.

Review of attachment 743781 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for digging in the reason and explaining over IRC that the Phi cannot be eliminated because the Phi seen at the observable is phi(a, phi(a, a)), where the phi(a,a) has not been removed by the isRedundant check as it has not yet been visited.

Having been that way before, it is easier to remove the assertion than bloating the code to make the assertion pass.

Comment 10

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04c51ac7fa21

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/04c51ac7fa21