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

VERIFIED FIXED in Firefox 20

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: gkw, Assigned: nbp)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
mozilla20
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 unaffected, firefox18 unaffected, firefox19 unaffected, firefox20 fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

7 years ago
Posted 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

Updated

7 years ago
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee

Comment 1

7 years ago
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)
Assignee

Updated

7 years ago
Duplicate of this bug: 822170
Assignee

Comment 5

7 years ago
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)
Assignee

Comment 9

7 years ago
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.
Assignee

Comment 13

7 years ago
This bug was not compiling before the support of DeclEnv.
https://hg.mozilla.org/mozilla-central/rev/aec01763cb6b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Reporter

Comment 15

7 years ago
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
Reporter

Updated

7 years ago
Depends on: 822938
You need to log in before you can comment on or make changes to this bug.