Assertion failure: !minimalBundle(bundle), at BacktrackingAllocator.cpp:1322

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cbook, Assigned: jandem)

Tracking

(Blocks 1 bug, {assertion})

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50- wontfix, firefox51- fixed, firefox52- fixed)

Details

()

Attachments

(3 attachments)

Posted file bughunter stack
found via bughunter and reproduced on current m-c tip windows debug tinderbox build.

Steps to reproduce:
-> Load http://www.blackdesertfoundry.com/map/#4/-3.82/65.04
--> Assertion failure 
Assertion failure: !minimalBundle(bundle), at c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/js/src/jit/BacktrackingAllocator.cpp:1322

affects debug builds from trunk to beta
[Tracking Requested - why for this release]:

bughunter real world found
Priority: -- → P1
NI bhackett per nbp
Flags: needinfo?(bhackett1024)
This assertion usually indicates a bug in the lowering phase, so a blame cset would be good (bug 1231024 was the last time this came up.)
Flags: needinfo?(bhackett1024)
(In reply to Brian Hackett (:bhackett) from comment #3)
> This assertion usually indicates a bug in the lowering phase, so a blame
> cset would be good (bug 1231024 was the last time this came up.)

I see that the previous time it took us multiple months to fix the issue, would there be a way to get more information on which LIR instruction causes us to fall in such assertion?
Setting IONFLAGS=regalloc and looking at the vreg it is trying to allocate when it fails should help with identifying the problematic LIR instruction.
(In reply to Brian Hackett (:bhackett) from comment #3)
> This assertion usually indicates a bug in the lowering phase, so a blame
> cset would be good

Well somebody has to do this.
Flags: needinfo?(jdemooij)
I have a shell test for this. Will post it after it has been reduced more.
Posted file Shell test
This fails with --no-threads on 32-bit.
The problem seems to be with LApplyArgsGeneric, it wants v44 in edx but somehow that use is minimal and minimalBundle returns true. Not sure but this looks like a regalloc bug.
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
(In reply to Jan de Mooij [:jandem] from comment #9)
> LApplyArgsGeneric

Er, s/Args/Array/
(In reply to Jan de Mooij [:jandem] from comment #9)
> The problem seems to be with LApplyArgsGeneric, it wants v44 in edx but
> somehow that use is minimal and minimalBundle returns true. Not sure but
> this looks like a regalloc bug.

The use 'v44 [242,244)' is minimal because that vreg is not usedAtStart, and is fixed to edx, which is also a fixed output register of the instruction (v85).  So I think changing the useBoxFixed call in LIRGenerator::visitApplyArray would fix this.

Now, despite the apparent conflict between the non-useAtStart useBoxFixed and the fixed output register, the regalloc might be able to allocate this instruction sometimes.  I think this is due to the issue that also just came up in bug 1310710, where the regalloc will sort-of-but-not-really treat non-useAtStart fixed registers on call instructions as if they were useAtStart.  I think this logic may have been mimicking similar logic in lsra back in the day for compatibility with the existing lowering code, but there isn't a coherent invariant being maintained AFAICT and it would be better if we just plain didn't allow non-useAtStart uses on call instructions.  Making this change would require some auditing but it shouldn't be hard to put in place.
Flags: needinfo?(bhackett1024)
Yeah I was wondering about that. If this is an invariant, we definitely need asserts to make this fail reliably. I'll try something.
Posted patch PatchSplinter Review
This forces call instructions to have atStart uses. We already asserted this, but made an exception for fixed registers.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8802047 - Flags: review?(bhackett1024)
Comment on attachment 8802047 [details] [diff] [review]
Patch

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

Thanks!
Attachment #8802047 - Flags: review?(bhackett1024) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc91be30f2aa
Fix Ion regalloc to require call instruction uses to be atStart. r=bhackett
https://hg.mozilla.org/mozilla-central/rev/bc91be30f2aa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking 52-, not necessary to track further since it is fixed.
Comment on attachment 8802047 [details] [diff] [review]
Patch

I think we should take this on Aurora at least. It's probably too late for beta, regalloc patches aren't without risk.

Approval Request Comment
[Feature/regressing bug #]: Old bug.
[User impact if declined]: Potential crashes or assertion failures in debug builds.
[Describe test coverage new/current, TreeHerder]: Landed on m-c a few days ago.
[Risks and why]: I think low risk, but regalloc is tricky so it's hard to say for sure.
[String/UUID change made/needed]: None.
Attachment #8802047 - Flags: approval-mozilla-aurora?
Comment on attachment 8802047 [details] [diff] [review]
Patch

Fix a potential crash. Take it in 51 aurora.
Attachment #8802047 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Un-track for 51 as fixed.
It maybe too late to this fix in 50, we are in RC mode and only taking fixes for release blocking issues.
You need to log in before you can comment on or make changes to this bug.