Closed Bug 488018 Opened 11 years ago Closed 11 years ago

TM: test_property_cache treats errors in js_LookupPropertyWithFlags just as signals to abort recording

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: jimb)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(4 files, 2 obsolete files)

test_property_cache contains the following code:

  5924         if (JOF_OPMODE(*pc) == JOF_NAME) {
  5925             JS_ASSERT(aobj == obj);
  5926             if (js_FindPropertyHelper(cx, id, &obj, &obj2, &prop, &entry) < 0)
  5927                 ABORT_TRACE("failed to find name");
  5928         } else {
  5929             int protoIndex = js_LookupPropertyWithFlags(cx, aobj, id,
  5930                                                         cx->resolveFlags,
  5931                                                         &obj2, &prop);
  5932             if (protoIndex < 0)
  5933                 ABORT_TRACE("failed to lookup property");

This effectively suppresses any errors or exceptions raised in js_LookupPropertyWithFlags and js_FindPropertyHelper. Instead the code treats them just as signals to abort the trace without propagating the conditions to the interpreter.
Flags: blocking1.9.1?
A similar problem exists in activeCallOrGlobalSlot. There errors from js_FindProperty are treated just as if the property was not found.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Assignee: general → jim
The approach that seems cleanest would be:

- Add a JSMRS_ERROR to the JSMonitorRecordingStatus enum.
- Make all the TraceRecorder::record_ functions return JSMonitorRecordingStatus instead of bool, with the obvious interpretations
- Have them return JSMRS_IMACRO to indicate that they've invoked an imacro.  Delete JSFRAME_IMACRO_START.
- Have TraceRecorder::monitorRecording propagate these values as needed.
- Have js_Interpret check for JSMRS_ERROR.

Brendan says, "may as well do the clean thing, sanity-check codesize and test perf carefully -- should work out"
v1.  Testing, benchmarking, and checking size.

'false' becomes 'JSMRS_STOP'; 'true' becomes 'JSMRS_CONTINUE'.  Since
subsequent patches will begin to use more than two values (the next
assigns meaning to JSMRS_IMACRO), take care to propagate values to
callers accurately.

Conditionally provide an alternative definition for the
JSMonitorRecordingStatus type that cannot be converted to 'bool', so
that we can use the C++ compiler to catch improperly converted code.
No test or perf regressions.
jstracer.o 468 bytes larger; jsinterp.o 6 bytes smaller.
Fixed up line wrapping.
Attachment #374481 - Attachment is obsolete: true
Attachment #374504 - Flags: review?(igor)
Comment on attachment 374504 [details] [diff] [review]
Bug 488018: Make 'record_' functions return JSMonitorRecordingStatus, not bool.

Saw some two-space indentation after the first four-space indent in CHECK_STATUS.

Non-nit about CHECK_STATUS: is it the cause of most codesize increase in jstracer.cpp?

>+#define ABORT_TRACE(msg)   do { debug_only_a(fprintf(stdout, "abort: %d: %s\n", __LINE__, msg);)  return JSMRS_STOP; } while (0)

Time to use JS_BEGIN/END_MACRO, standard backslash in column 79, etc.?

>@@ -648,7 +679,9 @@ public:
> #define TRACE_ARGS_(x,args)                                                   \
>     JS_BEGIN_MACRO                                                            \
>         TraceRecorder* tr_ = TRACE_RECORDER(cx);                              \
>-        if (tr_ && !tr_->wasDeepAborted() && !tr_->record_##x args)           \
>+        if (tr_ &&                                                            \
>+            !tr_->wasDeepAborted() &&                                         \
>+            tr_->record_##x args != JSMRS_CONTINUE)                           \
>             js_AbortRecording(cx, #x);                                        \
>     JS_END_MACRO

How does this actually propagate errors back to the interpreter?

/be
Attachment #374504 - Flags: review?(igor)
Comment on attachment 374504 [details] [diff] [review]
Bug 488018: Make 'record_' functions return JSMonitorRecordingStatus, not bool.

>-JS_REQUIRES_STACK bool
>+JS_REQUIRES_STACK JSMonitorRecordingStatus
> TraceRecorder::isValidSlot(JSScope* scope, JSScopeProperty* sprop)
> {

isValidSlot should stay bool - the code there cannot fail or trigger imacro. If that would change in future, lets do such status refactoring then. Doing it now just adds source complexity. r+ with that fixed.
Could we change the name "JSMonitorRecordingStatus" to maybe just JSRecordingStatus?
(In reply to comment #7)
> Could we change the name "JSMonitorRecordingStatus" to maybe just
> JSRecordingStatus?

I forgot to suggest that but the same thought occurred. More generic, shorter, +1.

/be
Jim, you must be planning multiple patches for this bug as summarized. From our experience in bug 488363 (followup work now split out as bug 490044) this usually bites back.

/be
Yes --- I thought it would be easier to review that way.  No, the current patch doesn't propagate errors at all.  I take it waiting for a complete patch set before asking for review is okay?
(In reply to comment #6)
> isValidSlot should stay bool - the code there cannot fail or trigger imacro. If
> that would change in future, lets do such status refactoring then. Doing it now
> just adds source complexity. r+ with that fixed.

isValidSlot calls ABORT_TRACE.  In other circumstances, ABORT_TRACE needs to return JSMonitorRecordingStatus.
Incremental review is fine I think -- can mq combine patches into one for landing?

/be
(In reply to comment #12)
> Incremental review is fine I think -- can mq combine patches into one for
> landing?

Of course! hg help qfold has all the answers.
(In reply to comment #11)
> isValidSlot calls ABORT_TRACE.  In other circumstances, ABORT_TRACE needs to
> return JSMonitorRecordingStatus.

This suggests to have a customizable variant of ABORT_TRACE that returns whatever the caller needs.
(In reply to comment #5)
> Non-nit about CHECK_STATUS: is it the cause of most codesize increase in
> jstracer.cpp?

No --- GCC generates instructions with 32-bit literals for JSRecordingStatus
values, vs. byte instructions for bool.  It's the cumulative effect of all
those 'return' statements.
(In reply to comment #13)
> Of course! hg help qfold has all the answers.

I can fold before landing.
Blocks: 452357
The JSMRS_ prefix becomes JSRS_, accordingly.
Attachment #374680 - Flags: review?(igor)
'false' becomes 'JSRS_STOP'; 'true' becomes 'JSRS_CONTINUE'.  Since
subsequent patches will begin to use more than two values (the next
assigns meaning to JSRS_IMACRO), take care to propagate values to
callers accurately.

ABORT_TRACE becomes either ABORT_TRACE or ABORT_TRACE_BOOLEAN,
depending on the value needed by the context.

Conditionally provide an alternative definition for the
JSRecordingStatus type that cannot be converted to 'bool', so that we
can use the C++ compiler to catch improperly converted code.
Attachment #374504 - Attachment is obsolete: true
Attachment #374681 - Flags: review?(igor)
Eliminate the JSFRAME_IMACRO_START frame flag.  Instead, return
JSRS_IMACRO directly from recording functions.
Attachment #374682 - Flags: review?(igor)
Define a new recording status, JSRS_ERROR.  Return it from recording
functions when appropriate.  Check for it at appropriate bottlenecks
in tracer and interpreter.
Attachment #374683 - Flags: review?(igor)
That's the complete patch series as it stands.
Passes js/tests and trace-test.js.
No SunSpider effect.
No new aborts.

While this does address the issues raised in comment #0, I don't know whether this covers bug 452357 as well.  I've looked over all the uses of JSRS_STOP and ABORT_TRACE in jstracer.cpp, but a more thorough check might be good.
(And I'll fold the patches together before landing.)
Attachment #374680 - Flags: review?(igor) → review+
Comment on attachment 374681 [details] [diff] [review]
Bug 488018: Make 'record_' functions return JSRecordingStatus, not bool.

>+#define ABORT_TRACE_WITH_VALUE(value, msg)                                    \
>+    JS_BEGIN_MACRO                                                            \
>+        debug_only_a(fprintf(stdout, "abort: %d: %s\n", __LINE__, (msg));)    \
>+        return (value);                                                       \
>+    JS_END_MACRO
> #else
> #define debug_only_a(x)
>-#define ABORT_TRACE(msg)   return false
>+#define ABORT_TRACE_WITH_VALUE(value, msg)   return (value)
> #endif
> 
>+#define ABORT_TRACE(msg)         ABORT_TRACE_WITH_VALUE(JSRS_STOP, msg)
>+#define ABORT_TRACE_BOOLEAN(msg) ABORT_TRACE_WITH_VALUE(false,      msg)

Nit: replace ABORT_TRACE_WITH_VALUE with ABORT_TRACE_RV (RV == return value) to follow the pattern from jsopcode.cpp. That eliminates the need for ABORT_TRACE_BOOLEAN as ABORT_TRACE_RV(false, msg) is just as short and to the point. 

Does this patch still increases the size of jstrace.o ?
Attachment #374681 - Flags: review?(igor) → review+
Comment on attachment 374682 [details] [diff] [review]
Bug 488018: Use recording function return value for imacro invocation.


>@@ -674,10 +675,13 @@ public:
> #define TRACE_ARGS_(x,args)                                                   \
>     JS_BEGIN_MACRO                                                            \
>         TraceRecorder* tr_ = TRACE_RECORDER(cx);                              \
>-        if (tr_ &&                                                            \
>-            !tr_->wasDeepAborted() &&                                         \
>-            tr_->record_##x args != JSRS_CONTINUE)                           \
>-            js_AbortRecording(cx, #x);                                        \
>+        if (tr_ && !tr_->wasDeepAborted()) {                                  \
>+            JSRecordingStatus status = tr_->record_##x args;                  \
>+            if (status != JSRS_CONTINUE) {                                    \
>+                js_AbortRecording(cx, #x);                                    \
>+            }                                                                 \

Nit: no need for { } around a single-line statement.
Attachment #374682 - Flags: review?(igor) → review+
Comment on attachment 374683 [details] [diff] [review]
Bug 488018: Propagate errors from tracer to interpreter.

diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -3019,6 +3019,8 @@ js_Interpret(JSContext *cx)
>                     atoms = COMMON_ATOMS_START(&rt->atomState);
>                     op = JSOp(*regs.pc);
>                     DO_OP();    /* keep interrupting for op. */
>+                } else if (status == JSRS_ERROR) {
>+                    goto error;
>                 } else {
>                     JS_ASSERT(status == JSRS_STOP);
>                 }

Nit: replace ifs with switch and comment that the code at error: aborts the recording.  

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -160,8 +160,9 @@ static const char tagChar[]  = "OIDISIBI
> #define ABORT_TRACE_WITH_VALUE(value, msg)   return (value)
> #endif
> 
>-#define ABORT_TRACE(msg)         ABORT_TRACE_WITH_VALUE(JSRS_STOP, msg)
>+#define ABORT_TRACE(msg)         ABORT_TRACE_WITH_VALUE(JSRS_STOP,  msg)
> #define ABORT_TRACE_BOOLEAN(msg) ABORT_TRACE_WITH_VALUE(false,      msg)
>+#define ABORT_TRACE_ERROR(msg)   ABORT_TRACE_WITH_VALUE(JSRS_ERROR, msg) 

I guess at one point ABORT_TRACE must be replaced with ABORT_RECORDING. But this is for another bug.


>   imacro:
>-    if (status != JSRS_STOP)
>+    if (! STATUS_ABORTS_RECORDING(status))

Nit: no blank after !

> 
>-            /* FIXME bug 488018 - this treats OOM as no-cache-fill. */
>-            if (!entry || entry == JS_NO_PROP_CACHE_FILL)
>+            if (!entry)
>+                ABORT_TRACE_ERROR("error in js_FindPropertyHelper");
>+            if (entry == JS_NO_PROP_CACHE_FILL)
>                 ABORT_TRACE("failed to find name");

Pre-existing nit: the message should be "cannot cache name".
Attachment #374683 - Flags: review?(igor) → review+
> Does this patch still increases the size of jstrace.o ?

On OSX, this patch does enlarge jstracer.o by 172 bytes.  I can't easily do a comparison on Linux at the moment, so we can't really compare comparisons.  But see comment #15; most of the original size increase was from the bool->enum change.  Even there, we were talking about 468 bytes; this is not some macro-driven explosion.
(In reply to comment #24)
> Nit: no need for { } around a single-line statement.

True, but it's no longer single-line by the time the whole queue is applied.
(Ready to push this; waiting for IT to install a new SSH key.)
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/78dc61dcc4c5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Need a blocking followup bug to fix this:

Darwin_OPT.OBJ/made:../jstracer.cpp:4584: warning: ‘status’ may be used uninitialized in this function

/be
(In reply to comment #32)
> Need a blocking followup bug to fix this:
> 
> Darwin_OPT.OBJ/made:../jstracer.cpp:4584: warning: ‘status’ may be used
> uninitialized in this function

http://hg.mozilla.org/tracemonkey/rev/308fa16b3bfc
You need to log in before you can comment on or make changes to this bug.