Closed Bug 1357024 Opened 3 years ago Closed 3 years ago

Crash [@ js::jit::CacheRegisterAllocator::allocateRegister] or Assertion failure: !availableRegs_.empty(), at jit/CacheIRCompiler.cpp:351

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed
firefox55 + fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,bisect])

Crash Data

Attachments

(1 file)

The following testcase crashes on mozilla-central revision ce69b6e1773e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager):

var o5 = Object.prototype;
function f39(o) {
    for (var j = 0; j < 5; j++) {
        o.__proto__ = (o) || (this) || "[1,2,3]" || (this) || (this);
    }
}
f39(o5);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::jit::CacheRegisterAllocator::allocateRegister (this=0xffffbae8, masm=...) at js/src/jit/CacheIRCompiler.cpp:351
#0  js::jit::CacheRegisterAllocator::allocateRegister (this=0xffffbae8, masm=...) at js/src/jit/CacheIRCompiler.cpp:351
#1  0x082631f5 in js::jit::AutoScratchRegister::AutoScratchRegister (reg=..., masm=..., alloc=..., this=0xffffb040) at js/src/jit/CacheIRCompiler.h:448
#2  js::jit::IonCacheIRCompiler::emitCallNativeSetter (this=0xffffb198) at js/src/jit/IonCacheIRCompiler.cpp:1698
#3  0x0826aa49 in js::jit::IonCacheIRCompiler::compile (this=0xffffb198) at js/src/jit/IonCacheIRCompiler.cpp:485
#4  0x0826b9a7 in js::jit::IonIC::attachCacheIRStub (this=0xf521c0c8, cx=0xf791d000, writer=..., kind=<incomplete type>, ionScript=0xf521c000, attached=0xffffbed4, typeCheckInfo=0xffffc028) at js/src/jit/IonCacheIRCompiler.cpp:2067
#5  0x0828f355 in js::jit::IonSetPropertyIC::update (cx=0xf791d000, outerScript=..., ic=0xf521c0c8, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:228
#6  0x50a366d3 in ?? ()
eax	0x8ac6234	145515060
ebx	0xed	237
ecx	0x1	1
edx	0x87c9d24	142384420
esi	0xff	255
edi	0x2	2
ebp	0xffffbae8	4294949608
esp	0xffffafb0	4294946736
eip	0x81e46ed <js::jit::CacheRegisterAllocator::allocateRegister(js::jit::MacroAssembler&)+365>
=> 0x81e46ed <js::jit::CacheRegisterAllocator::allocateRegister(js::jit::MacroAssembler&)+365>:	movl   $0x0,0x0
   0x81e46f7 <js::jit::CacheRegisterAllocator::allocateRegister(js::jit::MacroAssembler&)+375>:	ud2    


Marking s-s until investigated because this seems to be a problem with register allocation which could potentially be s-s.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
When two inputs alias (for example |o.x = o|), we spill one of them to the stack in fixupAliasedInputs. Here we had inputs like:

input1: ValueReg   {reg1, reg2}
input2: PayloadReg {reg1}

We would spill the ValueReg to the stack, but this would leave its type register (reg2) unavailable on 32-bit and we ran out of registers later. The simplest fix is to change it to spill the PayloadReg, so this case can no longer happen.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8859595 - Flags: review?(hv1989)
Not s-s as opt builds will hit MOZ_RELEASE_ASSERT(!availableRegs_.empty()) in allocateRegister.

Regression from bug 1341067.
Blocks: 1341067
Group: javascript-core-security
Tracking 54/55+ for this crash regression.
Comment on attachment 8859595 [details] [diff] [review]
Patch

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

Oh good find! Had to wrap my head around it to understand.

So for posterity:
This function makes sure that different inputs that happen to use the same registers for some inputs have different input locations.
We do this by pushing one or the other to the stack and use that location instead.
BUT we do not gain back the not-aliasing register by doing this.

As a result in the above example:
input1: ValueReg {reg1, reg2} input2: PayloadReg {reg1}

if we put input1 to the stack to solve this aliasing issue we get:
input1: Stack, input2: reg1
=> reg2 unused but not usable, since we didn't do a full scan if input1 was the last use and we can free it again. (We want to avoid having to do that. We want a quick small stupid reg allocator here).

This patch fixes it by pushing input2 instead.
Attachment #8859595 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08899003861
Fix fixupAliasedInputs to not leave an unusable register on 32-bit platforms. r=h4writer
Comment on attachment 8859595 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1341067.
[User impact if declined]: Crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Small patch, fix is pretty simple.
[String changes made/needed]: None.
Attachment #8859595 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/a08899003861
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8859595 [details] [diff] [review]
Patch

Fix a crash. Beta54+. Should be in 54 beta 3.
Attachment #8859595 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I tried to uplift this to Beta but had to back it out for js::jit::IonSetPropertyIC::update crashes across all platforms during test_ShortestPaths_01.html. Can you please take a look, Jan? :)

https://treeherder.mozilla.org/logviewer.html#?job_id=94536462&repo=mozilla-beta
Flags: needinfo?(jdemooij)
Flags: in-testsuite+
Depends on: 1360852
(In reply to Jan de Mooij [:jandem] from comment #6)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Not yet.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Thanks, Ryan.
Flags: needinfo?(jdemooij)
You need to log in before you can comment on or make changes to this bug.