-
Notifications
You must be signed in to change notification settings - Fork 52
Add VM::setDebuggerTrapCallback for runtime debugger activation #287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7436,7 +7436,21 @@ | |
|
|
||
| void ByteCodeParser::handleCheckTraps() | ||
| { | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| if (Options::usePollingTraps() || m_graph.m_plan.isUnlinked()) { | ||
| addToGraph(CheckTraps); | ||
| // With polling traps the debugger trap callback (VMTraps::handleDebuggerBreak) runs | ||
| // arbitrary JS from inside this frame and jettisons the code blocks on the stack before | ||
| // returning; this is where the frame then leaves, instead of carrying on with whatever | ||
| // it had hoisted above the loop. | ||
| if (!m_graph.m_plan.isUnlinked()) | ||
| addToGraph(InvalidationPoint); | ||
|
Check failure on line 7447 in Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
|
||
|
Comment on lines
+7441
to
+7447
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 The Extended reasoning...What the bug isUnder addToGraph(CheckTraps);
// ...
if (!m_graph.m_plan.isUnlinked())
addToGraph(InvalidationPoint);
The specific code path
Step-by-step proofConsider a Windows debug build (or any debug build run with
The same happens at every Why nothing prevents itThe upstream ( Impact
FixInsert an addToGraph(CheckTraps);
if (!m_graph.m_plan.isUnlinked()) {
// CheckTraps wrote InternalState; re-executing the trap poll on OSR exit is idempotent.
m_exitOK = true;
addToGraph(ExitOK);
addToGraph(InvalidationPoint);
}(or equivalently |
||
| return; | ||
| } | ||
| addToGraph(InvalidationPoint); | ||
| #else | ||
| addToGraph((Options::usePollingTraps() || m_graph.m_plan.isUnlinked()) ? CheckTraps : InvalidationPoint); | ||
| #endif | ||
| } | ||
|
|
||
| void ByteCodeParser::emitPutById( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1103,6 +1103,30 @@ class VM : public ThreadSafeRefCountedWithSuppressingSaferCPPChecking<VM> { | |
| JS_EXPORT_PRIVATE bool hasExceptionsAfterHandlingTraps(); | ||
|
|
||
| CONCURRENT_SAFE void notifyNeedDebuggerBreak() { traps().fireTrap(VMTraps::NeedDebuggerBreak); } | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| // Lets an embedder service NeedDebuggerBreak itself, e.g. to attach a debugger to a program | ||
| // that is already running and enter Debugger::breakProgram() from code that was compiled | ||
| // without op_debug sites. VMTraps::handleDebuggerBreak() calls it on the owning thread with | ||
| // the API lock held, with no exception pending, and it must return the same way; it may | ||
| // run JS and pause. It is never nested: a NeedDebuggerBreak fired while it runs is taken | ||
| // again once it returns. Termination requested meanwhile is deferred and re-fired after it. | ||
| // | ||
| // Where it runs: op_check_traps (loop headers, every tier), or wherever the embedder calls | ||
| // traps().handleTrapsIfNeeded(VMTraps::NeedDebuggerBreak) itself. The LLInt and IPInt | ||
| // function prologues deliberately do not service this bit, because they run before the | ||
| // callee's frame is initialised. Nothing else retires the bit either: an embedder that | ||
| // keeps the API lock while idle has to make that call from its idle loop, otherwise the | ||
| // signal sender keeps interrupting the thread until some loop happens to run. | ||
| // | ||
| // What it buys: pausing and evaluating in code that was already running. Breakpoints, | ||
| // `debugger` statements and stepping only take effect in code compiled after the debugger | ||
| // attached (CodeGenerationMode::Debugger), i.e. once the currently running code has been | ||
| // re-entered after the VM went idle; Debugger degrades steps out of such frames to a pause | ||
| // at the next opportunity. | ||
| using DebuggerTrapCallback = void (*)(VM&); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should fix (doc): say what this does and doesn't buy. emitDebugHook is gated on CodeGenerationMode::Debugger (BytecodeGenerator.cpp:4152-4155), the only recompile is whenIdle (VM.cpp:1041-1050, RELEASE_ASSERT(!vm.entryScope) in Heap), and attach doesn't recompile. So for code that was already running at activation you get pause and evaluate, but not breakpoints,
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: the comment on the setter now says it buys pause and evaluate in running code, and that breakpoints, |
||
| CONCURRENT_SAFE void setDebuggerTrapCallback(DebuggerTrapCallback cb) { m_debuggerTrapCallback.store(cb, std::memory_order_release); } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should fix (doc + consumer): nothing retires NeedDebuggerBreak for an embedder that is idle while holding the API lock. handleTraps is only reached from bytecode poll sites, C++ VM entry masks the bit out (NonDebuggerAsyncEvents, VM.cpp:1078-1082), and there is no other clearTrap(NeedDebuggerBreak). bun's idle path uses its own ACTIVATION_REQUESTED flag and never touches vm.traps(), and connect/disconnect re-fire the trap after activation, so on a mostly idle server (the headline Worth a line here saying an embedder that idles with the lock held must call vm.traps().handleTrapsIfNeeded(VMTraps::NeedDebuggerBreak) from its idle loop, and then bun can replace its flag re-check with exactly that so idle and busy activation share one path.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: documented on the setter, and bun's idle path becomes |
||
| DebuggerTrapCallback debuggerTrapCallback() const { return m_debuggerTrapCallback.load(std::memory_order_acquire); } | ||
| #endif | ||
| CONCURRENT_SAFE void notifyNeedShellTimeoutCheck() { traps().fireTrap(VMTraps::NeedShellTimeoutCheck); } | ||
| CONCURRENT_SAFE void notifyNeedTermination() { traps().fireTrap(VMTraps::NeedTermination); } | ||
| CONCURRENT_SAFE void notifyNeedWatchdogCheck() { traps().fireTrap(VMTraps::NeedWatchdogCheck); } | ||
|
|
@@ -1360,6 +1384,9 @@ class VM : public ThreadSafeRefCountedWithSuppressingSaferCPPChecking<VM> { | |
| const Ref<Waiter> m_syncWaiter; | ||
|
|
||
| std::atomic<int64_t> m_numberOfActiveJITPlans { 0 }; | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| std::atomic<DebuggerTrapCallback> m_debuggerTrapCallback { nullptr }; | ||
| #endif | ||
|
|
||
| Vector<Function<void()>> m_didPopListeners; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
currentCallFrameCanCompleteStep()inspectsm_currentCallFrame, butstepOutOfFunction()armsm_pauseOnCallFrameat the caller — so when paused in a freshly-compiled callee H (has debug opcodes) whose caller F was compiled pre-attach (no debug opcodes), the guard passes and step-out targets F, which has noop_debughooks to complete it. The step silently acts as continue,m_pauseOnCallFramedangles once F returns, andm_steppingModestays enabled.returnEvent()'s step-over→step-out retargeting (m_pauseOnCallFrame = callerFrame) has the same gap. Consider also checking the target frame'swasCompiledWithDebuggingOpcodes()and degrading tom_pauseAtNextOpportunitywhen it lacks them.Extended reasoning...
What the guard checks vs. what it needs to check
currentCallFrameCanCompleteStep()(Debugger.cpp:993-998) tests whetherm_currentCallFrame->codeBlock()->wasCompiledWithDebuggingOpcodes()is true. That is the correct question forstepNextExpression()andstepOverStatement(), which setm_pauseOnCallFrame = m_currentCallFrame— the frame being checked is the frame being targeted. ButstepOutOfFunction()(Debugger.cpp:1057) setsm_pauseOnCallFrame = m_currentCallFrame->callerFrame(topEntryFrame): the target is one frame up. The guard says nothing about whether that caller can complete a step.Why the mixed-opcode stack is reachable
This PR exists precisely to enable it. When the trap callback runs
attach()→setBreakpointsActivated(true),hasInteractiveDebugger()flips immediately, soScriptExecutable::defaultCodeGenerationMode()starts addingCodeGenerationMode::Debuggerto any function compiled after attach. MeanwhileVM::deleteAllCode()defers viawhenIdle, so already-linked functions on the stack keep their non-debug bytecode until the VM goes idle. During that window the stack is mixed: pre-attach frames have noop_debug, freshly-compiled callees do.Step-by-step trace
breakProgram()pauses in F.stepIntoStatement()setsm_pauseAtNextOpportunity = true. F resumes; F has noop_debug, so nothing fires until F calls a function H that is freshly compiled (with debug opcodes). H'scallEvent/atStatementfires →pauseIfNeededseesm_pauseAtNextOpportunity→ pause in H.m_currentCallFrame = H.currentCallFrameCanCompleteStep()reads H's code block →wasCompiledWithDebuggingOpcodes() == true→ returns true. Guard bypassed.stepOutOfFunction()setsm_pauseOnCallFrame = H->callerFrame() = F,m_pauseOnStepOut = true.returnEvent(H)fires (H hasop_debug). FirstupdateCallFrame(H, AttemptPause)→pauseIfNeededseesm_pauseOnCallFrame(F) != m_currentCallFrame(H)andm_pauseAtNextOpportunity == false→ no pause. Thenif (m_currentCallFrame == m_pauseOnCallFrame)isH == F→ false, so no retargeting. SecondupdateCallFrame(F, NoPause)setsm_currentCallFrame = F.op_debug, soatStatement/atExpression/returnEventnever fire on F.pauseIfNeededis never entered withm_currentCallFrame == F == m_pauseOnCallFrame. The step-out never completes;m_pauseOnCallFramedangles once F's stack slot is reused (can address-match a later frame → spurious pause);m_steppingModestaysEnabled(blocking DFG tier-up realm-wide) untildetach().The second site:
returnEvent()'s retargetreturnEvent()(Debugger.cpp:~1459-1462) doesif (m_currentCallFrame == m_pauseOnCallFrame) { m_pauseOnCallFrame = callerFrame; m_pauseOnStepOut = true; }— treating step-over-off-the-end as step-out. If the user step-over in H instead,m_pauseOnCallFrame = H; when H returns, this branch retargetsm_pauseOnCallFrametocallerFrame = Fwithout checking whether F has debug opcodes. Same failure mode.unwindEvent()has the analogous retarget.Impact and why this is a nit
The user-visible symptom is "step-out silently acts as continue" plus stuck stepping mode and a possible later spurious pause — a UX degradation in a specific interaction sequence, not a crash or data loss. The PR already documents on
setDebuggerTrapCallbackthat stepping in trap-attached code is degraded and only works fully after re-entry post-idle, so user expectations are set. Thedetach()cleanup this PR adds (Debugger.cpp:242-257) resetsm_pauseOnCallFrame/m_steppingModeon last-frontend disconnect, bounding the stale-state lifetime. The reviewer classified the parent issue as "should fix", not blocking, and the guard closes the primary case (stepping from a non-debug frame); this is the transitive case (stepping out into one).Suggested fix
In
stepOutOfFunction(), after computing the caller, check it too:And in
returnEvent()/unwindEvent(), when retargetingm_pauseOnCallFrame = callerFrame, apply the same check oncallerFrameand setm_pauseAtNextOpportunity = trueinstead when it lacks debug opcodes.