824347 - Assertion failure: !phi->block()->callerResumePoint(), at ion/IonAnalysis.cpp:208

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 7b74f4ee76c9 (run with --ion-eager):


function g() { assertEq(false, true) }
function f(b) {
    if (b) {
        f(false);
        var queue = new i(null, isnan, x);
    }
    g.apply(null, arguments);
}
f(true);

Comment 1

[Nicolas B. Pierron [:nbp]]

I guess this might be related to the inlining of fun_apply added in Bug 813784 when the function is compiled the with ion-eager.

The problem is likely located in the inlined version of “g” in the inlined version of “f” in “f”.  The problem should be located in IonBuilder, near jsop_funapplyarguments with an inlined depth of 1 around the makeCall done at the end of the function.

By the way, do we really want to inline a function in it-self ?!  This sounds like something which should be investigated as part of another analysis instead of consuming more time in IonBuilder.

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116797:662f338798a9
user:        Hannes Verschore
date:        Fri Dec 21 18:53:19 2012 +0100
summary:     Bug 813784: Allow inlining of |arguments| in IM, r=nbp

This iteration took 94.910 seconds to run.

Comment 3

[Hannes Verschore [:h4writer]]

The assert was added in "Bug 810253: Correctly dump arguments by nbp". If somebody could add me to that bug, it would be helpful. As far as I understand just removing the assert would be just fine.

static inline bool
IsPhiObservable(MPhi *phi, Observability observe)
{
    ...

    // 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.
    CompileInfo &info = phi->block()->info();
    if (info.fun() && info.hasArguments()) {
-       // We do not support arguments object inside inline frames yet.
-       JS_ASSERT(!phi->block()->callerResumePoint());
        uint32_t first = info.firstArgSlot();
        if (first <= slot && slot - first < info.nargs())
            return true;
    }
}

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: Remove assert

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 697104 [details] [diff] [review]
Remove assert

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

::: js/src/ion/IonAnalysis.cpp
@@ +206,2 @@
>      CompileInfo &info = phi->block()->info();
> +    if (info.fun() && info.hasArguments() && phi->block()->callerResumePoint()) {

Remove "&& phi->block()->callerResumePoint()" from the condition, and r=me.

By the way, this code should have triggered the test case added with Bug 810253.  If possible, can you adapt the test case such as it raise an error if it doesn't anymore.

Comment 6

[Hannes Verschore [:h4writer]]

Nicolas, you were right, I made a mistake there in my patch. Test should indeed trigger on that change. Adjusted it and added a modified testcase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99380aaa1487

Comment 7

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/99380aaa1487