1138693 - Assertion failure: index < length_, at jit/FixedList.h:82 or Crash [@ ??]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision eea6188b9b05 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-debug, run with --no-threads --ion-eager):

var T = TypedObject;
var ST = new T.StructType({x:T.int32});
function check(v) {
    return v.toSource();
}
function test() {
    var fake = { toSource: ST.toSource };
    try { 
        check(fake); 
    } catch (e) {}
}
test();
var uint8 = TypedObject.uint8;
function runTests() {
  uint8.toSource();
}
runTests();
test();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000017b7ea0 in ?? ()
#0  0x00000000017b7ea0 in ?? ()
#1  0x00000000007230a8 in is<js::jit::MPhi> (this=0x17b8bd0) at js/src/jit/MIR.h:747
#2  isPhi (this=0x17b8bd0) at js/src/jit/MIR.h:767
#3  js::jit::EliminatePhis (mir=0x17b38c8, graph=..., observe=js::jit::ConservativeObservability) at js/src/jit/IonAnalysis.cpp:739
#4  0x00000000007aadaf in js::jit::ScalarReplacement (mir=0x17b38c8, graph=...) at js/src/jit/ScalarReplacement.cpp:1073
#5  0x000000000073d3e3 in js::jit::OptimizeMIR (mir=mir@entry=0x17b38c8) at js/src/jit/Ion.cpp:1231
#6  0x000000000073dad9 in js::jit::CompileBackEnd (mir=mir@entry=0x17b38c8) at js/src/jit/Ion.cpp:1580
#7  0x000000000073eade in IonCompile (optimizationLevel=<optimized out>, recompile=false, constructing=<optimized out>, osrPc=<optimized out>, baselineFrame=<optimized out>, script=0x0, cx=0x16b7ce0) at js/src/jit/Ion.cpp:1952
#8  js::jit::Compile (cx=cx@entry=0x16b7ce0, script=script@entry=0x7ffff7e5e1f0, osrFrame=osrFrame@entry=0x0, osrPc=osrPc@entry=0x0, constructing=<optimized out>, forceRecompile=forceRecompile@entry=false) at js/src/jit/Ion.cpp:2106
#9  0x000000000073ee03 in js::jit::CanEnter (cx=0x16b7ce0, state=...) at js/src/jit/Ion.cpp:2245
#10 0x0000000000547f88 in js::RunScript (cx=cx@entry=0x16b7ce0, state=...) at js/src/vm/Interpreter.cpp:424
#11 0x00000000005483aa in js::Invoke (cx=cx@entry=0x16b7ce0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:517
#12 0x0000000000548f9b in js::Invoke (cx=0x16b7ce0, thisv=..., fval=..., argc=0, argv=<optimized out>, rval=...) at js/src/vm/Interpreter.cpp:554
#13 0x00000000006bb5fd in js::jit::DoCallFallback (cx=0x16b7ce0, frame=0x7fffffffd308, stub_=<optimized out>, argc=0, vp=0x7fffffffd2c8, res=JSVAL_VOID) at js/src/jit/BaselineIC.cpp:9578
[...]
#24 0x0000000000000000 in ?? ()
rax	0x17ba818	24881176
rbx	0x17ba4e8	24880360
rcx	0x1	1
rdx	0x17ba598	24880536
rsi	0x17b8bd0	24873936
rdi	0x17b8bd0	24873936
rbp	0x17b8bd0	24873936
rsp	0x7fffffffbca8	140737488338088
r8	0x2	2
r9	0x20	32
r10	0x17ba7d8	24881112
r11	0x17ba4a8	24880296
r12	0x1	1
r13	0x2	2
r14	0x17b56a8	24860328
r15	0x17ba7f8	24881144
rip	0x17b7ea0	24870560
=> 0x17b7ea0:	pushq  $0x17ba8
   0x17b7ea5:	add    %al,(%rax)


Marking this s-s because the assertion indicates some out-of-bounds access.

Comment 1

[Jan de Mooij [:jandem]]

JSBugMon is down so NI from nbp because ScalarReplacement is on the stack.

Comment 2

[Nicolas B. Pierron [:nbp]]

I am unable to reproduce this issue with either a debug/optimized build while compiling with clang/gcc4.8.
Running multiple times does not show intermittent behaviour.

Comment 3

[Daniel Veditz [:dveditz]]

