Closed
Bug 570650
Opened 15 years ago
Closed 15 years ago
JM: Implement trap part of debug API for methodjit
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: adrake, Assigned: adrake)
References
Details
Attachments
(1 file, 8 obsolete files)
48.13 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
SetTrap and related functions need to be implemented to allow debugging within JITed code.
Assignee | ||
Updated•15 years ago
|
Assignee: general → adrake
Blocks: JaegerDebug
Assignee | ||
Updated•15 years ago
|
Summary: [JAEGER] Implement trap part of debug API for methodjit → JM: Implement trap part of debug API for methodjit
Assignee | ||
Comment 1•15 years ago
|
||
Works on x86 OSX, Win32, and Linux. Needs to have 64-bit support added (currently #error s), and autoconf disables it for ARM if we ever get around to fixing that build.
I'd appreciate some feedback on races in particular -- I don't think there are any but I could have missed something.
Attachment #452141 -
Flags: review?(dvander)
Comment on attachment 452141 [details] [diff] [review]
Patch v0
This is really awesome, minimally invasive and the single-stepping solution is a nice surprise.
I'm stamping this in terms of JS changes; it might be good for someone else to review the OS-level stuff. I don't know it :)
njn, are you game?
Attachment #452141 -
Flags: review?(nnethercote)
Attachment #452141 -
Flags: review?(dvander)
Attachment #452141 -
Flags: review+
![]() |
||
Comment 3•15 years ago
|
||
Comment on attachment 452141 [details] [diff] [review]
Patch v0
The OS stuff (CONTEXT_FRAME et al) looks plausible, but I only have a passing understanding of that stuff. If it works, that's a good sign :)
Attachment #452141 -
Flags: review?(nnethercote) → review+
Comment 4•15 years ago
|
||
Comment on attachment 452141 [details] [diff] [review]
Patch v0
>+# else
>+# error Unsupported CPU!
>+# endif
>+
>+JS_STATIC_ASSERT(sizeof(sTrapPatch) <= sizeof(jsbytecode));
Why is this needed? It's vacuous given the uint8 manifest for sTrapPatch -- no smaller type possible.
Nit: we don't use sFoo for statics. Some inconsistency, but TrapPatch or trap_patch would be fine.
>+typedef struct JSPatch {
Lose the typedef, we're C++ not C now.
>+ JSCList links;
So you can declare 'struct JSPatch : public JSCList' instead and upcast for free.
>+ JSScript *script;
>+ void *address;
>+ jsbytecode savedCode[sizeof(sTrapPatch)];
>+ JSBool isApplied;
>+ jsint nReferences;
Jim Blandy is redoing debugging, but if this is meant to match existing code, it should use canonical type and member name: jsrefcount nrefs.
> typedef struct JSTrap {
> JSCList links;
Old C-era code, to be overhauled. We do big cleanups separately but sometimes you can do a small one in the course of fixing another bug. Feel free!
> JSScript *script;
> jsbytecode *pc;
> JSOp op;
>+#ifdef JS_METHODJIT_DEBUG
>+ JSPatch *patch;
>+ JSBool isPending;
>+#endif
> JSTrapHandler handler;
> jsval closure;
> } JSTrap;
>@@ -103,6 +132,77 @@
> return NULL;
> }
>
>+#ifdef JS_METHODJIT_DEBUG
>+static JSPatch *
>+FindPatch(JSRuntime *rt, void *native)
>+{
>+ JSPatch *patch;
>+ for (patch = (JSPatch *)rt->patchList.next;
>+ &patch->links != &rt->patchList;
>+ patch = (JSPatch *)patch->links.next) {
>+ if (patch->address == native) {
>+ return patch;
>+ }
Don't over-brace single line consequents unless there's a multi-line condition and/or else clause.
>+static JSPatch *
>+AcquireOrCreatePatch(JSContext *cx, JSScript *script, jsbytecode *pc)
>+{
>+ JS_ASSERT(script->ncode && script->nmap);
>+ JS_ASSERT(script->nmap[pc - script->code]);
>+
>+ void *native = script->nmap[pc - script->code];
>+ JSPatch *result = FindPatch(cx->runtime, native);
>+
>+ if (result) {
>+ result->nReferences++;
>+ } else {
>+ result = (JSPatch *)cx->malloc(sizeof(JSPatch));
>+ if (!result)
>+ return NULL;
>+ result->script = script;
>+ result->address = native;
>+ memcpy(result->savedCode, native, sizeof(sTrapPatch));
>+ result->isApplied = JS_FALSE;
>+ result->nReferences = 0;
>+
>+ JS_APPEND_LINK(&result->links, &cx->runtime->patchList);
>+
>+ ApplyPatch(result);
>+ }
>+ return result;
This leaves the newborn JSPatch with nReferences == 0. Instead of if (result) { ... } else { /* newborn case */ }, use if (!result) { /* newborn case */ } and always increment nrefs before successful (non-null) return result.
>+}
>+
>+static void
>+ReleasePatch(JSPatch *patch)
>+{
>+ patch->nReferences--;
>+ if (!patch->nReferences && patch->isApplied) {
>+ RemovePatch(patch);
>+ }
Don't over-brace.
Prevailing style is ok with 'if (--patch->nrefs == 0 && patch->isApplied)' for the combined -- and if (condition) parts.
>+js_ApplyPendingTraps(JSContext *cx, JSScript *script)
>+{
>+ JSRuntime *rt;
>+ JSTrap *trap;
>+
>+ rt = cx->runtime;
No locking here, but it would have to cover an overlarge critical section if you called AcquireOrCreatePatch and had to create. See JSTrap and JSWatchPoint code for how to lock, although again this may all get much simpler soon. For now, thread safety seems good to preserve, and not hard.
>+#if defined _WIN32
>+JS_FRIEND_API(LONG)
>+js_TrapHandler(DWORD exception, EXCEPTION_POINTERS *info)
>+{
>+ if (exception == EXCEPTION_BREAKPOINT || exception == EXCEPTION_SINGLE_STEP) {
...
>+ if (exception == EXCEPTION_BREAKPOINT) {
...
>+ for (JSTrap *trap = (JSTrap *)rt->trapList.next;
>+ &trap->links != &rt->trapList;
>+ trap = (JSTrap *)trap->links.next) {
>+ if (trap->patch == patch) {
>+ JS_ASSERT(trap->script == script);
>+ Value rval;
>+ switch (trap->handler(frame->cx, script, trap->pc,
>+ Jsvalify(&rval), trap->closure)) {
>+ case JSTRAP_ERROR:
Nit: half-indent (two spaces) switch case/default and goto labels.
...
>+ return EXECEPTION_EXECUTE_HANDLER;
>+ }
>+ }
>+
>+ return EXCEPTION_CONTINUE_EXECUTION;
>+ } else if (exception == EXCEPTION_SINGLE_STEP) {
No else after return, it's a non-sequitur that overindents the SINGLE_STEP code and creates the appearance of a missing return.
>+ JS_ASSERT(rt->recentPatch);
>+ JSPatch *patch = FindPatch(rt, rt->recentPatch);
>+ JS_ASSERT(patch);
>+ ApplyPatch(patch);
>+ rt->recentPatch = NULL;
>+
>+ return EXCEPTION_CONTINUE_EXECUTION;
>+ }
>+ } else
>+ return EXCEPTION_CONTINUE_SEARCH;
Ditto. And again for the #else'd JS_TrapHandler.
/be
Attachment #452141 -
Flags: review+ → review?(nnethercote)
Updated•15 years ago
|
Attachment #452141 -
Flags: review?(nnethercote)
Attachment #452141 -
Flags: review?(brendan)
Attachment #452141 -
Flags: review+
Assignee | ||
Comment 5•15 years ago
|
||
(In reply to comment #4)
> Why is this needed? It's vacuous given the uint8 manifest for sTrapPatch -- no
> smaller type possible.
Whoops! That assert was wrong both semantically and in code, and I thought I removed it.
> So you can declare 'struct JSPatch : public JSCList' instead and upcast for
> free.
I like it! That cleans up a lot of nastiness.
> Jim Blandy is redoing debugging, but if this is meant to match existing code,
> it should use canonical type and member name: jsrefcount nrefs.
A lot of that will be going away when we start to get his work into moo, hopefully. I will still update the name for consistency in the mean time.
> Old C-era code, to be overhauled. We do big cleanups separately but sometimes
> you can do a small one in the course of fixing another bug. Feel free!
Might as well! I'm touching it already.
> This leaves the newborn JSPatch with nReferences == 0. Instead of if (result) {
> ... } else { /* newborn case */ }, use if (!result) { /* newborn case */ } and
> always increment nrefs before successful (non-null) return result.
Good catch, thank you!
> No locking here, but it would have to cover an overlarge critical section if
> you called AcquireOrCreatePatch and had to create. See JSTrap and JSWatchPoint
> code for how to lock, although again this may all get much simpler soon. For
> now, thread safety seems good to preserve, and not hard.
Will do. ACK for all of your other comments, and I'll post v1 either later tonight or tomorrow morning. Thanks for the review!
Updated•15 years ago
|
Attachment #452141 -
Flags: review?(brendan)
Assignee | ||
Comment 6•15 years ago
|
||
Hey, just wanted to post an update since it's been a while. After finding a couple more issues in v0, and finally getting v1 all polished up and ready, we decided to explore an alternate method to handle traps that would make EvalInFrame easier and simpler.
Assignee | ||
Comment 7•15 years ago
|
||
Alrighty, new patch, unrelated to the old one. This one implements traps via a recompilation strategy, which allows us to have zero speed overhead with the debugger off and minimal overhead with the debugger on, and works even with a nondeterministic compiler. It also gives us EvalInFrame for free, and makes most hooks trivial.
This is the first version of the new patch. It's worked on the nastiest edge cases I can come up with, but tests for the suite are forthcoming.
Attachment #452141 -
Attachment is obsolete: true
Attachment #456136 -
Flags: review?(dvander)
Assignee | ||
Comment 8•15 years ago
|
||
Fixed bitrot. Tests still WIP.
Attachment #456136 -
Attachment is obsolete: true
Attachment #456136 -
Flags: review?(dvander)
Assignee | ||
Updated•15 years ago
|
Attachment #456935 -
Flags: review?(dvander)
Comment on attachment 456935 [details] [diff] [review]
Patch v2
>+ if (callSites[i].pc == script->code + pcOffset &&
>+ callSites[i].escapeReason == reason && callSites[i].id == id) {
>+ callSite(Escape_Call, 3, false);
What is this number? Let's get an enum in there. Ditto for other callers.
>+ if (callSites[i].stub)
>+ return (uint8*)script->nmap[-1] + masm.size() +
>+ stubcc.masm.distanceOf(callSites[i].location);
>+ else
>+ return (uint8*)script->nmap[-1] +
>+ stubcc.masm.distanceOf(callSites[i].location);
>+ }
>+ }
Braces needed around multi-line if blocks.
>+mjit::Compiler::escapeHelper(uint32 escapeReason, bool stub)
It would make my day if this used the enum you defined elsewhere :)
>-#ifdef DEBUG
>- script->jitLength = 0;
>-#endif
>+ script->inlineLength = 0;
>+ script->outOfLineLength = 0;
Please carefully test on Mac (I can help, if needed) that these do not regress performance. I spent a day a while back finding out that increasing JSScript by anything resulted in 1000ms v8 loss due to some mysterious cache effects on OS X only. I haven't increased the size since.
>+Recompiler::PatchableAddress
>+Recompiler::findPatch(void **location, uint32 escapeReason)
>+{
>+ for (uint32 i = 0; i < script->callSites[-1].codeOffset; i++) {
Can you union this member variable to a more appropriate name for this edge case? :)
>+/*
>+ * The strategy for this goes as follows:
Nice, thanks for detailed comment.
>+ JS_ASSERT(script->ncode && script->ncode != JS_UNJITTABLE_METHOD);
>+
>+ Vector<PatchableAddress> toPatch(cx);
All uses of toPatch.append() are fallible - must check return value and propagate failure up.
>+ for (CallStackIterator cs(cx); !cs.done(); ++cs) {
>+ VMFrame *f = cs.cs()->getActiveVMFrame();
As discussed in IRC, there will be multiple VMFrames per cs in the future.
>+ toPatch.append(findPatch(machineReturn, cs.cs()->getEscapeReason()));
OOM check.
>+ if (script->isValidJitCode(f->scriptedReturn))
>+ toPatch.append(findPatch(&f->scriptedReturn, 0));
Ditto.
>+ if (script->isValidJitCode(fp.fp()->ncode))
>+ toPatch.append(findPatch(&fp.fp()->ncode, 0));
Ditto.
>+ Compiler c(cx, script, firstFrame->fun, firstFrame->scopeChain);
>+ if (c.Compile() != Compile_Okay)
>+ return false;
For error propagation, Compile_Error means you must propagate an error. The compiler can also fail "normally", i.e. unsupported opcode or runtime condition. In this case, no error should be propagated. Right now, if you got a successful compile once, you won't get a non-error abort the next time. But better safe than sorry. We've been bitten by propagation bugs in TM too many times.
>+ AutoScriptRetrapper(JSContext *cx1, JSScript *script1) :
>+ cx(cx1), script(script1), traps(cx) {};
Nit: C++ compilers will do the right thing with s/cx1/cx/ and s/script1/script/
Nit: Can we get some preceding comment as to what this class does?
>+class Recompiler {
Nit: Can we get some preceding comment as to what this class does?
>+class FrameIterator
Why did this have to move? Isn't there another FrameIterator?
>+static void
>+EscapeInterrupt(VMFrame &f, jsbytecode *pc)
|pc| unused?
>+ for (uint32 i = 0; i < script->length; i++) {
>+ if (script->nmap[i] && script->code[i] == JSOP_STOP)
>+ return script->nmap[i];
>+ }
(If you care, searching backwards here will be faster - I suspect it doesn't matter though.)
>+ switch (reason) {
>+ case Escape_Interrupt:
>+ EscapeInterrupt(f, pc);
>+ break;
>+
Nit: house style is two-space indents for case statements, like:
switch (reason) {
case Escape_Interrupt:
EscapeInterrupt(f, pc);
break;
Looks good so far, mostly nits - looking forward to next patch to down-link VMFrames.
Attachment #456935 -
Flags: review?(dvander)
Assignee | ||
Comment 10•15 years ago
|
||
(In reply to comment #9)
> What is this number? Let's get an enum in there. Ditto for other callers.
That number is actually meaningless. It just has to be unique. Adding a way
to make that automatic in the next revision.
> It would make my day if this used the enum you defined elsewhere :)
I can actually do this in the next version. I couldn't figure out a way to do
it in this version because I needed to put that type in the CallStack, and
you can't forward-declare enums to break circular dependencies.
> Please carefully test on Mac (I can help, if needed) that these do not regress
> performance. I spent a day a while back finding out that increasing JSScript by
> anything resulted in 1000ms v8 loss due to some mysterious cache effects on OS
> X only. I haven't increased the size since.
Will do. I only have access to a Core i7, which has fancy new tech to make
caching effects like that less of an issue. Is buildmonkey older than that?
> Nit: C++ compilers will do the right thing with s/cx1/cx/ and s/script1/script/
I've had compilers (I think MSVC) get angry at me for doing this. Do we care about
MSVC warnings?
> >+class FrameIterator
>
> Why did this have to move? Isn't there another FrameIterator?
There's the iterator I wrote that was committed on its own that's not useful
anymore and has been backed out, and there's FrameRegsIterator which only does
previousInContext, and then there's JS_FrameIterator which refuses to cross
call stacks.
> (If you care, searching backwards here will be faster - I suspect it doesn't
> matter though.)
I can do that. Easy enough change; seems wasteful not to.
> Nit: house style is two-space indents for case statements, like:
Whoops, bitten by emacs.
Working on the next patch now, realized there are a couple more cleanups I can do.
Comment 11•15 years ago
|
||
> >+ if (callSites[i].stub)
> >+ return (uint8*)script->nmap[-1] + masm.size() +
> >+ stubcc.masm.distanceOf(callSites[i].location);
> >+ else
> >+ return (uint8*)script->nmap[-1] +
> >+ stubcc.masm.distanceOf(callSites[i].location);
>
> Braces needed around multi-line if blocks.
No else after return, either.
/be
Assignee | ||
Comment 12•15 years ago
|
||
Fix more bitrot (argh).
Attachment #456935 -
Attachment is obsolete: true
Assignee | ||
Comment 13•15 years ago
|
||
Interdiff between patch v3 and v4, which fixes all issues mentioned above, several cleanups, and adds a test suite.
Assignee | ||
Comment 14•15 years ago
|
||
The above interdiff applied, now in pre-applied form.
Attachment #457530 -
Attachment is obsolete: true
Attachment #457532 -
Flags: review?(dvander)
Assignee | ||
Comment 15•15 years ago
|
||
I ran 10 iterations of v8 several times with and without my debug stuff, and it seems to be a consistent small perf win with the debug patch applied (?!). Below is the most modest of the comparison results:
TEST COMPARISON FROM TO DETAILS
=============================================================================
** TOTAL **: - 4168.8ms +/- 0.2% 4155.2ms +/- 0.4%
=============================================================================
v8: - 4168.8ms +/- 0.2% 4155.2ms +/- 0.4%
crypto: *1.012x as slow* 239.0ms +/- 0.7% 241.8ms +/- 0.6% significant
deltablue: 1.016x as fast 841.5ms +/- 0.9% 828.4ms +/- 0.4% significant
earley-boyer: ?? 550.7ms +/- 0.3% 553.5ms +/- 0.7% not conclusive: might be *1.005x as slow*
raytrace: - 370.6ms +/- 0.2% 363.2ms +/- 2.5%
regexp: ?? 821.6ms +/- 0.1% 823.2ms +/- 0.5% not conclusive: might be *1.002x as slow*
richards: *1.014x as slow* 596.7ms +/- 0.1% 605.0ms +/- 0.2% significant
splay: 1.012x as fast 748.7ms +/- 0.2% 740.1ms +/- 0.2% significant
Assignee | ||
Comment 16•15 years ago
|
||
Fixed bit-rot.
Attachment #457532 -
Attachment is obsolete: true
Attachment #457532 -
Flags: review?(dvander)
Assignee | ||
Comment 17•15 years ago
|
||
This version even works!
Attachment #458770 -
Attachment is obsolete: true
Attachment #458778 -
Flags: review?(dvander)
Comment on attachment 458778 [details] [diff] [review]
Patch v6
>- uint32 jitLength; /* length of JIT'd code */
>+ js::mjit::CallSite *callSites;
>+ uint32 inlineLength; /* length of inline JIT'd code */
>+ uint32 outOfLineLength; /* length of out of line JIT'd code */
Let's make sure this doesn't regress on Mac - will talk on IRC.
> * BEGIN COMPILER OPS *
> **********************/
>-
> switch (op) {
Keep this newline.
> stubcc.call(stubs::Interrupt);
>+ CALLSITE(true);
Let's call this ADD_CALLSITE.
>+mjit::Compiler::callSite(uint32 id, bool stub)
Let's call this addCallSite.
>+ AutoScriptRetrapper(JSContext *cx1, JSScript *script1) :
>+ cx(cx1), script(script1), traps(cx) {};
>+ ~AutoScriptRetrapper();
Thanks for the comment. Existing nit: s/1//, this is definitely safe and warning-free.
>+/*
>+ * This class is responsible for sanely re-JITing a script and fixing up
>+ * the world. If you ever change a JSScript, you /must/ use this to
Hrm, clarify what "change a JSScript" means, since I'm not sure :)
>+ JSC::ExecutablePool *pool = execPool->poolForSize(masm.size());
This is going to leak the pool, I think. Double check with valgrind because I might be wrong.
>+ uint8 *result = (uint8 *)pool->alloc(masm.size());
>+ JSC::ExecutableAllocator::makeWritable(result, masm.size());
>+ memcpy(result, masm.buffer(), masm.size());
>+ masm.finalize(result);
>+ JSC::ExecutableAllocator::makeExecutable(result, masm.size());
Use JSC::LinkBuffer ; LinkBuffer::finalizeCodeAddendum instead of all this manually. See PolyIC.cpp for examples; Compiler.cpp has special needs because of the OOL path.
>+ typedef bool (* TrampolineGenerator)(Assembler &masm);
No space between * and type name.
>+
>+public:
>+ TrampolineCompiler(JSC::ExecutableAllocator *pool, Trampolines *tramps) :
>+ execPool(pool), trampolines(tramps) { }
{ } on newline
r=me w/ nits.
Awesome stuff. Dunno if I said this already, but having debugging tests is great.
Attachment #458778 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 19•15 years ago
|
||
> Let's make sure this doesn't regress on Mac - will talk on IRC.
Sorry forgot to mention, above perf numbers were from Mac, if that makes a difference.
Valgrind ignores the leak because (I'm pretty sure) the allocation happens out of the current small allocation pool, and on ExecutableAllocator destruction, the pool gets unconditionally deleted. I added a way to hang on to the pools to ensure this doesn't leak in other cases.
Additionally, the nmap thing from tracer integration broke me. I implemented a workaround that records the call site unconditionally, but this is probably slower than it needs to be. This overhead will be completely gone with my EvalInFrame stuff, which adds debug mode, so this stuff can just only be recorded then. An even better solution can be done if/when debug mode is eliminated.
Assignee | ||
Comment 20•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 21•15 years ago
|
||
Re: comment 19 second paragraph:
We had a patch quite a while ago from Graydon to teach valgrind about allocations from pools via a rotate no-op sequence hokey-pokey dance you could inject via asm macrology. Not sure where it ended up.
/be
Comment 22•15 years ago
|
||
Backed out due to Mochitest crash:
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/1661c73938c5
It crashes in the first few tests, in UnsetVMFrameRegs. I'm not exactly sure what the problem is, but it looks like the activeFrame/previous list might be relying on SetVMFrameRegs/UnsetVMFrameRegs invocations being paired, which is not true in the presence of exceptions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 23•15 years ago
|
||
Small change to fix how trampolines remove the current VMFrame from the list of active VMFrames. This doesn't include ARM support.
Attachment #457531 -
Attachment is obsolete: true
Attachment #458778 -
Attachment is obsolete: true
Attachment #459982 -
Flags: review?(dvander)
![]() |
||
Updated•15 years ago
|
Attachment #459982 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 24•15 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•