470310 - TM: Interpreter errors or pending exceptions should abort trace

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

this.__defineSetter__('m', [].map);
function f() { for (var j = 0; j < 4; ++j) if (j == 3) m = 6; }
try { f(); } catch(e) { print(e); }

Assertion failure: (uint32)((atoms - script->atomMap.vector + ((uintN)(((regs.pc + 0)[1] << 8) | (regs.pc + 0)[2])))) < objects_->length, at ../jstracer.cpp:8284

Comment 1

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

Variants of the testcase crash opt builds at seemingly scary addresses at TraceRecorder::record_JSOP_ENTERBLOCK(), I'm seeing this pretty often.

Comment 2

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

Variants of this testcase also crash opt builds near null at nanojit::LirBufWriter::insLinkToFar with the same assertion.

Comment 3

[Graydon Hoare :graydon]

Bisected down to rev c0d189525474, fix for bug 469927. Looking at details presently.

Comment 4

[Graydon Hoare :graydon]

Attachment: Fix the bug

Per diagnosis and discussion with Brendan, the cause here is quite distant from the symptom: the testcase raises an exception inside a native and we're not noticing it and aborting the trace, so the "culprit" cset is just the first traced opcode that has its assumptions violated. 

Fix is to (quite conservatively) treat all passes through the 'error:' label in jsinterp.cpp as trace-aborting, and avoid tracing into the catch block at all. Can be made more precise if we want to trace through exceptions in the future.

Correct behavior of the testcase is to raise 'TypeError: 6 is not a function'.

Comment 5

[Andreas Gal :gal]

Except for pathological benchmark programs (I am looking at you SpecJVM98) exceptions are rare, and should never be traced.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 357999 [details] [diff] [review]
Fix the bug

>diff -r 0ebfd2df2a20 js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp	Tue Jan 20 17:44:23 2009 -0800
>+++ b/js/src/jsinterp.cpp	Wed Jan 21 11:50:08 2009 -0800
>@@ -6880,6 +6880,15 @@
>     }
> 
>     JS_ASSERT((size_t)(regs.pc - script->code) < script->length);

Nit: blank line here.

>+#ifdef JS_TRACER
>+    /* 
>+     * This abort could be weakened to permit tracing through exceptions that
>+     * are thrown and caught within a loop, with the co-operation of the tracer. 
>+     * For now just bail on any sign of trouble.
>+     */
>+    if (TRACE_RECORDER(cx))
>+        js_AbortRecording(cx, "general interpreter error");

Nit: how about "error or exception while recording"? The code here is general but the error or exception is particular, and it's not the interpreter that has the error if a script such as this bug's testcase causes a native to throw.

>+#endif

Nit: blank line here.

>     if (!cx->throwing) {
>         /* This is an error, not a catchable exception, quit the frame ASAP. */
>         ok = JS_FALSE;

r=me with nits picked, thanks.

/be

Comment 7

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/bc46119d9d97

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bc46119d9d97

Comment 9

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3def7e4b05ce

Comment 10

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/8f8f86fb29f9

Comment 11

[Bob Clary [:bc] (inactive)]

js1_6/extensions/regress-470310.js
v 1.9.1, 1.9.2