Closed
Bug 819865
Opened 12 years ago
Closed 12 years ago
IonMonkey: "Assertion failure: false (Regalloc integrity failure),"
Categories
(Core :: JavaScript Engine, defect)
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)
7.27 KB,
text/plain
|
Details | |
5.02 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
[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•12 years ago
|
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 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 3•12 years ago
|
||
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 | ||
Comment 5•12 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.)
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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•12 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 10•12 years ago
|
||
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 11•12 years ago
|
||
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 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec01763cb6b
Assignee | ||
Comment 13•12 years ago
|
||
This bug was not compiling before the support of DeclEnv.
status-b2g18:
--- → unaffected
status-firefox-esr10:
--- → unaffected
status-firefox17:
--- → unaffected
status-firefox18:
--- → unaffected
status-firefox19:
--- → unaffected
status-firefox20:
--- → fixed
status-firefox-esr17:
--- → unaffected
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aec01763cb6b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Reporter | ||
Comment 15•12 years ago
|
||
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.
Description
•