1030378 - backtracking allocator miscompile on PJS

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Dan Gohman [:sunfish]]

As of 5322e6721141 (bug 1019304), tests/parallel/Array-reducePar-bail.js and several other parallel tests fail when run with --ion-regalloc=backtracking.

The test fails with this assertion:

Assertion failure: (ptrBits & 0x7) == 0, at ../../../dist/include/js/Value.h:756

and this stack trace:

(gdb) where
#0  0x00000000004214ee in js::types::TypeObject::clasp (this=0x0)
    at /Projects/celery/src/js/src/shell/../jsinfer.h:938
#1  0x0000000000422264 in js::ObjectImpl::getClass (this=0x7ffff7dd72e0 <__pthread_keys>)
    at /Projects/celery/src/js/src/shell/../vm/ObjectImpl.h:381
#2  0x0000000000427a90 in JSObject::is<JSFunction> (this=0x7ffff7dd72e0 <__pthread_keys>)
    at /Projects/celery/src/js/src/shell/../jsobj.h:1108
#3  0x00000000004275d5 in JSObject::as<JSFunction> (this=0x7ffff7dd72e0 <__pthread_keys>)
    at /Projects/celery/src/js/src/shell/../jsobj.h:1112
#4  0x000000000070787d in js::jit::InlineFrameIterator::findNextFrame (this=0x7ffff51fb000)
    at /Projects/celery/src/js/src/jit/IonFrames.cpp:1864
#5  0x000000000070716f in js::jit::InlineFrameIterator::InlineFrameIterator (
    this=0x7ffff51fb000, cx=0x7ffff51fb9c0, iter=0x7ffff51fb3f0)
    at /Projects/celery/src/js/src/jit/IonFrames.cpp:1765
#6  0x0000000000a423ca in RematerializeFramesWithIter<js::jit::IonBailoutIterator> (
    cx=0x7ffff51fb9c0, frameIter=..., frames=...)
    at /Projects/celery/src/js/src/vm/ForkJoin.cpp:1949
#7  0x0000000000a41926 in js::ParallelBailoutRecord::rematerializeFrames (this=0x7fffffff9aa0, 
    cx=0x7ffff51fb9c0, frameIter=...) at /Projects/celery/src/js/src/vm/ForkJoin.cpp:1980
#8  0x00000000007a1886 in js::jit::BailoutPar (sp=0x7ffff51fb588, 
    entryFramePointer=0x7ffff51fb580)
    at /Projects/celery/src/js/src/jit/ParallelFunctions.cpp:532
#9  0x00007ffff67c0404 in ?? ()

Comment 1

[Douglas Crosher [:dougc]]

Fwiw just started seeing a jit-test failure using the ARM simulator for the test parallel/Array-reducePar-bail.js when the backtracking register allocator is used. Probably some bad code causing some corruption.
Assertion failure: uintptr_t(obj) > 0x1000 || uintptr_t(obj) == 0x42, at ../../dist/include/js/Value.h:557

Comment 2

[Dan Gohman [:sunfish]]

I found the bug. It's a backtacking-specific bug in liveness analysis. I'll post a patch soon.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: dont-special-case-non-fixed.patch

This patch fixes the bug, and adds an assert which catches the bug in a more obvious way. I also left in a things I used during debugging which seem to be generally useful.

Fixed-register liveness around calls is special-cased in numerous places. This is confusing, but lots of things currently depend on it. The bug here is that in the backtracking allocator version of liveness, it was applying this special case to virtual registers also. This caused the backtracking allocator to think that it was safe to assign a call-clobbered (aka volatile) register to one of the snapshot uses on a GuardThreadExclusive. Since GuardThreadExclusive is a call, the register was clobbered while still live. Moving the snapshot use position from the INPUT to the OUTPUT position of the call makes it properly conflict with the call's fixed clobbers, so the register allocator knows not to assign it any call-clobbered registers.

Comment 4

[Douglas Crosher [:dougc]]

Comment on attachment 8446615 [details] [diff] [review]
dont-special-case-non-fixed.patch

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

Fixes the jit-test failure seen using the ARM simulator, thanks.

Comment 5

[Dan Gohman [:sunfish]]

Attachment: dont-special-case-non-fixed.patch

Here's the updated patch.

Comment 6

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8446926 [details] [diff] [review]
dont-special-case-non-fixed.patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +3339,5 @@
>          // Don't emit any code for trivial blocks, containing just a goto. Such
>          // blocks are created to split critical edges, and if we didn't end up
>          // putting any instructions in them, we can skip them.
>          if (current->isTrivial())
>              continue; 

Might as well remove the extra whitespace while you're touching this area.

Comment 7

[Dan Gohman [:sunfish]]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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