decoder: what compiler did you use, same as nbp?

Comment 4

[Nicolas B. Pierron [:nbp]]

clang version 3.3 (tags/RELEASE_33/final)
gcc (GCC) 4.8.2

Decoder mention that he was no longer able to reproduce it on tip, so I will try to see if I can reproduce it on top of the revision mentionned in comment 0.

Comment 5

[Nicolas B. Pierron [:nbp]]

I can reproduce this issue on top of revision eea6188b9b05.
I will investigate.

Comment 6

[Nicolas B. Pierron [:nbp]]

So far the problem seems to be that DescrToSource does

  UnsafeGetStringFromReservedSlot(this, 1);

which is inlined as a MLoadFixedSlot, where |this| is a MNewObject which has the following template object:

object 0x7ffff7e6d1f0
class 0x150a890 Object
flags:
proto <Object at 0x7ffff7e5b020>
parent <global object at 0x7ffff7e5a060>
properties:
    ((js::Shape *) 0x7ffff7e7de20) enumerate JSString* (0x7ffff7e1c970) = Latin1Char * (0x7ffff7e1c978) = "toSource"
: slot 0 = undefined

Apparently, MCallOptimize does not guard on the object type before doing the MLoadFixedSlot(slot = 1).

Comment 7

[Nicolas B. Pierron [:nbp]]

Ok, the problem is simple, Scalar Replacement is eager to replace the MLoadFixedSlot from the MNewObject, even if the path which is being optimized can never happen.  This problem is likely to exists since the introduction of Scalar Replacement.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Check if Loads can be optimized by Scalar Replcement.

This patch add inevitable bailouts on the paths where we saw Load/Store
instructions which are making references to slots which are not defined as
part of the shape of the NewObject.

This is not a big deal, as these branches are likely to be removed by UCE
(within GVN).

The problem seen here is that the self-hosted code has guards to check that
the object is correct, before doing any operations.  These guards are
replaced by constants when the function calls are inlined.  Unfortunately,
Scalar replacement has to run before the Apply Types phase, which runs
before UCE (within GVN), and thus see both branches, including the dead-one.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Add comments and test.

Comment 10

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 993eb76a8bd6).
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/2a0481539f38
user:        Nicolas B. Pierron
date:        Thu Feb 26 16:18:41 2015 +0100
summary:     Bug 1129313 - Scalar Replacement: Remove PostWriteBarrier at the same time as the stores. r=h4writer

This iteration took 145.106 seconds to run.

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Christian Holler (:decoder) from comment #10)
> The first bad revision is:
> summary:     Bug 1129313 - Scalar Replacement: Remove PostWriteBarrier at
> the same time as the stores. r=h4writer

I think the problem existed before this, as this patch only re-enabled scalar replacement after it got accidently disabled by the addition of new MIR nodes.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8572730 [details] [diff] [review]
Check if Loads can be optimized by Scalar Replcement.

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

Looks good. Some comments below that we can maybe fix in a separate patch.

::: js/src/jit/MIR.h
@@ +3180,5 @@
>          replaceOperand(slot + 1, def);
>      }
>  
> +    bool hasFixedSlot(uint32_t slot) const {
> +        return slot < numSlots() && slot < numFixedSlots();

I think it'd be nice to have the invariant numFixedSlots() <= numSlots(), to make it easier to reason about the code and so that we only need to check numFixedSlots().

@@ +3192,5 @@
>          setSlot(slot, def);
>      }
>  
> +    bool hasDynamicSlot(uint32_t slot) const {
> +        return numFixedSlots() < numSlots() && slot < numSlots() - numFixedSlots();

Then we could get rid of the && LHS here too.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8572731 [details] [diff] [review]
Add comments and test.

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

::: js/src/jit/ScalarReplacement.cpp
@@ +437,5 @@
>          state_ = BlockState::Copy(alloc_, state_);
>          state_->setFixedSlot(ins->slot(), ins->value());
>          ins->block()->insertBefore(ins->toInstruction(), state_);
>      } else {
> +        // UnsafeSetReserveSlot can inject baked-in slots which are guarded by

Nit: maybe s/inject/access/ ?

Comment 14

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0942dd86e375

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0942dd86e375

Comment 16

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 17

[Pulsebot]

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

Comment 18

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

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72341fb355fa

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/72341fb355fa