938130 - Assertion failure: returnAddr < method_->raw() + method_->instructionsSize(), at jit/BaselineJIT.cpp:573 or Crash [@ GetBytecodeLength] with parallel compilation

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 7b014f0f3b03 (threadsafe build, run with --fuzzing-safe --ion-parallel-compile=on --ion-eager):


function f() {    }
function g() {    }
var x = [f,f,f,undefined,g];
for (var i = 0; i < 5; ++i)
  y = x[i]("x");

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
js::jit::BaselineScript::pcForReturnOffset (this=<optimized out>, script=<optimized out>, nativeOffset=135957195) at  js/src/jit/BaselineJIT.cpp:729
729             curPC += GetBytecodeLength(curPC);
(gdb) bt 8
#0  js::jit::BaselineScript::pcForReturnOffset (this=<optimized out>, script=<optimized out>, nativeOffset=135957195) at  js/src/jit/BaselineJIT.cpp:729
#1  0x000000000070b121 in baselineScriptAndPc (pcRes=<synthetic pointer>, scriptRes=0x7fffffffd420, this=0x7fffffffd3a0) at  js/src/jit/IonFrames.cpp:204
#2  js::jit::GetPcScript (cx=<optimized out>, scriptRes=0x7fffffffd420, pcRes=0x7fffffffd4c0) at  js/src/jit/IonFrames.cpp:1169
#3  0x00000000004f0680 in currentScript (allowCrossCompartment=JSContext::DONT_ALLOW_CROSS_COMPARTMENT, ppc=<optimized out>, this=0x1450940) at ../jscntxtinlines.h:529
#4  JSContext::currentScript (this=0x1450940, ppc=<optimized out>, allowCrossCompartment=JSContext::DONT_ALLOW_CROSS_COMPARTMENT) at ../jscntxtinlines.h:511
#5  0x00000000004e68ab in GetPropertyHelperInline<(js::AllowGC)1> (vp=JSVAL_VOID, id=$jsid("__noSuchMethod__"), receiver=(JSObject * const) 0x7ffff5d4efa0 [object Array], obj=
    (JSObject * const) 0x7ffff5d4efa0 [object Array], cx=0x1450940) at  js/src/jsobj.cpp:4228
#6  js::baseops::GetProperty (cx=0x1450940, obj=(JSObject * const) 0x7ffff5d4efa0 [object Array], receiver=(JSObject * const) 0x7ffff5d4efa0 [object Array], id=$jsid("__noSuchMethod__"), vp=JSVAL_VOID)
    at  js/src/jsobj.cpp:4303
#7  0x0000000000585af5 in getGeneric (vp=JSVAL_VOID, id=$jsid("__noSuchMethod__"), receiver=..., obj=..., cx=0x1450940) at ../jsobj.h:994
(More stack frames follow...)
(gdb) info reg
rbp            0x101522aac      0x101522aac
(gdb) x /i $pc
=> 0x6745e5 <js::jit::BaselineScript::pcForReturnOffset(JSScript*, unsigned int)+117>:  movzbl 0x0(%rbp),%edx


Marking s-s and sec-critical based on the crash trace and assertion.

Djvj: Is the __noSuchMethod__ stuff in the trace in any way related to your recent changes?

Comment 3

[Kannan Vijayan [:djvj]]

(In reply to Christian Holler (:decoder) from comment #2)

> Djvj: Is the __noSuchMethod__ stuff in the trace in any way related to your
> recent changes?

Checking.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/58605e9a6ea1
user:        Kannan Vijayan
date:        Tue Nov 12 14:20:34 2013 -0500
summary:     Bug 912303 - Added noSuchMethod support to baseline CALLPROP/CALLELEM stubs. r=efaust

autoBisect thinks so, too?

Comment 5

[Kannan Vijayan [:djvj]]

Yeah, the issue is here:

#if JS_HAS_NO_SUCH_METHOD
    entersStubFrame_ = true;
    if (isCallElem_) {
        Label afterNoSuchMethod;
        Label skipNoSuchMethod;
        regs = availableGeneralRegs(0);
        regs.takeUnchecked(obj);
        regs.take(R1);

        masm.pushValue(R1);
        masm.loadValue(element, R1);
        masm.branchTestUndefined(Assembler::NotEqual, R1, &skipNoSuchMethod);

        // Call __noSuchMethod__ checker.  Object pointer is in objReg.
        scratchReg = regs.takeAnyExcluding(BaselineTailCallReg);
        enterStubFrame(masm, scratchReg);


Can't leave the R1 pushed naked before entering the stub frame, because entering the stub frame pops the returnAddr, which has been displaced by R1.  So enterStubFrame thinks the value that was in R1 is the returnAddr.

Pushing R1 before the enterStubFrame makes no sense.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: nosuchmethod-bug-938130.patch

Fix for issue.  Mostly involves register shuffling for the sake of x86 to avoid the push.

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8335330 [details] [diff] [review]
nosuchmethod-bug-938130.patch

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

Blegh. Sorry this was missed in review the first time. I totally forgot about the TailCallRegister pop.

Good. r=me.

::: js/src/jit/BaselineIC.cpp
@@ +4453,5 @@
> +        ValueOperand val = regs.takeValueOperand();
> +
> +        masm.loadValue(element, val);
> +        masm.branchTestUndefined(Assembler::NotEqual, val, &skipNoSuchMethod);
> +        regs.add(val);

blech, register pressure :P

::: js/src/jit/RegisterSets.h
@@ +415,5 @@
>          // registers are sometimes more efficient (e.g. optimized encodings for
>          // EAX on x86).
>          return getFirst();
>      }
> +    T getAnyExcluding(T preclude) {

This is a nice simplification.

Comment 8

[Kannan Vijayan [:djvj]]

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

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi decoder - did this miss our auto verification? Wanted to be sure it was fixed.

Comment 11

[Christian Holler (:decoder)]

This is from a threadsafe build and JSBugMon cannot verify those yet. Verified fixed manually.

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks decoder!