jsstack error calling TraceRecorder::record_SetPropHit from js_SetPropertyHelper

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

({fixed1.9.1})

unspecified
x86
Linux
fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
After bug 492059 and bug 492040 were fixed, one jsstack error remains:

../../../src/js/src/jsobj.cpp: In function ‘JSBool js_SetPropertyHelper(JSContext*, JSObject*, jsid, JSBool, jsval*)’:
../../../src/js/src/jsobj.cpp:4513: error: cannot call JS_REQUIRES_STACK function TraceRecorder::record_SetPropHit(typedef TraceRecorder*, typedef JSPropCacheEntry*, typedef JSScopeProperty*)
../../../src/js/src/jsobj.cpp:4513: error: cannot call JS_REQUIRES_STACK function js_AbortRecordingImpl(typedef JSContext*, typedef char*)
make[2]: *** [jsobj.o] Error 1

As far as I can tell the analysis is correct: is there external knowledge that this codepath cannot be called from trace?

http://hg.mozilla.org/mozilla-central/annotate/98dab8153e42/js/src/jsobj.cpp#l4513

Comment 1

9 years ago
TRACE_2 will only call SetPropHit if we're recording.  Which implies we're not on trace, or we've at least done a deep abort, so the property JS_REQUIRES_STACK is trying to enforce does actually hold.
(Assignee)

Comment 2

9 years ago
Indeed, but you have to prove it to the static analysis... VOUCH_DOES_NOT_REQUIRE_STACK or JS_ASSERT_NOT_ON_TRACE will do if there's not a more obvious way.
(Assignee)

Comment 3

9 years ago
Created attachment 376771 [details] [diff] [review]
Assert that we're not on trace in js_SetPropertyHelper, rev. 1

I feel uncomfortable patching code without really understanding it at all.
Assignee: general → benjamin
Status: NEW → ASSIGNED
Attachment #376771 - Flags: review?(jorendorff)
Attachment #376771 - Flags: review?(jorendorff) → review-
Comment on attachment 376771 [details] [diff] [review]
Assert that we're not on trace in js_SetPropertyHelper, rev. 1

No, that's not the right fix.  We want to put the ASSERT_NOT_ON_TRACE inside TRACE_ARGS.

Jim, can you take this?
Please disregard what I said in comment 4 about adding an assertion in TRACE_ARGS.  I think this is actually a bug and the static analysis is right to complain about the code!

I just remembered, as Jim and I discussed last week on IRC, that we can in fact be on trace and recording at the same time (recording an outer loop while executing an inner loop).  Then the inner loop can call into native code and we can get here.  In that case I think we have to deep-abort before calling any TRACE_ macro, and this should be done in other places that we can reach on trace, not just here.

Comment 6

9 years ago
We can call trees while recording, but C++ code called from trees can't instigate recording.  Tree frames nest inside recording interpreter frames, but not vice versa.


In the case at hand, js_SetPropertyHelper needs to call TraceRecorder::record_SetPropHit when js_SetPropertyHelper is invoked from recording code, and needs to contribute something to the current recording.  However, it should *not* contribute something when called from tree code; the relevant trace has already been completed and compiled.

Comment 7

9 years ago
In fact, isn't the code as it stands injecting random code into unrelated traces?  That is: we're recording; MONITOR_BRANCH -> jsMonitorLoopEdge -> js_RecordLoopEdge -> js_ExecuteTree -> tree -> C++ code -> js_SetPropertyHelper -> TraceRecorder::record_SetPropHit -> emits various things, even though the operation setting the property was already traced when we generated that tree?
The SetPropHit and Miss hooks were originally called from the interpreter only. They only recently collapsed (SetPropMiss was a thin layer on Hit) and some Hit calls moved into jsobj.cpp. I had spider-sense tingling, should have seen this. Cc'ing Igor.

/be

Comment 9

