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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Carsten Book [:Tomcat]]

Attachment: 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

Comment 1

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

bughunter real world found

Comment 2

[Julien Cristau [:jcristau]]

NI bhackett per nbp

Comment 3

[Brian Hackett [Laid off!]]

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.)

Comment 4

[Nicolas B. Pierron [:nbp]]

(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?

Comment 5

[Brian Hackett [Laid off!]]

Setting IONFLAGS=regalloc and looking at the vreg it is trying to allocate when it fails should help with identifying the problematic LIR instruction.

Comment 6

[Jan de Mooij [:jandem]]

(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.

Comment 7

[Jan de Mooij [:jandem]]

I have a shell test for this. Will post it after it has been reduced more.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Shell test

This fails with --no-threads on 32-bit.

Comment 9

[Jan de Mooij [:jandem]]

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.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> LApplyArgsGeneric

Er, s/Args/Array/

Comment 11

[Brian Hackett [Laid off!]]

(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.

Comment 12

[Jan de Mooij [:jandem]]

Yeah I was wondering about that. If this is an invariant, we definitely need asserts to make this fail reliably. I'll try something.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Patch

This forces call instructions to have atStart uses. We already asserted this, but made an exception for fixed registers.

Comment 14

[Brian Hackett [Laid off!]]

Comment on attachment 8802047 [details] [diff] [review]
Patch

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

Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc91be30f2aa
Fix Ion regalloc to require call instruction uses to be atStart. r=bhackett

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bc91be30f2aa

Comment 18

[Marcia Knous [:marcia]]

Tracking 52-, not necessary to track further since it is fixed.

Comment 19

[Jan de Mooij [:jandem]]

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.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8802047 [details] [diff] [review]
Patch

Fix a potential crash. Take it in 51 aurora.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/52773070231c

Comment 22

[Gerry Chang [:gchang]]

Un-track for 51 as fixed.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

It maybe too late to this fix in 50, we are in RC mode and only taking fixes for release blocking issues.