615277 - JM: Support break-on-next functionality

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Firebug wants to be able to break on the next script executed, which previously used the interruptHook functionality that is not supported by JM. That fired interrupts globally for every JSOp executed, so whatever happens next would be caught. This is not such a good idea with JM since it requires recompiling the world, probably repeatedly.

This bug is for enabling the break-on-next functionality. It probably means making sure topLevelHook works, as per https://bugzilla.mozilla.org/show_bug.cgi?id=609663#c17

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Enable executeHook for JM

Sure enough, topLevelHook was broken in JM (note: topLevelHook maps to debugHooks->executeHook). I'm pushing a patch to fix it through try; I'll add a link here when the builds are available for people to try.

Can anyone confirm whether topLevelHook is sufficient for break-on-next? (You can test by disabling JM on a current build.)

Comment 2

[John J. Barton]

Can you explain what topLevelHook does? I'm only guessing based on its name, but topLevel could be lots of things.

Comment 3

[Steve Fink [:sfink] [:s:]]

topLevelHook (which becomes executeHook within js/src) is very similar to functionHook (which becomes callHook within js/src); one or the other is called whenever any script starts to run, and again when the script completes its execution. "Script" here is a function or any other chunk of javascript text; it should include all the weird cases.

I wish I could give an exact description of when topLevelHook vs functionHook is called, but I don't know the exact semantics either. I can tell you that a script invocation will be considered "top level" if the corresponding stack frame contains either of the (internal) flags JSFRAME_GLOBAL or JSFRAME_EVAL, corresponding to the definition of isExecuteFrame() in jsinterp.h. (These flags are not visible via JSD.) But I don't know exactly what those flags mean.

By looking at the callers of Execute(), it seems like anything that isn't just a JS function call invocation will trigger the topLevelHook, so it should catch all of the weird cases.

I've posted a question at http://infomonkey.cdleary.com/questions/104/what-are-the-exact-semantics-of-jsframe_eval-and-jsframe_global for clarification.

I'm not sure what the intended semantics of break-on-next are, but it seems like you'd want to break on anything that topLevelHook catches, plus possibly other things that are reached from the current script that you're stopped within -- exception handlers? evals? Or are none of those wanted? (Is it possible that topLevelHook's semantics exactly match break-on-next?

Comment 4

[Steve Fink [:sfink] [:s:]]

Builds are temporarily available at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-35e112eaedbf/ if you'd like to take them for a spin to see how well topLevelHook() works for you. I confess I haven't tried it yet myself. And it's still probably easier to just turn off JM rather than try one of these builds; the semantics should be the same. (The only change in these builds should be that methodJIT-compiled scripts will call topLevelHook properly.)

Comment 5

[Steve Fink [:sfink] [:s:]]

Ok, now I know what the flags mean, thanks to Luke. My description above of what topLevelHook means is accurate but... strange.

You can break JS execution down into four categories, defined by two orthogonal considerations: within a function or not, and triggered by eval() or not.

<script>
  nonfunction_noneval++;
  eval("nonfunction_eval++");
  function f() {
    function_noneval++;
    eval("function_eval++");
  }
  f();
</script>

functionHook is used only for execution within a function, not triggered by eval() (so "function_noneval", above.) topLevelHook is used for everything else. If you executed the above code, you would get a functionHook callback for the line containing 'function_noneval++', and a topLevelHook callback for each of the lines containing the other increment commands. (You would also get an ending callback for all four of those, but they'd be on the final line of whatever they're executing.) The 'type' parameter passed to the callback enables you to distinguish start vs stop as well as functionHook vs topLevelHook.

So "top level" in this case apparently doesn't mean "outside of any function" (though there's an internal flag that means exactly that). It seems to mean "not a normal function entry/exit".

The overall toplevel can be detected by a null frame.callingFrame. I don't see any direct way of detecting whether a frame is from an eval(). Infer it from type==TYPE_TOPLEVEL_START and PC==0 and non-null frame.callingFrame, I guess?

Comment 6

[John J. Barton]