9 years ago
(Strike "even though...".  When the tree was recorded, we emitted a call to some
C++ code, which may or may not have reached js_SetPropertyHelper at that time. 
The point is that we shouldn't be emitting code here.)

Comment 10

9 years ago
Created attachment 376839 [details] [diff] [review]
Bug 492113: Don't call TraceRecorder::record_X from C++ called from a tree.
Attachment #376771 - Attachment is obsolete: true
Attachment #376839 - Flags: review?(jorendorff)
Benjamin pointed out that there is a cacheResult parameter in both functions that call TRACE_ARGS from outside the interpreter.  This parameter is true only if we are being called *directly* from the interpreter.

So what I said in comment 5 (and Jim echoed in comment 7) is not a real danger.

So ...I think we need to check in the patch I suggested in comment 4.
Unobsolete and r?igor -- I'm taking my spider-sense to the shop for a tune-up. Igor already schooled me on cacheResult meaning "from interpreter" in another bug :-/.

/be
(In reply to comment #12)
> Igor already schooled me on cacheResult meaning "from interpreter" in another
> bug :-/.

With a name like cacheResult, how could it mean otherwise? :)
(Assignee)

Updated

9 years ago
Attachment #376771 - Flags: review- → review?(igor)
(Assignee)

Updated

9 years ago
Attachment #376839 - Attachment is obsolete: true
Attachment #376839 - Flags: review?(jorendorff)

Comment 14

9 years ago
Comment on attachment 376771 [details] [diff] [review]
Assert that we're not on trace in js_SetPropertyHelper, rev. 1

>Bug 492113 - statically assert that we're not on trace in js_SetPropertyHelper, r?jorendorff
>
>
>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -4504,18 +4504,20 @@ js_SetPropertyHelper(JSContext *cx, JSOb
>              * must likewise re-task flags further below for the other 'goto
>              * read_only_error;' case.
>              */
>             flags = JSREPORT_ERROR;
>             if (attrs & JSPROP_READONLY) {
>                 if (!JS_HAS_STRICT_OPTION(cx)) {
>                     /* Just return true per ECMA if not in strict mode. */
>                     PCMETER(cacheResult && JS_PROPERTY_CACHE(cx).rofills++);
>-                    if (cacheResult)
>+                    if (cacheResult) {
>+                        JS_ASSERT_NOT_ON_TRACE(cx);
>                         TRACE_2(SetPropHit, JS_NO_PROP_CACHE_FILL, sprop);
>+                    }

Nit: use here VOUCH_DOES_NOT_REQUIRE_STACK and move the assert to the beginning of the function in the form of JS_ASSERT_IF(cacheResult, !JS_ON_TRACE(cx)) together with a comment that cacheResult implies a call from the interpreter. This would improve the assert coverage.

Ideally that VOUCH_DOES_NOT_REQUIRE_STACK should not be required as cacheResult is a boolean that never changes but I presume our static analysis checker is not powerful enough to deduce that.
Attachment #376771 - Attachment is obsolete: false

Updated

9 years ago
Attachment #376771 - Flags: review?(igor) → review+

Comment 15

9 years ago
Comment on attachment 376771 [details] [diff] [review]
Assert that we're not on trace in js_SetPropertyHelper, rev. 1

r+ with that nit addressed.

Comment 16

9 years ago
That totally makes sense now.

Ben's patch, not the variable name.
(Assignee)

Comment 17

9 years ago
Created attachment 377217 [details] [diff] [review]
Assertion at the beginning js_SetPropertyHelper is enough, rev. 1.1

The analysis is smart enough to correlate on cacheResult as long as you use JS_ASSERT_NOT_ON_TRACE, so the following patch is sufficient.
Attachment #376771 - Attachment is obsolete: true
Attachment #377217 - Flags: review?

Comment 18

9 years ago
Comment on attachment 377217 [details] [diff] [review]
Assertion at the beginning js_SetPropertyHelper is enough, rev. 1.1

>@@ -4440,16 +4440,20 @@ js_SetPropertyHelper(JSContext *cx, JSOb
> 
>+    if (cacheResult) {
>+        JS_ASSERT_NOT_ON_TRACE(cx);
>+    }

Nit: remove {} braces to follow SM book of style.
Attachment #377217 - Flags: review? → review+
(Assignee)

Comment 19

9 years ago
http://hg.mozilla.org/tracemonkey/rev/fcdd5f0d00a6

sayrer, if you could merge this through to -central and -1.9.1 along with the other TM stuff, I'd appreciate that.
Whiteboard: fixed-in-tracemonkey

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.