605330 - Mismatched enters/exits in --enable-trace-jscalls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

This is rev2 of bug 602067, which was done before bug 603044 and has some clutter.

I now have something working, but I don't fully understand why: it works and makes sense with just TM and just JM. But when I run with JM+TM in non-debug mode, I get too many enters. Or I did, until I forced a call to  stubCall(stubs::EnterScript) and ::LeaveScript even when debugMode is off.

Now it works in all of my test cases, but I don't understand why the above made a difference -- shouldn't the matchedness with both ::EnterScript and ::LeaveScript on be the same as with them both off?

It seems like TM can do a Probes::enterJSFun() then somehow skip over the mjit prologue, picking up a matching Probes::exitJSFun() from stubs::LeaveScript. But I don't know how that's possible. Unless it traces up to a JSOP_CALL, then stops tracing but jumps into the mjit in a fast path skipping the prologue? I didn't see how that was possible.

In short, it seems to work, but I don't understand why.

Comment 1

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

Attachment: Part 1: Move jscall probes to callee-based, mostly, sorta

Invoke Probes::exitJSFun() when doing a JSOP_RETURN, JSOP_STOP, or JSOP_RETRVAL only.

Invoke Probes::enterJSFun() from interpretedFunctionCall for TM, which seems ugly but I see no other way to detect in tracejitted code when you are beginning to execute a method call. (Previously, I could use JSOP_BEGIN.)

Switch the TM function call tracking over to Probes:: stuff now that the other probe handlers are no longer inspecting the stack.

Will discuss with JS folks before putting up for review.

Comment 2

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

Attachment: Part 2: add debugging info to enterJSFun/exitJSFun

Pass through some extra information to aid in debugging mismatches. Attaching here because my 3rd patch depends on its modified API.

This also throws out some unused cruft in the dtrace handlers for function call enter/exit.

Comment 3

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

Attachment: Part 3: force probe calls for JM as well

This forces JM to call Probes::enterJSFun and ::exitJSFun as well, which mysteriously balances enter/exits with JM+TM.

This also adds a Probes:: API entry Probes::callTrackingActive() and uses it for all sites. If no profilers are configure-time enabled, this will be an inline "return false". This has an additional behavior change (above getting JM+TM to work) in that you can now use the TM and JM probes for function call tracking without --enable-trace-jscalls. (You'll need to request at least one of the other handlers, eg --enable-ETW or --enable-dtrace, and also have them runtime-enabled when the JIT actually compiles the code.)

Comment 5

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

Attachment: Part 4: Make all jscall tracking dependent on debugMode

I couldn't let it rest without understanding why forcing the JM call tracking would balance enters/exits.

There are ways to cause an JM function enter to get paired up with a TM function exit, and I think vice versa as well. Compiler.cpp has jumpAndTrace that could switch to tjit code and do a return. Exceptions can also muck with things (I didn't keep notes on how.) But in short, the conditions for calling Probes::enter/exitJSFun need to be identical on all paths.

Attached is a patch that makes them all conditional on debugMode. I don't actually want to do this; the probes should be usable when not in debug mode. So I'll be obsoleting most or all of these attached patches before putting up for review. I just want to record one working state in the history.

Comment 6

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

Attachment: Part 1: Tweak jscalls API and normalize conditions for invoking

Add Probes::callTrackingActive() to ask whether JS call tracking is desired. Change all probe sites to test this. That includes JM -- it will do call tracking even when not in debugMode (but it still won't do the debug hooks).

Also mixed in are some minor API changes to the probes API. I can pull them out into a separate patch if you want; I wrapped them together because they're both API changes. You could also just let your eyes glaze over until you reach Compiler.cpp in the patch. (Only the Compiler.cpp and InvokeHelpers.cpp changes are needed to match up enters with exits, though they use the added API entry in jsprobes.h.)

There are no in-tree users of the functionCallback API yet, and the JSBool -> int change is binary compatible FWIW.

Comment 7

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

Attachment: Part 2: remove extra probes for script execution

As dvander pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=603044#c22 there is an extra pair of probes for script execution. This removes them.

Comment 8

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

Attachment: Part 3: convert TM's functionProbe to call the Probes:: probes

Previously, TM needed its own path for invoking the function callbacks because the standard Probes:: ones inspected arguments, the stack, etc. I removed all that functionality so now they are safe.

Comment 9

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

Attachment: Rollup patch for fixing jscall tracking and merging code paths

dvander: sorry, needed to unbitrot the patches before applying, and since I need to mark checkin-needed, I didn't want to just r=myself.

No changes of note, this is just a merge of the three that applies to current TM (e99b3aca8eda) and compiles.

Comment 10

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

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