879647 - Assertion failure: label->used(), at jit/arm/CodeGenerator-arm.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

(function () {
    (! (o == "" && typeof b == ""))
    s = [, 0 / 0, 1 / 0, 1 / 0, , , 80001, -0, -0, , 0xffffffff, 0x100000000, 0x100000001]
    return {
        c: s,
        r: o
    }
})()

asserts js debug shell on m-c changeset 7e3a4ebcf067 with --ion-eager at Assertion failure: label->used() && !label->bound(), at ion/arm/CodeGenerator-arm.cpp

This seems different from bug 829534, the regression changeset shown below is a lot later than that one.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/63b006573c65
user:        Brian Hackett
date:        Wed May 22 11:36:29 2013 -0600
summary:     Bug 870052 - Various tweaks to reduce recompilation on asm.js style apps, r=jandem.

Comment 1

[Christian Holler (:decoder)]

Yep, this looks like what I'm seeing too. I'll come up with a signature.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Error: Unsupported architecture "ARM" required by bug

Comment 4

[Brian Hackett [Laid off!]]

It's hard to tell what's going on here, but bug 870052 doesn't have anything to do with it and if anything exposed an existing bug in the assembler.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting needinfo from Marty based on comment 4.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

... and setting needinfo from myself to retest since bug 879701 has landed, likely modifying this assert.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This assert turned into:

Assertion failure: label->used(), at jit/arm/CodeGenerator-arm.cpp

Tested on 32-bit debug Ubuntu Linux m-c rev d8a62355ea26 on ARM.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Updated stack

Comment 9

[Marty Rosenberg [:mjrosenb]]

So looking into this, the issue appears to be:
(gdb) p lir->mir()->resultTypeSet()->empty()
$1 = true

which leads to us never generating a branch to the fail path, so the label throws an assertion due to not having anything branching to it.  I'm looking into why the result typeset is empty.

Comment 10

[Marty Rosenberg [:mjrosenb]]

Attachment: fixAssertion-r0.patch

previous assessment is wrong.  It turns out this landed in a tricky situation that the assembler buffer can't handle.  (there is code for handling it, but it is subtly wrong, so I decided to just abort ION in this case.)  anyhow, nobody told the bind code that compilation has been aborted, so it throws an assertion.
This patch just has us bail out sooner, so we don't get to the assertion case.

Comment 11

[Jacob Bramley [:jbramley]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #10)
> Created attachment 815959 [details] [diff] [review]
> fixAssertion-r0.patch
> 
> (there is code for handling it, but it is subtly wrong, so I decided to just
> abort ION in this case.)

Can it be fixed? Is there a TODO or bug in place?

Comment 12

[Jacob Bramley [:jbramley]]

Comment on attachment 815959 [details] [diff] [review]
fixAssertion-r0.patch

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

If the assembler buffer gets fixed, this bailout presumably isn't needed, but it will probably get forgotten. Could you add a TODO explaining that it should be removed at some point?

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Is this ready for landing soon?

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'd like this to be landed before the upcoming merge window, please. This assertion is also one of the asserts blocking Android 2.2 debug tests, see bug 866795 comment 15.

Comment 15

[Marty Rosenberg [:mjrosenb]]

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/cacd16757698

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Is there any chance this can be nominated for backport? (this bug appears pretty frequently on ARM fuzzing prior to the fix)

Comment 18

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 815959 [details] [diff] [review]
fixAssertion-r0.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IM landing
User impact if declined: assertions in debug builds, annoyance for fuzzers
Testing completed (on m-c, etc.): it has been on m-c for a while
Risk to taking this patch (and alternatives if risky): corner cases and OOM are handled slightly differently now.
String or IDL/UUID changes made by this patch:

Comment 19

[bhavana bajaj [:bajaj]]

Comment on attachment 815959 [details] [diff] [review]
fixAssertion-r0.patch

[

Comment 20

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #19)
> Comment on attachment 815959 [details] [diff] [review]
> fixAssertion-r0.patch
> 
> [

whoops hit save too fast. Switched the nom from aurora to beta given Firefox27 is already fixed here.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/46ff69fc509e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/dcc8c99781dc

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #22)
> https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/dcc8c99781dc

This is likely wrong, it should be:

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/46ff69fc509e