Closed Bug 605330 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sfink, Assigned: sfink)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 7 obsolete files)

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.
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.
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.
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.)
Assignee: general → sphink
Attachment #484161 - Attachment description: Move jscall probes to callee-based, mostly, sorta → Part 1: Move jscall probes to callee-based, mostly, sorta
Duplicate of this bug: 602067
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.
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.
Attachment #484161 - Attachment is obsolete: true
Attachment #484163 - Attachment is obsolete: true
Attachment #484167 - Attachment is obsolete: true
Attachment #484849 - Attachment is obsolete: true
Attachment #484924 - Flags: review?(dvander)
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.
Attachment #484925 - Flags: review?(dvander)
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.
Attachment #484926 - Flags: review?(dvander)
Attachment #484924 - Flags: review?(dvander) → review+
Attachment #484925 - Flags: review?(dvander) → review+
Attachment #484926 - Flags: review?(dvander) → review+
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.
Attachment #484924 - Attachment is obsolete: true
Attachment #484925 - Attachment is obsolete: true
Attachment #484926 - Attachment is obsolete: true
Attachment #486511 - Flags: review?(dvander)
Attachment #486511 - Flags: review?(dvander) → review+
Keywords: checkin-needed
http://hg.mozilla.org/tracemonkey/rev/e8c612257ca5
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
You need to log in before you can comment on or make changes to this bug.