496813 - "Assertion failure: isInt32(*p)" with simple testcase

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

for (var i = 0; i < 4; ++i) {
    var x = 1;
    for (var j = 0; j < 3; ++j) {
        if (j > 1) {
            x = {};
        }
    }
}
print(x);

Incorrectly gives 2885984 when it should give "[object Object]".  If I change the inner loop bound from 3 to 4, I get an assertion instead:

Assertion failure: isInt32(*p), at ../jstracer.cpp:2031

This bug affects both TM tip and 1.9.1 branch.  The testcase is simple enough that I'd expect web sites to hit it.

Comment 1

[Andreas Gal :gal]

This is really not fun. I don't understand why none of our tests catches this stuff.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Reduced testcase:

function f() {
    x = 1;
    for (var j = 0; j < 3; ++j) {
        if (j > 1) {
            x = {};
        }
    }
}

f();
f();
f();

print(x);

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Silly bug. Side exit coalescing code wasn't checking that both exits had the same number of global types.

Comment 4

[Andreas Gal :gal]

Comment on attachment 382165 [details] [diff] [review]
fix

Possible fix but I think it would be better to extend the destination map if it matches but is too short.

Comment 5

[Andreas Gal :gal]

Comment on attachment 382165 [details] [diff] [review]
fix

r+ with the rationale that memcmp without length check is wrong for sure and this is a minimal fix. will discuss further whether this causes duplicate traces which could be avoided by growing an existing trace.

Comment 6

[Brendan Eich [:brendan]]

Are we memcmp'ing off the end of an allocation even with the patch?

/be

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #6)
> Are we memcmp'ing off the end of an allocation even with the patch?
> 
> /be

Since stack slots should always match, it should be okay.

Comment 8

[Brendan Eich [:brendan]]

Would asserting that be vacuous? Not obviously but maybe I am missing context.

/be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2b57dcfdb13d

Comment 10

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b828fd81311a