906858 - Assertion failure: v->toGCThing(), at gc/Marking.cpp with --ion-eager --ion-gvn=pessimistic --ion-inlining=off --ion-regalloc=backtracking

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

The upcoming testcase asserts js debug shell (32-bit threadsafe deterministic) on m-c changeset a8daa428ccbc with --ion-eager --ion-gvn=pessimistic --ion-inlining=off --ion-regalloc=backtracking at Assertion failure: v->toGCThing(), at gc/Marking.cpp

Setting needinfo from Brian because this involves --ion-regalloc=backtracking.

Comment 2

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

Kannan, I don't think this is correct, is it?

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/133bde40e6bc
user:        Kannan Vijayan
date:        Wed Jul 31 17:36:09 2013 -0400
summary:     Bug 893038 - Re-enable heavyweight function and cloned lambda inlining. r=nbp

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Kannan, I don't think this is correct, is it?

I don't think so either.  Especially given that it's with --ion-inlining=off.  That patch affects only inlining behaviour.

Comment 4

[Dan Gohman [:sunfish]]

(In reply to Gary Kwong [:gkw] [:nth10sd] (PTO Sep 24 - 27) from comment #0)
> The upcoming testcase asserts js debug shell (32-bit threadsafe
> deterministic) on m-c changeset a8daa428ccbc with --ion-eager
> --ion-gvn=pessimistic --ion-inlining=off --ion-regalloc=backtracking at
> Assertion failure: v->toGCThing(), at gc/Marking.cpp

Could you attach the testcase for this bug? Thanks!

Comment 5

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

I have sent the testcase to :sunfish.

Comment 6

[Dan Gohman [:sunfish]]

This doesn't fail for me on top of mozilla-inbound. Automated binary search found 75380019dc49 as the patch where the crash stops happening. However, I don't see anything in that patch which looks relevant.

Comment 7

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

(In reply to Dan Gohman [:sunfish] from comment #6)
> This doesn't fail for me on top of mozilla-inbound. Automated binary search
> found 75380019dc49 as the patch where the crash stops happening. However, I
> don't see anything in that patch which looks relevant.

I tried it in a recent m-c (not really latest, but still quite recent) rev:

./js-dbg-32-dm-ts-darwin-211337f7fb83 --ion-eager --ion-gvn=pessimistic --ion-inlining=off --ion-regalloc=backtracking 906858-c1.js

Assertion failure: v->toGCThing(), at gc/Marking.cpp

Configuration options:

LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 8

[Dan Gohman [:sunfish]]

I can reproduce the problem reliably now. The testcase is rather slippery in multiple respects. But I now have a reduced testcase which is about 40 lines long and evokes the crash reliably.

The patch from bug 931487 makes the crash go away. Unlike bug 931487, this doesn't look related to OSR entries. But like bug 931487, it does look like there's a stack argument slot getting overwritten in an unexpected way.

Comment 9

[Dan Gohman [:sunfish]]

Argument slots arg:8 and arg:12 make up a box (this is 32-bit x86 so it's two parts), which which appears to be registered as a GC root. The slots are loaded into separate virtual registers, and assigned as the canonical spill slots for separate virtual register groups. The register allocator is then spilling a register from one of these groups. At runtime, the GC is activated and it scans the roots, which includes the arg:8 and arg:12 pair, but after the spill they are holding halves of unrelated values.

The the tag half happens to be holding the String tag, and the payload half happens to be zero, and their combination is triggering assertion failures.

It doesn't seem safe to let the register allocator spill values into argument slots which are registered as GC roots in general. Is there a way for the register allocator to know which argument slots are GC roots?

Comment 10

[Dan Gohman [:sunfish]]

Actually, it's more than that. The argument slot in question is a GC root in a safepoint on a call instruction, and the callee of that call is clobbering the slot by spilling into it.

It appears that the optimization of using argument slots for spill space was broken by the fix for bug 771398, which I unfortunately don't currently have access to view. That patch adds argument slots to safepoints, which effectively requires them to hold valid values whenever the GC runs, which means that the register allocator can't do things like spill half of a value into them on nunbox32 platforms. punbox64 platforms might be ok because they wouldn't ever spill half of a value, but there could be other problems, especially depending on the nature of bug 771398.

Reverting the fix for bug 771398 fixes this bug and doesn't cause any obvious regressions, though without the testcase from that bug it's difficult to fully investigate.

Comment 11

[Dan Gohman [:sunfish]]

Attachment: regalloc-dont-spill-to-args.patch

Disabling the optimization to use argument slots for spill space is another way to fix this bug, and it also happens to fix bug 932465 and bug 931487.

If this patch is ok, I'll file another bug for working out all the tricky issues discussed here and in related bugs, and re-enabling this optimization.

Comment 12

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c610c3ac1ba9

Comment 13

[Dan Gohman [:sunfish]]

I filed bug 934502 for re-enabling this feature.

Comment 14

[Dan Gohman [:sunfish]]

Oops, I put bug 931487 on the commit instead of this bug, though that bug is also fixed by the same patch :).

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

Comment 15

[Ioana (away)]

Could you please send me the testcase to verify this bug (use the email bugzilla shows you for me)?

Comment 16

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

I verified this.

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/8408cc15ce95
user:        Hannes Verschore
date:        Sun Nov 03 22:22:11 2013 +0100
summary:     Bug 914255 - Reduce the number of objects tracked in a TypeSet, r=bhackett

(seems to be a slightly different changeset - this bug in any case got fixed in the early November 2013 timeframe)