This is the first step toward calling arbitrary native code on trace. The tracer needs a chance to reconstruct the JS stack (making a note to bail off any currently executing traces) when a native tries to access cx->fp.

Can we assume that if a native needs cx->fp in any given call, it will need cx->fp in all calls? If so, is the idea to reconstruct frames but resume the trace? That would leave the trace's loop exit finding frames at cx->fp above the entry frame. How could we detect that and avoid re-reconstruction? /be

(In reply to comment #1) > Can we assume that if a native needs cx->fp in any given call, it will need > cx->fp in all calls? That's not the case if the native is simply using cx->fp in its error reporting, which I think is a common pattern.

(In reply to comment #2) > (In reply to comment #1) > > Can we assume that if a native needs cx->fp in any given call, it will need > > cx->fp in all calls? > > That's not the case if the native is simply using cx->fp in its error > reporting, which I think is a common pattern. We could arrange for any error-as-exception to abort the trace, and therefore always reconstruct frames from js_ErrorToException/InitExnPrivate -- of course provided that js_ExecuteTree can detect that the non-entry frames have already been reconstructed, and avoid re-reconstructing 'em disastrously. So the two tricks seem to be ensuring cx->fp-dependency trace recording/execution invariance, and detecting early reconstruction and coping on trace exit. /be

(In reply to comment #1) > Can we assume that if a native needs cx->fp in any given call, it will need > cx->fp in all calls? Heh - it's not true even for the hand-picked functions we've been whitelisting (like Array.p.join, bug 460336), much less arbitrary native functions. (In reply to comment #3) > So the two tricks seem to be ensuring cx->fp-dependency trace > recording/execution invariance, and detecting early reconstruction and coping > on trace exit. I think we have to do the latter, with an extra guard when we call an unknown native. The former seems fundamentally untrustworthy. (Natives can already declare themselves INFALLIBLE to avoid the extra guard; we'll keep that.)

Right now, the code to regenerate the stack can fail with OOM. It looks like I have to choose between (a) making that stuff completely infallible; and (b) breaking APIs like JS_IsConstructing, JS_IsAssigning, JS_FrameIterator, etc. that access cx->fp but are supposed to be infallible.

(c) pre-allocate stackPool space before entering a trace for the worst-case needs of any native traced from the tree being triggered plus trees it calls. /be

We already have a similar mechanism for doubles so its easy to extend and should cost perf.

"should not cost perf", of course. All we need to do is ensure enough stackPool space is pre-allocated. That reminds me: we need stackPool to change from a JSArenaPool to a jsval growable array that doubles and halves as needed. I'll file a bug if one isn't already on file. /be

There are a few kinds of places where cx->fp is used. 1. Inside the interpreter (functions marked JS_REQUIRES_STACK for static analysis), the stack is always complete: the fact that we're in the interpreter means we're not on trace. Direct access to the JS stack is fine. 2. Outside the interpreter but in js/src, we'll call js_GetTopStackFrame. This is the new function where the JS stack reconstitution will ultimately go. 3. Outside js/src, I'm calling JS_FrameIterator and JS_GetScriptedCaller, both of which now call js_GetTopStackFrame. Some places (e.g. LiveConnect) still assign to cx->fp, but that's OK. The static analysis makes sure there are no paths from 2 or 3 to 1 that don't go through js_GetTopStackFrame at some point.

Comment on attachment 345342 [details] [diff] [review] v1 I'm withdrawing this one for now. It needs a few tweaks.

This version puts the assertion in js_GetTopStackFrame under #ifdef DEBUG_jason. The only test that currently triggers that assertion for me is doing new String("foo").match("bar") which ends up in js_ValueToString and ultimately js_Invoke. But this is because our existing tests don't do everything in a loop. :) The test in bug 460336 comment 0 triggers it too. (The latter, at least, is a good thing. In bug 462027 we'll replace the assertion with code that allows life to go on from there.)

Comment on attachment 345605 [details] [diff] [review] v2 >+ *scriptp = JS_GetScriptedCaller(cx, NULL)->script; For JS_GetScriptedCaller and JS_IsConstructing, I wondered aloud on IRC whether it's worth adding a js_* API to avoid relocatable function call penalties. >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp >@@ -4562,20 +4563,20 @@ js_generic_native_method_dispatcher(JSCo >- JS_ASSERT(cx->fp->argv == argv); >+ JS_ASSERT(js_GetTopStackFrame(cx)->argv == argv); > if (!js_ComputeThis(cx, JS_TRUE, argv)) > return JS_FALSE; >- cx->fp->thisp = JSVAL_TO_OBJECT(argv[-1]); >+ js_GetTopStackFrame(cx)->thisp = JSVAL_TO_OBJECT(argv[-1]); Calling js_GetTopStackFrame inside JS_ASSERT seems like it's asking for trouble. You could, instead, move the assertion under the call to js_ComputeThis, after you've called it to set thisp for real. >diff --git a/js/tests/jsDriver.pl b/js/tests/jsDriver.pl The changes to this file deserve their own bug. >diff --git a/xpcom/analysis/jsstack.js b/xpcom/analysis/jsstack.js dmandelin should really review this file... >+ warning("(Remove the workaround in jsinterp.js and recompile to get a JS stack trace.)", >+ location_of(isn)); ...but that jsinterp.js should be jsstack.js anyway. r=mrbkap with my comments addressed.

Addresses mrbkap's comments. Carrying forward the r+. dmandelin, please review the analysis.

This patch has no effect on the SunSpider numbers.

The explanation of the RED and GREEN properties is very clear, but I think you want to add a note saying which annotations are trusted and which are untrusted. Partly because I'm not entirely sure after reading the code. +// Why is this necessary? We don't know. +MapFactory.use_injective = true; The answer is that it makes things run faster because each hash bucket can only ever contain one thing. +function attrs(tree) { + let a = DECL_P(tree) ? DECL_ATTRIBUTES(tree) : TYPE_ATTRIBUTES(TREE_TYPE(tree)); + return translate_attributes(a); +} + +function hasUserAttribute(tree, attrname) { + let attributes = attrs(tree); + if (attributes) { + for (let i = 0; i < attributes.length; i++) { + let attr = attributes[i]; + if (attr.name == 'user' && attr.value.length == 1 && attr.value[0] == attrname) + return true; + } + } + return false; +} Please put the above 2 functions into Treehydra in gcc_util.js. + // Ordinarily a user of ESP runs the analysis, then generates output based + // on the results. But in our case (a) we need sub-basic-block resolution, + // which ESP doesn't keep; (b) it so happens that even though ESP can + // iterate over blocks multiple times, in our case that won't cause + // spurious output. (It could cause us to the same error message each time + // through--but that's easily avoided.) Therefore we generate the output + // while the ESP analysis is running. This is entirely reasonable. + // Tell ESP that fndecl is a "variable". It'll be 1 in RED regions and + // "don't know" in GREEN regions. We are lying to ESP because ESP tracks the + // state of variables only. + this._state_var_decl = fndecl; + let state_var = new ESP.PropVarSpec(this._state_var_decl, + false, + isRed(fndecl) || isTurnRed(fndecl) ? 1 : undefined); The comment is not quite true. ESP tracks the variables that your flow function sets. The special significance of property variables is that ESP analyzes them in a path-sensitive way. If you set param 2 (keep) to 'true', you will not need to add these to keepVars later on. If that doesn't work, it's a bug in ESP. Param 3 seems wrong. If the function is a TurnsRed, doesn't that mean the state is not necessarily red at the start? Or are they more magical than I think? + // Lame - hack private data structures to make sure ESP doesn't throw away + // the value of our state variable. + for (let bb in cfg_bb_iterator(cfg)) + bb.keepVars.add(this._state_var_decl);

(In reply to comment #16) > The explanation of the RED and GREEN properties is very clear, but I think you > want to add a note saying which annotations are trusted and which are > untrusted. Partly because I'm not entirely sure after reading the code. What do "trusted" and "untrusted" mean here? > +function attrs(tree) { ... > +function hasUserAttribute(tree, attrname) { ... > > Please put the above 2 functions into Treehydra in gcc_util.js. Per IRC discussion I'm not doing this yet. I'm not comfortable enough that my use of attributes here makes any sense. Waiting for more experience with GCC attributes.

(In reply to comment #17) > (In reply to comment #16) > > The explanation of the RED and GREEN properties is very clear, but I think you > > want to add a note saying which annotations are trusted and which are > > untrusted. Partly because I'm not entirely sure after reading the code. > > What do "trusted" and "untrusted" mean here? By "untrusted" I meant checked by the analysis. By "trusted" I meant not checked by the analysis (i.o.w., the analysis assumes the annotation is correct).

Ah, right, OK. If I explained that, it would also answer this comment: > Param 3 seems wrong. If the function is a TurnsRed, doesn't that mean the state > is not necessarily red at the start? Or are they more magical than I think? because the TurnsRed annotation is trusted. It would be pretty easy to check it, given a single trusted TurnsRed function that the others would call.

Rebased for checkin at last. This also addresses dmandelin's comments. Carrying forward mrbkap's review again.

Comment on attachment 351403 [details] [diff] [review] v4 need this on 1.9.1?

benjamin: the test is part of treehydra?

Yes, we have automated static analysis to verify that code doesn't use cx->fp incorrectly. It's currently running on mozilla-central, but I plan to set something up for tracemonkey as well to catch bugs early.

verified 1.9.1, 1.9.2 based on green static analysis for mozilla-central and tracemonkey.