Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect hook cleanup and regression-test validity.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes stale debugger step hooks during stopped-state evaluation, preventing newly created coroutines from triggering unintended stops.
Changes:
- Cancel pending step hooks centrally in
runLoop. - Add Lua and DAP reproduction coverage for pause, entry, breakpoint, and stepping scenarios.
File summaries
| File | Summary | Findings |
|---|---|---|
test/repro/target.lua |
Provides the coroutine evaluation reproducer. | None |
test/repro/dap-client.js |
Automates DAP reproduction and validation. | Moderate: assert breakpoint stops and fail on stale binaries (1–3 votes). Nits: use thread locations for stop reporting (1 vote each). |
extension/script/backend/worker.lua |
Cancels pending step hooks before stop events. | Moderate: refresh hooks for stepped coroutines beyond the current host (1 vote). |
Review details
Suppressed comments (7)
extension/script/backend/worker.lua:701
- This cancels the step state globally but refreshes the hook mask only for the current debug host. With stepOver/stepOut,
stepLcan be a different coroutine; if a breakpoint or exception stops that other coroutine, the stepped coroutine keeps its old full hook even thoughstep_maskis zero, so it continues generating callbacks after continue. The cancellation path needs to refresh the stepped coroutine (and any other states carrying the step hook), not only the current host.
hookmgr.step_cancel()
test/repro/dap-client.js:434
- This branch already means that an extra
stoppedevent arrived whileevaluatewas pending, so it should fail regardless of where the second stop occurred. As written, a stop at any line other than the marker makesoktrue and exits successfully, masking the same blocked-evaluation regression when the reported top frame changes. KeepstopLineonly for diagnostics and treat every second stop as a failure.
ok = stopLine !== coroutineLine;
test/repro/dap-client.js:375
stoppedevents do not contain a source or line (the helper above documents that), so passingstop.bodytowherealways logs<unknown>in this branch. Uselocate(dap, stop.body.threadId)here so the repro output identifies where the breakpoint stop occurred.
log(`[repro] 断点在 ${where(stop.body)} (reason=${stop.body.reason})`);
test/repro/dap-client.js:396
stoppedevents do not contain a source or line (the helper above documents that), so passingstop.bodytowherealways logs<unknown>in this branch. Uselocate(dap, stop.body.threadId)here so the repro output identifies where the entry stop occurred.
log(`[repro] 入口停在 ${where(stop.body)} (reason=${stop.body.reason})`);
test/repro/dap-client.js:400
stoppedevents do not contain a source or line (the helper above documents that), so passingstop.bodytowherealways logs<unknown>in this branch. Uselocate(dap, stop.body.threadId)here so the repro output identifies where the pause stop occurred.
log(`[repro] 暂停在 ${where(stop.body)} (reason=${stop.body.reason})`);
test/repro/dap-client.js:404
stoppedevents do not contain a source or line (the helper above documents that), so passingstop.bodytowherealways logs<unknown>in this branch. Uselocate(dap, stop.body.threadId)here so the repro output identifies where the step stop occurred.
log(`[repro] 单步停在 ${where(stop.body)} (reason=${stop.body.reason})`);
test/repro/dap-client.js:122
- This check only prints a warning and then runs the repro with the potentially stale binary. That allows the required release-build precondition to be violated (the description notes that a debug build masks this regression), producing a false passing result; make the stale-binary condition terminate the run instead of continuing.
if (newer.length > 0) {
log(`[repro] 警告:${LUADEBUG_DLL} 比这些目录里的源码旧,请 luamake -mode release 重新构建:`);
for (const d of newer) {
log(' ' + d);
}
}
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+389
to
+393
| for (let i = 0; i < 10 && stop.body.reason !== 'breakpoint'; i++) { | ||
| await dap.request('stepIn', { threadId: stop.body.threadId }); | ||
| stop = await dap.waitStopped(15000); | ||
| log(`[repro] stepIn(${i + 1}) 停在 ${where(await locate(dap, stop.body.threadId))} (reason=${stop.body.reason})`); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
场景
目标 VM 主动加载调试器并监听端口(
dbg:start{address=...}),关掉 autoUpdate,由 Lua 侧在合适周期手动抛update事件:update的调用处;coroutine.create+coroutine.resume的代码(协程体在真实文件里);影响 PUC Lua(实测 5.1/5.3/5.4/5.5),LuaJIT 不受影响。
原因
CMD.stop(pause/entry)用hookmgr.step_in()装 hook:step_mask = MASKCALL | MASKRET | MASKLINE,且stepL = 0(对所有线程生效)。event.step,state == 'stopped'分支直接进runLoop,没有像用户单步那条分支那样step_cancel(),于是停止等待期间 step hook 一直挂着。debug_pcall只把当前线程的allowhook置 0(src/luadebug/rdebug_visitor.cpp),而lua_newthread新建的协程会继承创建者的hook和hookmask,它自己的allowhook是 1。full_hook走 step 分支(step_mask & LUA_MASKLINE && !stepL)→event.step,此时state仍是stopped→ 嵌套runLoop→ 又发送一次stopped事件,evaluate 一直卡到用户 continue。断点停止时
step_mask == 0,行事件走event.bp分支,不进runLoop,所以正常。修复方式
runLoop是“进入停止状态”的唯一汇聚点,在这里取消 step hook:local function runLoop(reason, level) baseL = hookmgr.gethost() + hookmgr.step_cancel() sendToMaster 'eventStop' (reason)没有选择在
event.step里补step_cancel(),因为那条路径覆盖不全:event_breakpoint(以及event.funcbp/event.instbp/runException)会在状态判断之前就return并自己调用runLoop。实测“暂停 → stepIn 踩到断点行”这条路径,只改event.step仍然能复现(node test/repro/dap-client.js stepbp)。复现步骤(含测试文件)
需要 release 构建:debug 构建里
src/luadebug/util/protected_area.h的check_recursive()(#if !defined(NDEBUG))会让停止状态下的重入rdebug.*调用直接报can't recursive,反而掩盖这个问题。本次一并提交的测试文件:
test/repro/target.lua:目标进程。加载调试器、监听端口、关闭 autoUpdate、主循环手动抛update,并提供调试控制台里要调用的make_coroutine()(创建协程并立即 resume,协程体在真实文件里)。test/repro/dap-client.js:假 VS Code,直接讲 DAP。流程是 attach → pause → 在停止帧上evaluate make_coroutine(),判断是 evaluate 先返回还是又收到一次stopped(并打印第二次停止的位置 /continue后 evaluate 才返回)。脚本会自动把extension/script同步到publish/script(等同copy_extension),并检查luadebug.dll是否比src/luadebug旧。副作用分析
step_cancel内部有if (step_mask != mask)守卫,没挂 step 时直接 return,不会调sethook。普通断点停止零开销,只有 pause/entry 和被中途打断的 step 会真正改一次 hook。sendToMaster 'eventStop'之前;worker 只在workerThreadUpdate()里处理命令,而那次调用在其后,所以stackTrace/evaluate一定发生在取消之后。continue无影响(CMD.run本来就会step_cancel);再按一次单步会重新计算 level(stepbp里连续 3 次stepIn:debugger.lua:210→target.lua:42→target.lua:36,正常);语义上也合理:停止优先于挂起的单步。stepL/step_current_level/step_target_level清零后,在step_mask == 0时不会被读取(if (stepL == hL) { if (step_mask & LUA_MASKCALL) ... }),下一次step_*自己重算。updatehookmask只刷当前 host(非 LuaJIT 还会刷主线程);已启用thread_open(true),任何协程 resume 都会发 THREAD 事件并被updatehookmask(co)刷新干净。LuaJIT 的sethook不镜像主线程,但这与已有的CMD.run/ 单步分支调用点行为一致,不是新增行为。full_hook(step)变成idle/update hook,这正是“运行时无断点”和“断点停止”本来就会出现的状态;REPRO_AUTOUPDATE=1实测正常。bee.thread多 worker)场景未测(step_mask/stepL按 hookmgr 实例隔离);restartFrame不走runLoop,未测。测试结果
pauseentrystepbpbpstep运行时矩阵(
pause/stepbp/step各跑一遍):REPRO_AUTOUPDATE=1(走默认 update hook)下pause/stepbp修复后同样正常。原始场景(游戏内实测)已通过。