Closed Bug 824347 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: decoder, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

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);
Whiteboard: [jsbugmon:update,bisect]
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.
Depends on: 813784
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
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;
    }
}
Attached patch Remove assertSplinter Review
Assignee: general → hv1989
Attachment #697104 - Flags: review?(nicolas.b.pierron)
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.
Attachment #697104 - Flags: review?(nicolas.b.pierron) → review+
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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/99380aaa1487
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.