(In reply to comment #5)
Excellent progress! So far your theory is:
 <script>
   nonfunction_noneval++;       // type==TYPE_TOPLEVEL_START frame.callingFrame null
   eval("nonfunction_eval++");  // type==TYPE_TOPLEVEL_START frame.callingFrame  
   function f() {
     function_noneval++; // type==TYPE_FUNCTION_CALL frame.callingFrame 
     eval("function_eval++"); // type==TYPE_TOPLEVEL_START frame.callingFrame  
   }
   f();
 </script>

However I think there are more cases:
1) browser generated event scripts
   script.functionName null, frame.callingFrame == null, TOP_LEVEL_START??
   (onScriptCreated has calling stack frames)
2) XPCOM components
   script.functionName null, frame.callingFrame ? TOP_LEVEL_START??
3) XUL scripts
   script.fileName .xul or .xml, frame.callingFrame? TOP_LEVEL_START??
4) new Function()
   unknown, I could not figure it out.

Any idea if topLevelHook would be faster than taking a breakpoint at PC=0 ?
We currently use PC=0 with frame.callingFrame not null to detect eval. Based on above, we may be able to use topLevelHook with frame.callingFrame not null for the same purpose. (This does not relate to break-on-next, it would use the frame.callingFrame null path).

Comment 7

[Steve Fink [:sfink] [:s:]]

Actually, I tried it out, and it doesn't work quite the way I expected. I can't get it to distinguish global evals from function evals. PC==0 for any eval, it seems.

So I have:

  if (type == Ci.jsdICallHook.TYPE_TOPLEVEL_START) {
      if (frame.callingFrame === null)
          numGlobalNonevals++;
      else
          numEvals++;
  } else if (type == Ci.jsdICallHook.TYPE_FUNCTION_CALL) {
      numFunctionNonevals++;
  }

I'll look into the other cases. I think you're right about #1-#3 all being TYPE_TOPLEVEL_START. Not so sure about the rest of it. 

Relative speeds depend on whether you're running in the interpreter or the method JIT, but it should be safe to say that topLevelHook should be at least as fast in all cases. Thinking about it, though, I doubt the speed difference is large. There might be some indirect fallout from breakpoints screwing up optimizations, but at the beginning of a script it really shouldn't matter.

Comment 8

[Steve Fink [:sfink] [:s:]]

Empirically, it looks like the two eval cases can be distinguished via frame.callee:

  if (type == Ci.jsdICallHook.TYPE_TOPLEVEL_START) {
      if (frame.callingFrame === null)
          numGlobalNonevals++;
      else if (frame.callee === null)
          numGlobalEvals++;
      else
          numFunctionEvals++;
  } else if (type == Ci.jsdICallHook.TYPE_FUNCTION_CALL) {
      numFunctionNonevals++;
  }

This seems to result from this code in jsdbgapi.cpp's JS_GetFrameCallObject:

    if (!fp->isFunctionFrame())
        return NULL;

which is great, because if we ignore JSFRAME_DUMMY (which is safe to do here), this finally gives a (somewhat indirect) way to access the main frame type flag that distinguishes between function frames and global frames. In fact, it might make more sense to write the above as:

  // Only pay attention to start callhooks
  if (type != Ci.jsdICallHook.TYPE_TOPLEVEL_START &&
      type != Ci.jsdICallHook.TYPE_FUNCTION_CALL)
  {
      return;
  }

  if (frame.callee === null) {
    // Global frame
    if (frame.callingFrame === null)
        numGlobalNonevals++;
    else
        numGlobalEvals++;
  } else {
    // Function frame
    if (type == Ci.jsdICallHook.TYPE_TOPLEVEL_START)
        numFunctionEvals++;
    else
        numFunctionNonevals++;
  }

(untested)

Comment 9

[John J. Barton]

I don't think we need to distinguish the two eval() cases, I don't think they require different processing.

Comment 10

[John J. Barton]

I played around with topLevelHook a bit but it looks like browser generated event scripts, the most important case, do not trigger the hook.

e.g. if you follow the instructions on 
http://getfirebug.com/tests/content/script/singleStepping/index.html
in step 2 (should say "Click ClickMe") does not trigger the hook.

