Closed Bug 819865 Opened 8 years ago Closed 8 years ago

IonMonkey: "Assertion failure: false (Regalloc integrity failure),"

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(2 files, 2 obsolete files)

Attached file stack
[0, 0].sort((function() {
    return function x()(x.r = x)
})())

asserts js debug shell on IonMonkey changeset 725eb8792d27 with --ion-eager at Assertion failure: false (Regalloc integrity failure),

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   115435:93cac86bdd95
user:        Nicolas B. Pierron
date:        Sat Dec 08 18:28:37 2012 -0800
summary:     Bug 807443 - IonMonkey, Compile named lambdas. r=dvander
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
This patch set the MCallee as moveable such as GVN can alias it with another instance of it-self.
Attachment #690667 - Flags: review?(jdemooij)
Why would that cause an LSRA integrity failure?
Comment on attachment 690667 [details] [diff] [review]
Mark MCallee as moveable to benefit from GVN.

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

This looks reasonable, but GVN can be disabled so we can't rely on this.

The integrity check fails because there are multiple LCallee instructions with the same LArgument output. It assumes the second clobbers the first, but that's not true.
Attachment #690667 - Flags: review?(jdemooij)
Duplicate of this bug: 822170
1/ Use GVN to get rid of duplicated MCallee.
2/ Weaken the assertion of the register allocator to ensure that fixed LArgument output are not considered as clobbered for no-op operations.

I haven't added test case from Bug 822170 as it is clearly identical to this one, even if it is also fixed by this bug.
Attachment #690667 - Attachment is obsolete: true
Attachment #692975 - Flags: review?(jdemooij)
Comment on attachment 692975 [details] [diff] [review]
Weaken assertion for fixed LArgument.

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

::: js/src/ion/RegisterAllocator.cpp
@@ +184,5 @@
>                  // Found the original definition, done scanning.
>                  return true;
>              } else {
> +                // LArgument fixed allocation might seem clobbered in cases
> +                // where GVN is disabled.

Drive-by nit: this comment is too cryptic. It would be better to have something explaining why it's okay to have multiple LIR with the same argument-slot output.

(Or, we could just create a canonical MCallee.)
I think that MCallee should be canonical, which should sidestep this issue and would be more in keeping with how MParameter works.  This weakening to the integrity checker can cause it to miss bugs.  Right now I do not believe LSRA ever overwrites the arg: slots with other vregs, but I want to do this for the backtracking allocator as part of bug 822116 (only when the arg's original value is dead).
Comment on attachment 692975 [details] [diff] [review]
Weaken assertion for fixed LArgument.

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

Resetting review per comment 6 and 7 (use a canonical MCallee).
Attachment #692975 - Flags: review?(jdemooij)
The canonical MCallee needs to be added unconditionally when initializing the scope chain in the first block of the function such as it can be push on the stack.  If it is not used DCE should get rid of the MCallee instruction.
Attachment #692975 - Attachment is obsolete: true
Attachment #693311 - Flags: review?(jdemooij)
Comment on attachment 693311 [details] [diff] [review]
Make MCallee canonical.

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

::: js/src/ion/IonBuilder.cpp
@@ +1053,5 @@
>        case JSOP_CALLEE:
>        {
> +        JS_ASSERT(callee_);
> +        current->push(callee_);
> +        return callee_;

"return true;"
Attachment #693311 - Flags: review?(jdemooij) → review+
Comment on attachment 693311 [details] [diff] [review]
Make MCallee canonical.

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

::: js/src/ion/IonBuilder.cpp
@@ +1053,5 @@
>        case JSOP_CALLEE:
>        {
> +        JS_ASSERT(callee_);
> +        current->push(callee_);
> +        return callee_;

Nit I forgot: can remove the { } now.
This bug was not compiling before the support of DeclEnv.
https://hg.mozilla.org/mozilla-central/rev/aec01763cb6b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.