470409 - TM: Crash [@ js_EqualStrings]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

for (var z = 0; z < 3; ++z) { {switch( '' ) { default: ; case 9: break; case  "" : case <x/>: break; } } }

crashes opt at null at js_EqualStrings and asserts dbg at Assertion failure: str1, at ../jsstr.cpp

Comment 1

[Jeff Walden [:Waldo]]

(gdb) disas $pc-50 $pc
Dump of assembler code from 0x8e2f8a to 0x8e2fbc:
0x008e2f8a:     add    %al,(%eax)
0x008e2f8c:     add    %al,(%eax)
0x008e2f8e:     add    %al,(%eax)
0x008e2f90:     push   %edi
0x008e2f91:     push   %esi
0x008e2f92:     push   %ebx
0x008e2f93:     push   %ebp
0x008e2f94:     mov    %esp,%ebp
0x008e2f96:     sub    $0xc,%esp
0x008e2f99:     mov    %ecx,%ebx
0x008e2f9b:     mov    (%ebx),%edi
0x008e2f9d:     mov    8(%ebx),%esi
0x008e2fa0:     movl   $0x0,(%edi)
0x008e2fa6:     movl   $0x86a000,8(%edi)
0x008e2fad:     mov    $0x86a000,%edx
0x008e2fb2:     mov    $0x0,%ecx
0x008e2fb7:     call   0x11366a <js_EqualStrings>

Looks like somehow we think this is supposed to be called with three (!) arguments, not two, and somehow there's a constant 0 as the first argument to make the obvious crash.  Still investigating...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Don't forget the guard in the type-mismatch case

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 353941 [details] [diff] [review]
Don't forget the guard in the type-mismatch case

>+    LIns* x;
>+    bool cond;
> 
>     uint8 ltag = getPromotedType(l);
>     if (ltag != getPromotedType(r)) {
>-        set(&l, lir->insImm(!equal));
>-        return;
>-    }
>-
>-    LIns* x;
>-    bool cond;
>-    if (ltag == JSVAL_STRING) {
>+        x = lir->insImm(!equal);
>+        cond = !equal;

Nit: transpose these two statements, common !equal into cond.

>+    } else if (ltag == JSVAL_STRING) {
>         LIns* args[] = { r_ins, l_ins };
>         x = lir->ins2i(LIR_eq, lir->insCall(&js_EqualStrings_ci, args), equal);
>         cond = js_EqualStrings(JSVAL_TO_STRING(l), JSVAL_TO_STRING(r));

Rest of code is:

    } else {
        LOpcode op = (ltag != JSVAL_DOUBLE) ? LIR_eq : LIR_feq;
        x = lir->ins2(op, l_ins, r_ins);
        if (!equal)
            x = lir->ins_eq0(x);
        cond = (l == r);
    }
    cond = (cond == equal);

    if (cmpCase) {
        /* Only guard if the same path may not always be taken. */
        if (!x->isconst())
            guard(cond, x, BRANCH_EXIT);
        return;
    }

    set(&l, x);

But x = lir->insImm(!equal) makes x a constant, so the guard won't be emitted. What am I missing?

/be

Comment 4

[Jeff Walden [:Waldo]]

Hmm, point.  I wonder if maybe the logic of determining operands for calls is off by one somehow in rare circumstances.  Implausible, I know, but verbose is verifying that we're setting up a call with the first argument as 0.  Will stare some at the assembly-generation step and see if I can figure out where the problem lies...

Comment 5

[Brendan Eich [:brendan]]

Thinking about this more, the code is better as patched in one regard, because it uses the common tail that covers cmpCase == true.

We must not set(&l, x) in a switch case, of course -- that must be the bug with the unpatched code, eh?

The patched code emits nothing that's actually used, and simply leaves control flow staying on trace, as is done elsewhere (ifop, record_JSOP_IFPRIMTOP), since differently typed operands are not strictly equal (!==) by definition.

While we don't need to emit a constant guard, I'm not sure it's worth optimizing here to avoid the x = lir->insImm(!equal). I would common !equal into cond and use that as the insImm arg, though.

/be

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Better patch

(In reply to comment #5)
> We must not set(&l, x) in a switch case, of course -- that must be the bug with
> the unpatched code, eh?

Ah, yes, that makes sense -- we're just tracing linearly from switch through whatever case finally gets hit, so we just add guards for the previous conditions we might hit differently (plus any evaluation of expressions in the case clauses) and do nothing else.

> While we don't need to emit a constant guard, I'm not sure it's worth
> optimizing here to avoid the x = lir->insImm(!equal). I would common !equal
> into cond and use that as the insImm arg, though.

Makes sense.  We're already doing the x = ... stuff in the non-type-mismatch cases anyway even when it's possible x might be constant then (e.g. boolean comparisons, or string-equality if we were clever enough to do that in insCall).

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 354109 [details] [diff] [review]
Better patch

Looks good. Great to see a bit of the banner comment in the trace-test.js diff ;-).

/be

Comment 8

[Jeff Walden [:Waldo]]

Fixed in TM:

http://hg.mozilla.org/tracemonkey/rev/debe29698839

Comment 10

[Robert Sayre]

merged to mc

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8feb9fdc3faf

Comment 12

[Bob Clary [:bc] (inactive)]

test included in js1_8_1/trace/trace-test.js 
http://hg.mozilla.org/mozilla-central/rev/8f967a7729e2

Comment 13

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2