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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 2

[Jan de Mooij [:jandem]]

Not s-s as opt builds will hit MOZ_RELEASE_ASSERT(!availableRegs_.empty()) in allocateRegister.

Regression from bug 1341067.

Comment 3

[Marcia Knous [:marcia]]

Tracking 54/55+ for this crash regression.

Comment 4

[Hannes Verschore [:h4writer]]

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a08899003861
Fix fixupAliasedInputs to not leave an unusable register on 32-bit platforms. r=h4writer

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

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

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

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8859595 [details] [diff] [review]
Patch

Fix a crash. Beta54+. Should be in 54 beta 3.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dbdaa649ec55

Comment 12

[Andrei Vaida [:avaida]]

(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.

Comment 13

[Jan de Mooij [:jandem]]

Thanks, Ryan.