510644 - TM: "Assertion failure: JSVAL_IS_DOUBLE(val), at ../jsopcode.cpp"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

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

function foo(f) {
    f()
}
foo((eval("\
    (function () {\
        for each(l in [0, 0xB504F332, 0]) {\
            for (d in Error()) {}\
        }\
    })\
")))

asserts js debug shell on TM branch with -j at Assertion failure: JSVAL_IS_DOUBLE(val), at ../jsopcode.cpp:3964. It previously asserted at Assertion failure: (((val) & (((JSUint32)1 << (3)) - 1)) == 0x2), at ../jsopcode.cpp:3959 - no idea when the assertion morphed though.

autoBisect shows it is probably related to bug 487134, where it just began asserting:

The first bad revision is:
changeset:   27516:b8cf788763a0
user:        jorendorff
date:        Tue May 05 14:26:06 2009 -0700
summary:     Record all calls to native functions (487134, r=gal, brendan).

Comment 1

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

This asserts on 1.9.1 branch too.

Comment 2

[Jason Orendorff [:jorendorff]]

Once again, a pre-existing bug revealed by my innocent patch...

GET_ATOM_FROM_BYTECODE behaves strangely when the decompiler tries to decompile the currently-executing function while cx->fp->imacpc is non-null.

The patch below works for *this* test case, but it's probably not correct generally. It's not clear to me which atom-loading macros are supposed to be used in which cases. I need to look at it more carefully before patching.

diff --git a/js/src/jsopcode.h b/js/src/jsopcode.h
--- a/js/src/jsopcode.h
+++ b/js/src/jsopcode.h
@@ -312,7 +312,7 @@ js_GetIndexFromBytecode(JSContext *cx, J
 #define GET_ATOM_FROM_BYTECODE(script, pc, pcoff, atom)                       \
     JS_BEGIN_MACRO                                                            \
         uintN index_ = js_GetIndexFromBytecode(cx, (script), (pc), (pcoff));  \
-        JS_GET_SCRIPT_ATOM((script), index_, atom);                           \
+        (atom) = (script)->getAtom(index_);                                   \
     JS_END_MACRO
 
 #define GET_OBJECT_FROM_BYTECODE(script, pc, pcoff, obj)                      \
diff --git a/js/src/jsscript.h b/js/src/jsscript.h
--- a/js/src/jsscript.h
+++ b/js/src/jsscript.h
@@ -130,6 +130,11 @@ struct JSScript {
 #ifdef CHECK_SCRIPT_OWNER
     JSThread        *owner;     /* for thread-safe life-cycle assertions */
 #endif
+
+    JSAtom * getAtom(size_t index) const {
+        JS_ASSERT(index < atomMap.length);
+        return atomMap.vector[index];
+    }
 };
 
 #define JSSF_NO_SCRIPT_RVAL     0x01    /* no need for result value of last

Comment 3

[Brendan Eich [:brendan]]

JS_GET_SCRIPT_ATOM does try to handle imacros, and succeeds for this bug's testcase when run in the interpreter. Why does it go wrong with -j? Because we call imacros only when recording, duh (to self ;-).

This is easy: imacros do not use common atoms (all of which are string-keyed, none is double-keyed) for ops such as JSOP_DOUBLE. Only JSOP_STRING and other string-atom consumers (property name ops, etc.) should test imacpc and switch to the common atoms if non-null and the script being decompiled matches the one that is running.

/be

Comment 4

[Brendan Eich [:brendan]]

Sorry for the latent bug -- I will try to fix quickly.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

This is hard to type-check due to multi-typed immediates for JSOP_LOOKUPSWITCH*. Also we cannot use JSOP_DOUBLE in imacros. For now I've injected assertions (some redundant but given this mess that seems better). Suggestions for a better way are welcome, but this does fix the bug.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix up decompiler's JSOP_LOOKUPSWITCH* assertion

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 398721 [details] [diff] [review]
fix up decompiler's JSOP_LOOKUPSWITCH* assertion

Here's the test case that breaks this for string atoms.

function f() {
    for (var i=0; i<9; i++)
        assertEq("" + f, expected);
}
var expected = "" + f;
f();

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix, v3

Comment 9

[Jason Orendorff [:jorendorff]]

>               case JSOP_LOOKUPSWITCH:
>               case JSOP_LOOKUPSWITCHX:
>               {
>+#ifdef DEBUG
>[...]
>+                JSStackFrame *fp = js_GetTopStackFrame(cx);
>+                JS_ASSERT(!fp || !fp->imacpc || fp->script != jp->script);
>+#endif

This seems too strict. We might be decompiling the current function, which contains a switch, and we might be in an imacro too. That would trip this assertion, I think, even though the LOOKUPSWITCH isn't in the imacro.

You can test by adding a LOOKUPSWITCH to function f in comment 7.

>+        if (pc_ && size_t((pc_) - (script_)->code) >= (script_)->length) {  \

Can't tell you how happy I was to hear this doesn't compile if you pass pc_ to the macro. :) Cc-ing Andreas and Shaver.

Don't forget the trace-tests!

r=me with those fixes.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to comment #9)
> >+        if (pc_ && size_t((pc_) - (script_)->code) >= (script_)->length) {  \
> 
> Can't tell you how happy I was to hear this doesn't compile if you pass pc_ to
> the macro. :) Cc-ing Andreas and Shaver.

"if you pass NULL to the macro." Sorry, got a little carried away.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 398771 [details] [diff] [review]
proposed fix, v3

It'll be nice to get rid of this bug. I have seen bizarre results from js_Disassemble() during debugging, almost certainly due to this.

Comment 12

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/6a7fc22fd61d

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/6a7fc22fd61d

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/4c625fa977e0

Comment 15

[Damon Sicore (:damons)]

Blocking 1.9.2+.  P2.

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a2ec496f3b29

Comment 17

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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