backtracking allocator miscompile on PJS

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla33
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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 ?? ()
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
(Assignee)

Comment 2

4 years ago
I found the bug. It's a backtacking-specific bug in liveness analysis. I'll post a patch soon.
Assignee: nobody → sunfish
(Assignee)

Comment 3

4 years ago
Created attachment 8446615 [details] [diff] [review]
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 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.
Attachment #8446615 - Flags: feedback+
(Assignee)

Comment 5

4 years ago
Created attachment 8446926 [details] [diff] [review]
dont-special-case-non-fixed.patch

Here's the updated patch.
Attachment #8446615 - Attachment is obsolete: true
Attachment #8446926 - Flags: review?(mrosenberg)
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.
Attachment #8446926 - Flags: review?(mrosenberg) → review+
https://hg.mozilla.org/mozilla-central/rev/ce4166ba83b9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.