Closed Bug 510644 Opened 13 years ago Closed 13 years ago

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


(Core :: JavaScript Engine, defect, P2)




Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- wanted


(Reporter: gkw, Assigned: brendan)



(Keywords: assertion, regression, testcase, Whiteboard: fixed-in-tracemonkey)


(1 file, 2 obsolete files)

function foo(f) {
    (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).
Flags: blocking1.9.2?
This asserts on 1.9.1 branch too.
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_);                                   \
 #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 {
     JSThread        *owner;     /* for thread-safe life-cycle assertions */
+    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
Depends on: 513119
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.

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

Assignee: general → brendan
Attached patch proposed fix (obsolete) — Splinter Review
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.

Attachment #398509 - Flags: review?(jorendorff)
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Attachment #398509 - Attachment is obsolete: true
Attachment #398721 - Flags: review?(jorendorff)
Attachment #398509 - Flags: review?(jorendorff)
Attachment #398721 - Flags: review?(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;
Attached patch proposed fix, v3Splinter Review
Attachment #398721 - Attachment is obsolete: true
Attachment #398771 - Flags: review?(jorendorff)
>               case JSOP_LOOKUPSWITCH:
>               case JSOP_LOOKUPSWITCHX:
>               {
>+#ifdef DEBUG
>+                JSStackFrame *fp = js_GetTopStackFrame(cx);
>+                JS_ASSERT(!fp || !fp->imacpc || fp->script != jp->script);

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.
(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.
Attachment #398771 - Flags: review?(jorendorff) → review+
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.

Whiteboard: fixed-in-tracemonkey
Depends on: 514790
Depends on: 514943
Closed: 13 years ago
Resolution: --- → FIXED
Blocking 1.9.2+.  P2.
Flags: blocking1.9.2? → blocking1.9.2+
blocking1.9.1: --- → ?
blocking1.9.1: ? → ---
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.