Comment 11

[Steve Fink [:sfink] [:s:]]

Browser-generated event scripts appear to get compiled into functions and then called as functions. So instead of getting a topLevelHook for the onclick="clicker()" script, you'll get a functionHook for a function named "onclick". Unfortunately, it appears to have a bogus line number (1).

Comment 12

[John J. Barton]

Yes, tho the complication isn't the line number, I already deal with that. It's the combination of the global functionHook (all function calls come in, so they have to be filtered) and the dynamic compile of the generated function (so I have to figure out how to update the filters after the compile). The functionHook for the onclick hits just before my BP=0 for dynamic compilation hits that would tell me that the onclick is one we want to trap back at the functionHook....

Comment 13

[John J. Barton]

OK at R8724 on branches/firebug1.7 I rewrote the break-on-next using the information from this bug report. Our tests pass on FF3.6, but we have low coverage for this issue. Nevertheless I believe we can do everything related to break-on-next without the frameless interrupt handler.

I tried to test on a nightly build of FF4.0, but I got inconsistent results. On my first try it did not even look like jsd was alive. When I restarted to double check, everything worked.  However I don't think this is related to the current bug.

Comment 14

[Steve Fink [:sfink] [:s:]]

If this WORKSFORYOU, you got lucky. :-) JM requires the patch in this bug to call the topLevelHook in some cases. Reopening.

Comment 15

[John J. Barton]

(In reply to comment #7)
> So I have:
> 
>   if (type == Ci.jsdICallHook.TYPE_TOPLEVEL_START) {
>       if (frame.callingFrame === null)
>           numGlobalNonevals++;

frame.callingFrame === null is not reliable. Sometimes jsd sends us frames that are part of a stack but nevertheless frame.callingFrame is not set. For example, if I debug Firebug and Firebug dispatches an event to a Web page, the event run synchronously but when the call stack gets back to the top of the part that runs in the page, frame.callingFrame is null. The next JS operation that runs is the one after the event dispatch.  

I've not figured out yet how to distinguish a FUNCTION_CALL that starts a new execution from one that is part of another call stack but with frame.callingFrame === null.

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 495987 [details] [diff] [review]
Enable executeHook for JM

JM needs to issue topLevelHook JSD callbacks like the interpreter does. topLevelHook translates to cx->debugHooks->executeHook.

This patch makes js::mjit::stubs::EnterScript and ::LeaveScript invoke the appropriate hook, which turns out to be ->callHook for regular function calls and ->executeHook for everything else. I also changed where the interpreter invokes the hooks for consistency; it now does the same calls in ScriptProlog and ScriptEpilogue, rather than calling ->executeHook from Execute() as it used to. I'm not 100% sure whether that change was needed for correctness or not -- when doing something related, I needed to match everything up exactly because you could leave a JM method through the tracer or something. I don't know whether that applies here, but it's definitely safer this way.

Comment 17

[David Mandelin [:dmandelin]]

Comment on attachment 495987 [details] [diff] [review]
Enable executeHook for JM

r+. A few minor comments.

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

I like the control flow cleanup here.

>+    if (fp->isExecuteFrame()) {
>+        JSInterpreterHook hook = cx->debugHooks->executeHook;
>+        if (JS_UNLIKELY(hook != NULL))

I do think the above 2 lines read a little nicer as:

          if (JSInterpreterHook hook = cx->debugHooks->executeHook) {

As implied by that suggestion, I really doubt the JS_UNLIKELYs there did any good on modern processors, so I recommend cutting them out.

>+            fp->setHookData(hook(cx, fp, JS_TRUE, 0, cx->debugHooks->executeHookData));
>+    } else {
>+        JSInterpreterHook hook = cx->debugHooks->callHook;
>+        if (JS_UNLIKELY(hook != NULL))
>+            fp->setHookData(hook(cx, fp, JS_TRUE, 0, cx->debugHooks->callHookData));
>         Probes::enterJSFun(cx, fp->maybeFun(), fp->maybeScript());
>+    }

Comment 18

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/tracemonkey/rev/ac952c471f59