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

NI bhackett per nbp

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

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

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

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.

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

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

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

Comment on attachment 8802047 [details] [diff] [review] Patch Review of attachment 8802047 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

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

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.

Comment on attachment 8802047 [details] [diff] [review] Patch Fix a potential crash. Take it in 51 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.