Closed Bug 470310 Opened 13 years ago Closed 13 years ago

TM: Interpreter errors or pending exceptions should abort trace

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: jruderman, Assigned: graydon)

Details

(4 keywords)

Attachments

(1 file)

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
Variants of the testcase crash opt builds at seemingly scary addresses at TraceRecorder::record_JSOP_ENTERBLOCK(), I'm seeing this pretty often.
Flags: blocking1.9.1?
Keywords: crash
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.1b3
Flags: blocking1.9.1? → blocking1.9.1+
Variants of this testcase also crash opt builds near null at nanojit::LirBufWriter::insLinkToFar with the same assertion.
Assignee: brendan → graydon
Bisected down to rev c0d189525474, fix for bug 469927. Looking at details presently.
Attached patch Fix the bugSplinter Review
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'.
Attachment #357999 - Flags: review?(brendan)
Except for pathological benchmark programs (I am looking at you SpecJVM98) exceptions are rare, and should never be traced.
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
Attachment #357999 - Flags: review?(brendan) → review+
Summary: TM: JS_GET_SCRIPT_OBJECT assertion failure in record_JSOP_ENTERBLOCK → TM: Interpreter errors or pending exceptions should abort trace
http://hg.mozilla.org/tracemonkey/rev/bc46119d9d97
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 474771
No longer depends on: 474771
Whiteboard: [needs 191
Whiteboard: [needs 191 → [needs 191 landing]
http://hg.mozilla.org/mozilla-central/rev/8f8f86fb29f9
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
js1_6/extensions/regress-470310.js
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.