492113 - jsstack error calling TraceRecorder::record_SetPropHit from js_SetPropertyHelper

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Smedberg]

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

[Jim Blandy :jimb]

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.

Comment 2

[Benjamin Smedberg]

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.

Comment 3

[Benjamin Smedberg]

Attachment: Assert that we're not on trace in js_SetPropertyHelper, rev. 1

I feel uncomfortable patching code without really understanding it at all.

Comment 4

[Jason Orendorff [:jorendorff]]

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?

Comment 5

[Jason Orendorff [:jorendorff]]

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

[Jim Blandy :jimb]

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

[Jim Blandy :jimb]

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?

Comment 8

[Brendan Eich [:brendan]]

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

[Jim Blandy :jimb]

(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

[Jim Blandy :jimb]

Attachment: Bug 492113: Don't call TraceRecorder::record_X from C++ called from a tree.

Comment 11

[Jason Orendorff [: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.

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(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? :)

Comment 14

[Igor Bukanov]

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.

Comment 15

[Igor Bukanov]

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

[Jim Blandy :jimb]

That totally makes sense now.

Ben's patch, not the variable name.

Comment 17

[Benjamin Smedberg]

Attachment: 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.

Comment 18

[Igor Bukanov]

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.

Comment 19

[Benjamin Smedberg]

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.

Comment 20

[Robert Sayre]

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

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ab3f0c9fc75e