Closed Bug 489255 Opened 16 years ago Closed 12 years ago

Add ability for debugger to disable upvar optimizations

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED WORKSFORME
mozilla1.9.1
Tracking Status
status1.9.1 --- wanted

People

(Reporter: mrbkap, Assigned: gal)

References

Details

(Whiteboard: [firebug-p3][debugger only])

Attachments

(2 files, 1 obsolete file)

Attached file testcase
Steps to notice: * Launch Venkman (or, presumably, Firebug) * Load the testcase * Add a breakpoint on line 12 (the call to fib2). * Click on the button * Enter 'fib2()' in the interactive box Actual results: I get an 'infinite recursion' error Expected results: The fibonacci number for the current value of 'n'. Analysis: The reason for this bug is that in the C++ stack looks like: #18 0xb7dcf432 in js_Interpret (cx=0xb2267000) at /home/mrbkap/work/main/mozilla/js/src/jsinterp.cpp:5144 #19 0xb7de2cda in js_Invoke (cx=0xb2267000, argc=3, vp=0xaff460dc, flags=0) at /home/mrbkap/work/main/mozilla/js/src/jsinterp.cpp:1381 #20 0xb6203cad in nsXPCWrappedJSClass::CallMethod (this=0xa9ab3040, wrapper=0xa8f3dbc0, methodIndex=3, info=0xad56f540, nativeParams=0xbfc57034) at /home/mrbkap/work/main/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1608 #21 0xb61fa967 in nsXPCWrappedJS::CallMethod (this=0xa8f3dbc0, methodIndex=3, info=0xad56f540, params=0xbfc57034) at /home/mrbkap/work/main/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:561 #22 0xb7cfc1cd in PrepareAndDispatch (methodIndex=3, self=0xa8f36ff0, args=0xbfc570f4) at /home/mrbkap/work/main/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:95 #23 0xb643f0b7 in jsds_ExecutionHookProc (jsdc=0xb6c204f0, jsdthreadstate=0xa890fc60, type=1, callerdata=0x1, rval=0xbfc577fc) at /home/mrbkap/work/main/mozilla/js/jsd/jsd_xpc.cpp:682 #24 0xb642f365 in jsd_CallExecutionHook (jsdc=0xb6c204f0, cx=0xb2267000, type=1, hook=0xb643ed14 <jsds_ExecutionHookProc>, hookData=0x1, rval=0xbfc577fc) at /home/mrbkap/work/main/mozilla/js/jsd/jsd_hook.c:177 #25 0xb6431b75 in jsd_TrapHandler (cx=0xb2267000, script=0xad7a18d0, pc=0xad7a1917 "S", rval=0xbfc577fc, closure=0xa8b4a661) at /home/mrbkap/work/main/mozilla/js/jsd/jsd_scpt.c:758 #26 0xb7d8848b in JS_HandleTrap (cx=0xb2267000, script=0xad7a18d0, pc=0xad7a1917 "S", rval=0xbfc577fc) at /home/mrbkap/work/main/mozilla/js/src/jsdbgapi.cpp:317 #27 0xb7dd29f5 in js_Interpret (cx=0xb2267000) at /home/mrbkap/work/main/mozilla/js/src/jsinterp.cpp:5613 note that the trap handler runs jsd JS code on the same context. This temporarily overwrites cx->display, so calling the (usually non-escaping) fib2 function does the Wrong Thing (tm).
Assignee: nobody → general
Status: NEW → ASSIGNED
Component: Venkman JS Debugger → JavaScript Engine
Flags: blocking1.9.1?
OS: Linux → All
Product: Other Applications → Core
QA Contact: venkman → general
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.1b4
Assignee: general → brendan
This is a potentially firebug-crippling regression. Breaking in a function and evaluating expressions is standard procedure, but now some inner functions that used to be evaluated correctly, won't be. Not sure if this is a crash bug, but it seems like it could be. Blake? /be
Yeah, it shouldn't be too hard to construct a testcase that crashes.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Priority: P1 → P2
I don't know what Venkman uses for the interactive box, but I guess its frame.eval() the same as Firebug. Using Firebug 1.4 on FF 3.0.9, the test case gives NaN. I guess the bug here is that fib2() calls fib() which has a breakpoint? I don't have a build of 3.0 to check this. Using Firebug 1.4 on Gecko/20090420 Shiretoko/3.5b4pre, the test case gives "TypeError: fib2 is not a function". Using the expression "fib2" gives back firebug's 'fbs' object, an object global to the component that interfaces with jsd. That really makes no sense. So something is broken in 1.9.1, but my breakage differs from yours.
So Firebug's Watch panel feature will fail in 1.9.1. My guess is that this is the same problem as Blake reports. It looks like frame.eval() is grabbing junk out of newer scopes. So if I eval 'n' from the example above I get the function "onNest()" out of Firebug. Perhaps Blake is getting a different junk, being on a different build and OS. (evals of objects in older scopes or at least the window scope seem ok). But this is just a guess. Do you want a different bug report for Firebug's version of this problem?
No (don't mind my Holmes-quoting ;-). This bug is understood and it can cause every symptom observed in Venkman and Firebug without need for more hypotheses. Let me fix this first and if anything ails any debugger, file as needed. A new bug would just be administrative deadwood at this point. Feel free to note Firebug vs. Venkman differences -- much appreciated. /be
Blocks: 453978
Whiteboard: [firebug-p1]
(In reply to comment #3) > Using the expression "fib2" gives back firebug's 'fbs' object, an object global > to the component that interfaces with jsd. That really makes no sense. > > So something is broken in 1.9.1, but my breakage differs from yours. This sounds like the (now fixed) bug 488843.
(In reply to comment #6) > (In reply to comment #3) > > Using the expression "fib2" gives back firebug's 'fbs' object, an object global > > to the component that interfaces with jsd. That really makes no sense. > > > > So something is broken in 1.9.1, but my breakage differs from yours. > > This sounds like the (now fixed) bug 488843. No. /be
Target Milestone: mozilla1.9.1b4 → mozilla1.9.1
So should JS_HandleTrap just create a new context to pass to the trap? I'd be happy to write up a patch to that effect. That seems cleaner than messing with jsd_TrapHandler... but the latter seems to actually depend on getting some object associated to the cx, so this might not work right...
Brendan, how are we doing here?
Assignee: brendan → igor
Thanks, I am out of time today (I'll try to fix that other firebug-p2 though). Igor, Andreas, Blake and I were thinking we could disable optimizations if the debugger is hooked at compile-time, otherwise throw if debugging optimized code. What were you thinking? /be
(In reply to comment #10) > Andreas, Blake and I were thinking we could disable optimizations if the > debugger is hooked at compile-time, otherwise throw if debugging optimized > code. I initially though it would be possible to hack js_GetUpvar machinery so it could fallthrough to JSOP_NAME after debugger activities. But I have not spotted any simple way do it. Moreover, AFAICS any such hack is similar to supporting escaping upvars. Again, this is possible, but it does not sound like one-liner to replace cx->display with per frame structure that, when frame exists, can be moved to a call object so any child closures can continue to refer to it as is. So currently I am checking another alternative. The idea is to patch script bytecode and replace any JSOP_GETUPVAR with JSOP_NAME when the debugger runs against such script's frame.
(In reply to comment #11) > The idea is to patch script > bytecode and replace any JSOP_GETUPVAR with JSOP_NAME when the debugger runs > against such script's frame. That patching is rather hard. The problem is that JSScript may not have an atom in its atom table for the upvar or the atom may be at the position exceeding 64K. Any of these conditions prevents turning JSOP_GETUPVAR and similar into JSOP_NAME. So I guess a simplest fix would be indeed to have a new context option like JSOPTION_ONLY_DEBUGGABLE_SCRIPTS. The option would prevent the compiler to emit JSOP_GETUPVAR etc. bytecodes. The tricky part is when to check for debugger-safe scripts. It is not sufficient to check for such script when the interpreter is about to execute a function from JSFRAME_DEBUGGER frame. The problem is that the debugger may leak a closure with upvars outside the initial frame and it could be called long after the debugger is disabled. To properly deal with that it would be necessary to verify in any upvar-referencing function that it is only called from the right frame. Alternatively for 1.9.1 it could be just documented that with a debugger installed JSOPTION_ONLY_DEBUGGABLE_SCRIPTS must be set for any script that debugger may ever debug.
Another option is to add JSOP_{GET,CALL}UPVAR_DBG or some such. /be
I'm sort of confused as to why the debugger can't push its own context and run the JS-implemented stuff on that...
(In reply to comment #12) > The problem is that the debugger may leak > a closure with upvars outside the initial frame and it could be called long > after the debugger is disabled. To properly deal with that it would be > necessary to verify in any upvar-referencing function that it is only called > from the right frame. Such leak is only possible through Call, Block or Arguments objects. Which suggests to censor any function object containing upvar references when the getters for those objects find them in the current frame. This may also make possible bugs in the compiler escape analysis less harmful.
(In reply to comment #14) > I'm sort of confused as to why the debugger can't push its own context and run > the JS-implemented stuff on that... Because the problem is that the debugger can leak the upvar-referencing closures and call them from any point when, for example, the parent frame is long gone.
(In reply to comment #16) > Because the problem is that the debugger can leak the upvar-referencing > closures and call them from any point when, for example, the parent frame is > long gone. Don't do that, then? I'm happy to relnote that limitation, given that we're in overtime on the _final_ code freeze, and the alternatives all sound pretty invasive. All debuggers have limitations, and it's not hard to crash or corrupt data by using them dangerously.
(In reply to comment #13) > Another option is to add JSOP_{GET,CALL}UPVAR_DBG or some such. That can be an option. But judging from jsopcode.cpp it is rather non-trivial to recover from the index the name of the corresponding variable. So that patching may make the script to run way to slow even after the debugger is stopped.
(In reply to comment #17) > Don't do that, then? I'm happy to relnote that limitation, given that we're in > overtime on the _final_ code freeze, and the alternatives all sound pretty > invasive. It is not simple to follow that "Don't do" especially when debugging a complex script with closures. So I am in favor of at least adding JSOPTION_ONLY_DEBUGGABLE_SCRIPTS and a minimal sanity check that upvar-bearing closures do not leak that may add an extra protection against the escape analysis bugs.
Depends on: 494235
We need at least an option, although couldn't we detect the presence of a debugger by non-null hook(s) instead of adding a JSOPTION_* code? /be
(In reply to comment #20) > couldn't we detect the presence of a > debugger by non-null hook(s) instead of adding a JSOPTION_* code? The question is which hook? Also, a user may activate a debugger after a script is compiled (like when debugging chrome or extension code). That case requires some option set in advance.
Any jsdbgapi hook, perhaps -- point is how would the jsd layer know when to set a JSOPTION? Timeless, JJB, please weigh in. /be
jsd has on/off and pause/unpause. I've not looked but presumable their is a flag set for these and I guess |pause| disables the hooks without deleting the jsd objects, which should be adequate for this purpose. It's not the presence of the debugger so much as calls to its functions right?
(In reply to comment #23) > jsd has on/off and pause/unpause. I've not looked but presumable their is a > flag set for these and I guess |pause| disables the hooks without deleting the > jsd objects, which should be adequate for this purpose. It's not the presence > of the debugger so much as calls to its functions right? The problem is that the UPVAR optimization is not compatible with the debugger assumption that it can freely call JS_EvaluateInStackFrame and similar API on any JS frame. Disabling the optimization restores the compatibility. Ideally the optimization should be disabled only when compiling a script that will be debugged at some later point. Unfortunately SpiderMonkey cannot yet predict the future reliably and the ideal case has to be approximated. One way is to check for some debugger hook and assume that its mere presence mean that JS_EvaluateInStackFrame can be called. But this may not work if the script user will be debugging was compiled before the hook was installed. In this case an explicit option that a user can set to disable the optimization can be desirable.
I(In reply to comment #24) > But this may not work if the > script user will be debugging was compiled before the hook was installed. In > this case an explicit option that a user can set to disable the optimization > can be desirable. For Firebug I think this is all not so important. The current user experience includes having to reload the page after turning on the debugger. If you don't optimize while the debugger is one, we're good. For Chromebug (and Venkman) it could be more of an issue, if you assume users need a mostly optimized app and only part debugged. I assume you can detected optimized scripts? Is the problem really just frame.eval()? If you want to solve this problem in the short run without API changes, just throw something out of the eval() "NO_CAN_DO_OPTIMIZED". We're used to funky errors anyway ;-). It is possible to recompile the script without the optimization? On the fly recompilation is at the top of our list for jsd2 so that may where a better solution can come from. If this is all off base, sorry. jjb
so, i haven't been reading this (just read it start to finish). igor is right... the debugger it likely to appear (and disappear) randomly and poke a random frame, because that's what its user has asked it to do. the question is basically what can you do when JS_EvaluateInStackFrame and friends are called. I take it that it's impossible to convert the js frame from native to script and resurrect state. Would it be possible to create fake objects solely for the purpose of talking to the debugger? (would that help?)
(In reply to comment #25) > I assume you can detected optimized scripts? Is the problem really just > frame.eval()? If you want to solve this problem in the short run without API > changes, just throw something out of the eval() "NO_CAN_DO_OPTIMIZED". We're > used to funky errors anyway ;-). Besides the frame.eval there are few other cases when a debugging API may expose the optimized closure in a way that may lead to crashes, for example, when user tries to debug inside it. The bug 494235 should address this via throwing an exception when a potentially harmful access is detected. But it will mean that, for example, variables defined in optimized closure can not be accessed. > It is possible to recompile the script without the optimization? One idea for this bug was to patch the bytecode to turn the optimized form into a slow one when it is exposed to the debugger. But it does not sound that easy to do for 1.9.1.
I am going to grab this bug and play with it. I have almost no idea what I am doing here, so if anyone else wants to poke at it, please feel free. Here is my plan: 1. Figure out a way to disable upvar analysis based on a flag in cx and emit code that is safe to run under firebug. 2. Figure out how to make sure that flag gets set when firebug connects. 3. Figure out a way what do to with code that was generated before firebug connected.
Assignee: igor → gal
Igor, if you want to work on this bug this weekend please grab it back.
(In reply to comment #28) > 3. Figure out a way what do to with code that was generated before firebug > connected. For simplicity I suggest to throw an exception when this is detected. This is what the bug 494235 is about.
(In reply to comment #29) > if you want to work on this bug this weekend please grab it back. I plan to focus on the bug 494235 first.
igor, thats a good split since I have absolute no clue how 494235 works. I will try to hack up 1 and 2 and then get some feedback from you. I probably need some serious hand-holding with this. Its not really my area of expertise. I will post an initial patch soon.
Whiteboard: [firebug-p1] → [firebug-p1][debugger only]
Attached patch patch (obsolete) — Splinter Review
This patch adds an option to disable upvar. -u in the shell DISABLES upvar jit.options.upvar is also on by default, and can be set to off
Attached patch patchSplinter Review
Killed a bogus newline.
Attachment #380074 - Attachment is obsolete: true
Attachment #380075 - Flags: review?(igor)
Comment on attachment 380075 [details] [diff] [review] patch Seems to work in the shell. I will do some testing in the browser. Igor, can you take a look?
Tested in a TM debug build. Seems to work fine. I will fire off a try server build.
Comment on attachment 380075 [details] [diff] [review] patch General notion: do we really want upvar disabled by default requiring for embedding to take explicit steps to activate it? I do not think that this is the case. So something like JSOPTION_DISABLE_UPVAR would serve the purpose better IMO. >diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp >@@ -1826,16 +1826,18 @@ JSCompiler::setFunctionKinds(JSFunctionB > if (funbox->tcflags & TCF_FUN_HEAVYWEIGHT) { > FUN_METER(heavy); > JS_ASSERT(FUN_KIND(fun) == JSFUN_INTERPRETED); >+ } else if (!JS_HAS_UPVAR_OPTION(context)) { >+ /* Nothing to be done here. */ Nice - just 2 lines to do the job. >diff --git a/js/src/shell/js.cpp b/js/src/shell/js.cpp >@@ -692,16 +693,20 @@ ProcessArgs(JSContext *cx, JSObject *obj >+ case 'u': >+ JS_ToggleOptions(cx, JSOPTION_UPVAR); >+ break; >+ I do not think that this is necessary. -e 'options("upvar")' would do the job just as well and disabling upvar is not something that should be frequently used. >@@ -4630,16 +4635,17 @@ main(int argc, char **argv, char **envp) > > WITH_LOCKED_CONTEXT_LIST( > cx = JS_NewContext(rt, gStackChunkSize) > ); > if (!cx) > return 1; > > JS_SetGCParameterForThread(cx, JSGC_MAX_CODE_CACHE_BYTES, 16 * 1024 * 1024); >+ JS_ToggleOptions(cx, JSOPTION_UPVAR); /* Enable UPVAR optimization by default. */ Move that to SetContextOptions so upvar would be activated for any context. But this would only be necessary if the engine disables the upvar by default. >diff --git a/js/src/xpconnect/shell/xpcshell.cpp b/js/src/xpconnect/shell/xpcshell.cpp >@@ -1089,16 +1089,19 @@ ProcessArgs(JSContext *cx, JSObject *obj >+ case 'u': >+ JS_ToggleOptions(cx, JSOPTION_UPVAR); >+ break; Hm, apparently xpcshell does not have option() function so here -u is necessary. >@@ -1637,16 +1640,17 @@ main(int argc, char **argv, char **envp) > > gOldJSContextCallback = JS_SetContextCallback(rt, ContextCallback); > > cx = JS_NewContext(rt, 8192); > if (!cx) { > printf("JS_NewContext failed!\n"); > return 1; > } >+ JS_ToggleOptions(cx, JSOPTION_UPVAR); /* Enable UPVAR by default. */ Move that to ContextCallback (but only if the engine would not enable it by default).
We don't want an option to disable an optimization with bugs to fix (there are lots of optimizations including tracing with bugs to fix over time). We want the bugs fixed. /be
(In reply to comment #38) > We don't want an option to disable an optimization with bugs to fix (there are > lots of optimizations including tracing with bugs to fix over time). We want > the bugs fixed. Still an option to provide maximum debugging possibilities can be useful and feature proof. So what about JSOPTION_MAX_DEBUG? Currently the only thing it would do is to disable upvar and, perhaps, jit.
The option allows full debuging without crashing. Combined with a filter in the debugger API to stop closures from leaking that throws an exception and tells the user to turn off upvar in the prefs we would have a viable fix for 3.5rc1. In addition this can be useful to diagnose problems in the field. If we don't want the ability to disable upvar, then we should close/invalidate this bug. The rest of the issue (not leaking flat/null closures) is already being dealt with in a separate bug.
Does the summary of this bug bear any resemblance to the problem that's being worked on at this point, out of curiosity?
(In reply to comment #41) > Does the summary of this bug bear any resemblance to the problem that's being > worked on at this point, out of curiosity? Anyone? (In reply to comment #40) > If we don't want the ability to disable upvar, then we should close/invalidate > this bug. The rest of the issue (not leaking flat/null closures) is already > being dealt with in a separate bug. Are we going to do this?
Resummarizing per comment 40, maybe this doesn't need to block or even be open?
Summary: JavaScript debugger runs JS code on the same context as the debugged code → Add ability for debugger to disable upvar optimizations
(In reply to comment #38) > We don't want an option to disable an optimization with bugs to fix (there are > lots of optimizations including tracing with bugs to fix over time). We want > the bugs fixed. I think we want both. The option to turn off the jit has more than paid for itself as we've been fixing its bugs.
Be that as it may, unblocking. (Next someone will want it to be reset in safe mode, too.)
Flags: blocking1.9.1+ → blocking1.9.1-
Firebug won't need this feature, unless of course the optimization interferes with debugging. Then it will be vital. ;-)
Whiteboard: [firebug-p1][debugger only] → [firebug-p3][debugger only]
Seems like it sure interferes with debugging per comment 0!
Yes, but the upvar bug, 494235 is blocking; so this bug is now just for the hypothetical case of another opt bug. Might be a good strategy to work on this in parallel, but I think there are no more available processors ;-)
Comment on attachment 380075 [details] [diff] [review] patch >@@ -1826,16 +1826,18 @@ JSCompiler::setFunctionKinds(JSFunctionB > > JSParseNode *pn = fn->pn_body; > JSFunction *fun = (JSFunction *) funbox->object; > > FUN_METER(allfun); > if (funbox->tcflags & TCF_FUN_HEAVYWEIGHT) { > FUN_METER(heavy); > JS_ASSERT(FUN_KIND(fun) == JSFUN_INTERPRETED); >+ } else if (!JS_HAS_UPVAR_OPTION(context)) { >+ /* Nothing to be done here. */ > } else if (pn->pn_type != TOK_UPVARS) { > /* > * No lexical dependencies => null closure, for best performance. > * A null closure needs no scope chain, but alas we've coupled > * principals-finding to scope (for good fundamental reasons, but > * the implementation overloads the parent slot and we should fix > * that). See, e.g., the JSOP_LAMBDA case in jsinterp.cpp. > * Apart from the desire to invert the sense of the option (we could have done that for the JIT too, to be sure; maybe we should still do that, but separate topic), the above is too pessimistic. You can see this from the context: if the function's body (pn) is not a TOK_UPVARS node, then the function can be a null closure even in a world without JSOP_*UPVAR or flat closures. /be
Attachment #380075 - Flags: review?(igor) → review-
From bug 494235 comment 101 it seems we'll want this bug patched too -- but the option to disable upvar optimizations should be made carefully. Having Chromebug (what a confusing name) use that option always will produce spurious performance bug reports, I predict. It would be better if we had a "if you get an optimized closure leak exception, then disable upvars" protocol. Is that something we can foist on debugger users? /be
Flags: blocking1.9.1- → blocking1.9.1?
(In reply to comment #50) > It would be better if we had a "if you get an optimized closure leak exception, > then disable upvars" protocol. Is that something we can foist on debugger > users? I'm not sure this is something users would want or need to control themselves. Gracefully disabling upvars sounds like it might have the potential to affect performance in unpredictable ways, however. A pref to turn them on or off might suffice.
Talked this over with Brendan on why he nominated. This is an edge case that may be partially mitigated by by 494235 (Brendan, please correct me if I'm wrong), resulting in even fewer problems (hopefully). Still, we don't have enough of an argument to say we'd hold the release for this. -'ing.
Flags: blocking1.9.1? → blocking1.9.1-
We'll get this in 3.5.1, I think. Damon talked with Brendan and Andreas and that seemed to be the consensus.
Flags: wanted1.9.1.x+
acceptable.
This didn't make 3.5.1. Do we still need this in a dot release? Andreas: Are you still working on this?
Flags: wanted1.9.1.x+
andreas, ping? Should this be moved to 3.6? Wontfix?
After all the recent scope work, all debugger access to the scope goes through DebugScopeProxy which hides all the scope optimizations.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: