655499 - [infer failure] Missing type at #2:00021 pushed 0: string

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on TI revision e09e209d988e (run with -n -a),
tested on 64 bit:

test();
function test() {
    try {
        var result, arguments = 'adddb', arguments;
    } catch (ex) {}
}

Comment 1

[Paul Biggar]

Slightly simplified:

(function () {
   var arguments = '';
   var arguments;
})();

Comment 2

[Paul Biggar]

Am a little confused by this:

 $ JMFLAGS=jsops ../../../tm-7/js/src/objdir.DBG/js -m -a a.js
[jaeger] JSOps         0 00000:   1  lambda (function () {var arguments = "";var arguments;})
[jaeger] JSOps         1 00003:   1  nullblockchain
[jaeger] JSOps         1 00004:   1  push
[jaeger] JSOps         2 00005:   1  call 0
[jaeger] JSOps         1 00008:   1  pop
[jaeger] JSOps         0 00009:   4  stop
[jaeger] JSOps         0 00000:   2  defvar "arguments"
[jaeger] JSOps         0 00003:   3  defvar "arguments"
[jaeger] JSOps         0 00006:   2  bindname "arguments"
[jaeger] JSOps         1 00009:   2  string ""
[jaeger] JSOps         2 00012:   2  setname "arguments"
[jaeger] JSOps         1 00015:   2  pop
[jaeger] JSOps         0 00016:   3  getgname "arguments"
[jaeger] JSOps         1 00019:   3  pop
[jaeger] JSOps         0 00020:   3  stop

Opcode 16 has getgname. That's not right is it?

Comment 3

[Brian Hackett [Laid off!]]

Yeah, that's not right.  Bugs where GNAME opcodes are emitted incorrectly crash JM due to its stronger assertions and more delicate assumptions (just cause incorrect behavior on TM, but even that is hard to tickle as GNAME is treated like NAME by the interpreter).  Bug 647695 also has some broken GNAME testcases.  Would be tremendously happy if these got fixed!

Comment 4

[Paul Biggar]

Am I right in thinking that these assertions should hold?

diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
--- a/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -4665,16 +4665,18 @@ END_CASE(JSOP_SETCALL)
     JS_END_MACRO                                                              \
 
 BEGIN_CASE(JSOP_GETGNAME)
 BEGIN_CASE(JSOP_CALLGNAME)
 BEGIN_CASE(JSOP_NAME)
 BEGIN_CASE(JSOP_CALLNAME)
 {
     JSObject *obj = &regs.fp()->scopeChain();
+    JS_ASSERT_IF(op == JSOP_GETGNAME, obj->isGlobal());
+    JS_ASSERT_IF(op == JSOP_CALLGNAME, obj->isGlobal());

Comment 5

[Brian Hackett [Laid off!]]

I think it's possible to have a non-global scope chain, it's just that the holder, once found, should be a global object.  So these should be added at the head of the 'if (!atom)' branch (prop cache hit) and after the js_FindPropertyHelper later on (prop cache miss).  Definitely need these assertions.

Comment 6

[Paul Biggar]

Attachment: WIP

This seems to fix it, but it doesn't seem right.

Comment 7

[Paul Biggar]

jorendorff, waldo: In attachment 531927 [details] [diff] [review], removing the |arguments| special case from BindFunctionLocal fixes the bytecode generation. There is a subsequent check for |arguments| in BindLocalVariable which special cases an argument named "arguments". So this patch treats an assignment to arguments as any other assignment.


function A() { var arguments = ''; var arguments; }
function B() { var a = ''; var a; }

Before this patch, when executing A, the first assignment's upvarCookie is "free", due to its path through BindFunctionLocal (that is, it exits early). When executing B however, the first assignment gets its cookie set in BindFunctionLocal. I really don't understand what cookies represent (comment improvement needed!), so I'm really not sure the right way to proceed here.

Comment 8

[Paul Biggar]

Attachment: fix

Think I've got this one mostly worked out. Nothing to do with upvarCookies as it happens.

Consider the difference between the |var arguments;| stmts in these functions:

function A() { var arguments = ''; var arguments; }
function C() { var arguments; }

In jsparse.cpp:BindVarOrConst, arguments has an attached definition (|var arguments = '';|. That causes the function to return early, and call BindFunctionLocal (which has a special case to convert it from a JSOP_NAME to a JSOP_ARGUMENTS). I don't think we want to go down that path, but we also don't want to go down the define path, so this looks right.

Comment 9

[Paul Biggar]

Review ping?

Comment 10

[David Mandelin [:dmandelin]]

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

Stealing review.

Comment 11

[David Mandelin [:dmandelin]]

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

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

::: js/src/jsinterp.cpp
@@ +4679,5 @@
>      JSAtom *atom;
>      JS_PROPERTY_CACHE(cx).test(cx, regs.pc, obj, obj2, entry, atom);
>      if (!atom) {
>          ASSERT_VALID_PROPERTY_CACHE_HIT(0, obj, obj2, entry);
> +        JS_ASSERT_IF(op == JSOP_GETGNAME || op == JSOP_CALLGNAME, obj->isGlobal());

I believe this should be asserting on obj2, which is the holder, as mentioned in an earlier comment. Same for the other instance of this.

::: js/src/jsparse.cpp
@@ +4006,5 @@
>  
>      if (!ale) {
>          if (!Define(pn, atom, tc))
>              return JS_FALSE;
> +    } else if (atom != cx->runtime->atomState.argumentsAtom) {

This doesn't seem right to me: don't we still have to do all the stuff relating to let, etc., for the name |arguments|? It seems that the special case in BindFunctionLocal is what needs fixing.

Comment 12

[Christian Holler (:decoder)]

I cannot reproduce this on TI, is the underlying bug still present?

Comment 13

[Brian Hackett [Laid off!]]

The crash was fixed in bug 647695, but the underlying semantic issue is (I think) still present.

Comment 14

[Norbert Lindenberg]

Attachment: Source and byte code version of crashing function

I'm now seeing crashes with a similar assertion failure in the attached function:

Missing type at #4:00344 pushed 0: [0x103534610]
Assertion failure: [infer failure] Missing type pushed 0: [0x103534610], at /Users/standards/mozilla/intl/js/src/jsinfer.cpp:328
/Users/standards/bin/js: line 14: 92564 Segmentation fault: 11  /Users/standards/mozilla/builds/$SRC-js-debug/dist/bin/js -U "$@"

Unfortunately, the environment in which I'm seeing them involves includes over 6000 lines of patches (none of them near type inference or code generation) and a library yet to be integrated into Mozilla, and I haven't been able to trim it down to a self-contained test case that I could attach here.

Comment 15

[Norbert Lindenberg]

(In reply to Norbert Lindenberg from comment #14)
> Created attachment 666415 [details]
> Source and byte code version of crashing function
> 
> I'm now seeing crashes with a similar assertion failure in the attached
> function (...)

The crashes I saw were fixed through bug 811566.

Comment 16

[Christian Holler (:decoder)]

(In reply to Brian Hackett (:bhackett) from comment #13)
> The crash was fixed in bug 647695, but the underlying semantic issue is (I
> think) still present.

Is there anything left to do here or can we close this bug? :)

Comment 17

[Brian Hackett [Laid off!]]

I have no clue.

Comment 18

[Christian Holler (:decoder)]

Then I'll close this as WFM now since the crash is fixed.