621126 - TypeInference: sputnik test failures with type inference

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

I just ran the sputnik tests (in the interpreter) with a tracemonkey build and a TI build, and diffed the results. The TI build fails 12 more tests (not bad considering there are 5246 tests). I think there are only 4 different problems here:

1) causing 2 failures:
---
delete Function.prototype.toString;
assertEq(Function.prototype.toString(), "[object Function]");
---
[infer failure] Missing type at #3:00022 popped 1: 16:Object.toString

2) causing 2 failures:
---
var obj;
obj = new (Function("function f(){this.p1=1;};return f").apply());
assertEq(obj.p1, 1);
obj = new (Function("function f(){this.p1=1;};return f").call());
assertEq(obj.p1, 1);
---
Assertion failure: parent, at ../jsobjinlines.h:893

3) causing 3 failures:
---
with({}) {
    function f() {
        this.foo = "bar";
    }
    o = new f();
    assertEq(o.foo, "bar");
}
---
[infer failure] Missing type at #2:00004 popped 1: 162:161:156:#2:prototype:new

4) Remaining 5 failures: bug 619333

With these fixed, TI passes another large test suite...

Comment 1

[Jan de Mooij [:jandem]]

Oops, there is another inference failure. I didn't notice it because we fail this test without type inference as well.
---
function f() {
    return typeof arguments;
    function arguments() {
        return 7;
    }
}
assertEq(f(), "object");
---
[infer failure] Missing type at #3:00007 popped 0: 13:12:Object:prototype:new

Comment 2

[Brian Hackett [Laid off!]]

Fix for 1), there is a splicePrototype method for handling the prototype bootstrapping that occurs when constructing the Function and Object classes, and it did not handle inheritance of shadowed properties from Object.prototype to Function.prototype right.

http://hg.mozilla.org/projects/jaegermonkey/rev/32d51249b97c

Comment 3

[Brian Hackett [Laid off!]]

Fix for 3), the inference computes .prototype as well as the interpreter on some calls to 'new' (that needs to get fixed), and was using the wrong object.

http://hg.mozilla.org/projects/jaegermonkey/rev/4f133dc20ac7

Comment 4

[Brian Hackett [Laid off!]]

Fix for the issue in comment 1.  We did not add the default value for the 'arguments' variable of a function if that name was redeclared as a local or argument.  (This behavior was added to handle locals/args redefining the name of the function itself; this callee variable evidently behaves differently from arguments).

http://hg.mozilla.org/projects/jaegermonkey/rev/f405f5f83fbe

I do not get a failure in 2) anymore, do the corresponding Sputnik tests still fail?

Comment 5

[Jan de Mooij [:jandem]]

Nice! The interpreter now fails the same tests as the tracemonkey build, without inference failures or assertions.

I expected more failures with -m, but there are only 4, I think 2 different issues. I hope it's okay if I paste them here, but let me know if you want one-bug-per-issue in the future.

1) 2 failures:
--
Function.prototype(undefined)
--
Assertion failure: fun, at ../jsinferinlines.h:751

2) 2 failures:
--
function f(x, y) {
    return y + arguments[0];
};
f(11, 1.2);
==
Assertion failure: fe->data.inMemory(), at ../methodjit/FrameState-inl.h:1067

Comment 6

[Brian Hackett [Laid off!]]

(In reply to comment #5)
> I hope it's okay if I paste them here, but let me know if you want
> one-bug-per-issue in the future.

In the future one-bug-per-issue will be good, but this should be fine now.

Comment 7

[Brendan Eich [:brendan]]

function f() {
    return typeof arguments;
    function arguments() {
        return 7;
    }
}
assertEq(f(), "object");

Is this code from an actual test? It's wrong, but we have regressed somewhere along the line. A js shell from Firefox 3 era:

js> function f() {
   return arguments;
   function arguments() {
       return 7;
   }
}
js> a=f(1,2,3)
function arguments() {
    return 7;
}

And a tm tip shell:

js> function f() {
   return arguments;
   function arguments() {
       return 7;
   }
}
js> a=f(1,2,3)
({0:1, 1:2, 2:3})

This bug is filed as bug 530650 -- I think it should block. But my question here is what testsuite (if any) relies on this bug not being fixed?

/be

Comment 8

[Jan de Mooij [:jandem]]

(In reply to comment #7)
> This bug is filed as bug 530650 -- I think it should block. But my question
> here is what testsuite (if any) relies on this bug not being fixed?
> 

Sputnik has a (different) test, something like this:
---
function f() {
    return typeof arguments;
    function arguments() {
        return 7;
    }
}
assertEq(f(), "function");
---
As you said, FF4 fails this test and this causes one sputnik failure. I (incorrectly) assumed we failed this test due to some ES5 fix (Sputnik has more ES5 issues) and modified the test (comment 1) myself to match current SM behavior. Sorry for the confusion.

Comment 9

[Brian Hackett [Laid off!]]

I can't get either of the tests in comment 5 to crash now.  2) was separately fixed yesterday in rev 228e319574f9 (we would try to fixup doubles types for locals and args that were closed, and then keep them in registers).  1) works in a build from the end of december.