Repository navigation
Fix debugger lifecycle, DAP inspection, and Lua runtime edge cases - #400
Merged
Merged
Conversation
A private execution copy used to carry its trap instructions permanently, so every line boundary, call, return and loop back-edge paid a second dispatch even with no breakpoint set. Measure: a loop-heavy chunk ran 1.4-1.6x slower with an attached but idle debugger. The copy now stores its trap positions and patches or restores them when controls change. A disabled copy is byte-for-byte identical to the prototype, so idle debugging adds no per-instruction work (measured 1.00-1.06x). Stops no longer rescan every prototype's line table: breakpoint membership is cached per copy and refreshed only when breakpoints or sources change. Per-stop cost with 1,001 executed prototypes drops from 0.48-0.65 ms to 0.035 ms. Also names the reserved trap opcodes, adds IsDebugTrap, bounds-checks the ForPrep header probe and keeps Instruction.ToString from indexing past OpNames for a trap value.
Stopped runs on the paused execution before its command pump starts, so blocking on GetVariablesAsync there waited on a pump that could never run: no error, no timeout, one hung host. Inspections issued from that callback now throw InvalidOperationException naming the fix, and the event documents the constraint. Also drops the unsynchronised 'step == null' fast path in ExecutionEnded; it read a field written under the session lock from another thread for no measurable gain and could miss a step that had to be cleared.
ActiveExecutions decides when a debugger may attach and when a pending step has to end, but it was incremented with ++/-- from host calls and coroutine resumes that can run on different threads. It is now a compare-exchange for the outermost entry plus Interlocked updates elsewhere. A malformed DAP frame raised InvalidDataException, which is not an IOException, so the listener's per-connection catch missed it: the accept loop unwound and the debugger was detached from the host. The client is now dropped and the listener keeps accepting, so an editor can reconnect.
…notes The benchmark reorganization deleted the four debugger benchmarks while docs/debugger.md still pointed at them and at a cost model that no longer matched the code. Two focused workloads replace them: attached-versus-idle execution, and one stop/continue per iteration with 1 versus 2,001 executed prototypes. Both are wired into the existing BenchmarkSwitcher. The documentation now describes the checkpoint lifecycle that the code implements (patched only while controls need them), states the stop-handler inspection constraint, and records the malformed-frame behaviour of the listener.
… for A pause that arrived while Lua was inside a host call was never consumed if that execution then unwound (cancellation, host error), so the next, unrelated execution stopped at its first line. ExecutionEnded now drops the abandoned request, while a step that owns a still-suspended coroutine keeps its frames and its step. CurrentStop also reports no stop while it is resuming, matching the predicate that RequireStop and TryContinue already use, so a session cannot adopt a stop that is on its way out.
stackTrace, scopes and evaluate(frameId: 0) each built every frame of the suspended stack, then sliced. Protocol clients page 20 frames at a time, so a deeply recursive script cost O(depth) work per page and O(depth^2) to walk a large stack; the editor could appear to hang. The debugger now exposes an internal paged read whose cost follows the page, and the DAP layer uses it for all three call sites. Public GetStackTraceAsync keeps its signature and returns the full stack as before. The paging test also pins totalFrames and the stability of frame ids across pages. The documentation records stack paging, the pause-request lifetime and the Locals naming that follows debug.getlocal.
The line-event parity test compared debugger stops with Lua's own line hook for nine one-liners. Repeat-until with a local in the condition, goto labels, method calls, multi-line expressions, long comments and strings, nested closures and a loop break all place traps differently; they are now checked against the same oracle. Also pins the trap encoding: the reserved opcodes must stay outside the published instruction set, and a trap value must not overflow the instruction name table used by diagnostics.
BadArgument's (expected, actual) overloads pre-wrapped their text while the single-message overload wraps it again, so every type mismatch printed either "(string expected, got boolean))" or "((tonumber expected, got base out of range))". Lua 5.2 prints exactly one pair: "bad argument #1 to 'rep' (string expected, got boolean)". The four overloads now hand the wrapper unwrapped text, and ArgumentErrorFormatTests asserts the exact wording plus the absence of a doubled closing parenthesis for string, table, math and byte checks.
Comparison failures named the internal operator, so `1 < 'a'` produced "attempt to lt a number value with a string value". luaG_ordererror instead describes the operands: "attempt to compare number with string", or "attempt to compare two table values" when both types match. The opcode path passes "compare" and table.sort's comparator passes "lt", so both spellings are routed through one helper. OperatorTests asserted the previous wording; they now expect Lua 5.2's, and ArgumentErrorFormatTests covers number/string, table/number, table/table and a mixed table.sort.
`next(t, key)` returned nil both when iteration finished and when the key was never part of the table, so the second case silently ended a traversal instead of reporting it. Lua 5.2, and 5.3/5.4 which use the same findindex, raise "invalid key to 'next'" for that case only. The dictionary already models Lua's dead keys: clearing a field keeps its slot with a weak reference so the next() cursor stays valid, which is why clearing fields during traversal is allowed. The fix surfaces that existing distinction instead of testing membership, which is what broke nextvar.lua and the collected-key lifetime test: - LuaValueDictionary.TryGetNext returns Found/End/InvalidKey from FindIterationIndex. - LuaTable exposes TryGetNextStatus; the public TryGetNext keeps its bool contract. - next() maps InvalidKey to the Lua error. - Deleting the last live entry now keeps its dead slot, and the next insertion into an all-dead table compacts it, mirroring Lua's rehash so a stale cursor becomes invalid. Verified case by case against a native Lua 5.2 build (including single-key clear, clear-all-while-iterating, delete-then-insert, number/table keys, array holes and a replaced table); all twelve agree. NextIterationTests pins them, and the full suite (3518 tests, official Lua 5.2 suite included) still passes.
A 60-case differential run against native Lua 5.2 (patterns, string.format, #,
io, debug, load, os.date, table) left three real differences:
- malformed patterns printed "(ends with %)" without the quotes Lua includes.
- string.format('abc%') reported "invalid option '%' to 'format'". Lua reads the
missing conversion and, when no value is left, reports the missing argument:
"bad argument #2 to 'string.format' (no value)". Both places that raise this
now go through BadArgument, so the argument number and the function name agree
with the rest of the library.
- os.date hard-coded "to 'date'" instead of routing through BadArgument.
Fixing the last two exposed the naming rule behind them: when a library function
is called without a usable call-site name (through pcall, or from a C# callback),
luaL_argerror falls back to the function's global path by searching the globals,
so it reports "string.format" or "os.date"; a field call still reports "format".
GetCurrentFunctionName now implements that fallback, searching _G and its
one-level tables while skipping underscore entries.
The same corpus now differs only in debug.getinfo's short_src, which deliberately
prints "[C#]" instead of "[C]". Six regression tests pin the new messages, and the
suite (3524 tests, official Lua 5.2 suite included) passes.
ThrowBadArgument forwarded its message as the *expected type*, so every caller rendered "(select expected, got index out of range)" or "(debug.getinfo expected, got invalid option)" instead of Lua's "(index out of range)" / "(invalid option)". Eleven call sites (select, tonumber base range, math.random interval, debug options and levels, upvalue indices) were affected. The helper now hands the message to BadArgument unchanged. debug.getinfo also reported the options argument as #1; Lua numbers it #2 since the level is #1. Verified with the Lua 5.2 oracle: debug.getinfo(1,'z') now matches exactly, and select/tonumber messages match apart from the known tail-call position prefix. Suite: 3524 tests pass.
Lua 5.2 reuses a call frame only for Lua closures: `return host()` is an ordinary call, so an error raised inside the host function still reports the line that made the call. This engine discards the caller's frame for native tail calls (so values held only by that caller stay collectable while the native call is suspended, a guarantee tail-call lifetime tests enforce), which left such errors without any position: `bad argument #1 to 'rep' (...)` instead of `chunk:2: bad argument ...`. The frame now records the discarded caller's prototype and call-site pc. Holding a prototype keeps no caller values alive, so the lifetime guarantee is unchanged. WriteLastLuaTrace consults that record when the innermost frame is such a tail call. Its loop previously required at least two frames, but an unwound trace commonly holds only the host frame, so the search now also inspects index 0. The differential corpus drops from 19 to 10 differences; the rest are platform NaN text, `[C#]` branding, a `table.remove` argument number, error-position prefixes Lua omits for `next`/`yield`, and one real gap left open: debug.getinfo still reports the grandparent for the level above a tail-called host function, because the frame itself is gone. 3527 tests pass, including the tail-call lifetime suite; the new tests fail against the pre-fix library.
…es per exception The previous commit put the discarded tail-call site on CallStackFrame, which grew the frame from 32 to 48 bytes for information only error paths read. Measured on this machine (min of 3 runs, same probe binary against each build): workload 32-byte frame 48-byte frame lua calls 5M 231 ms 483 ms host calls 2M 104 ms 223 ms coroutines 20k 21.06 MB 25.94 MB allocated deep recursion 50k 10.01 MB 12.01 MB total (fresh state) pcall errors 100k 1639 ms 1634 ms So the extra fields roughly halved call throughput. The original pre-branch 32-byte frame measures 237 / 107 ms, confirming the frame size (an extra reference field to copy and a larger struct per push/pop), not the surrounding changes, is responsible. The frame is back to the fields execution needs: Base, ReturnBase, Function, VariableArgumentCount, CallerInstructionIndex, Flags (32 bytes, plain struct instead of a record struct). Both pieces of debug-only state moved out of it: - debugger frame ids live in a per-state int[] indexed by call depth, allocated on the first use and cleared whenever a depth gets a new frame, so ordinary runs never touch it (LuaDebugger reads the current frame's id through CurrentFrameVersion); - the discarded caller's position is captured by LuaRuntimeException when it is created, while the tail call is still current, and used only when the trace yields no position. That keeps the tail-call error positions from the previous commit and drops its storage cost to zero. Verified: 3527 tests, 84 debugger and 44 DAP tests pass, including the tail-call position tests and the tail-call lifetime suite (values discarded by a native tail call remain collectable). pcall throughput is unchanged, so the capture costs nothing measurable.
…le debugging Three costs the BenchmarkDotNet runs exposed, measured with ShortRun on .NET 10: DebuggerIdleBenchmark (attached vs detached) calls 120.39 us / 64,080 B -> 89.59 us / 112 B (1.55x -> 1.06x) coroutines 43.98 us / 8,576 B -> 45.44 us / 8,584 B (1.01x, unchanged) numeric-loop 89.74 us / 32 B -> 93.50 us / 32 B (1.00x, unchanged) DebuggerStopBenchmark (500 stops per invocation) 0 functions 120.3 us -> 116.3 us 2,000 functions 8,325 us -> 1,157 us (16.7 us -> 2.3 us per stop) The 64 KB per operation on the calls workload was an allocation regression from the checkpoint redesign: every frame entry and return asked the session for its execution copy, so each call took two cache lookups. The context now keeps the copy it just replaced and reuses it when execution alternates between a caller and its callee, which is the common pattern, and only falls back to the session for a third prototype or a changed debugger identity. That removes both the lookups and the allocation (calls now allocate 112 B per operation rather than 64,080 B) and also fixes the pre-existing 1.6x cost: the pre-branch build showed the same slowdown with no allocation, so the lookups, not the copies, were the expense. Stops no longer un-patch and re-patch every prototype. A step stays armed while the program is stopped (Continue clears it when stepping really ends), a breakpoint or step stop leaves the traps as they are, and control updates only run when the enabled state actually changes rather than on every stop, continue and stop-end. The remaining difference between 116 us and 1,157 us is the benchmark workload itself creating 2,000 closures per invocation, not debugger bookkeeping. Verified: 3527 tests, 84 debugger and 44 DAP tests pass, including stepping, breakpoint editing during a stop, detach/reset and the lifetime suites.
The solution build for every target framework caught a netstandard2.1-only error in the new frame-version array: Array.Clear(array) does not exist there. It now uses the (start, length) overload. Typed argument getters reported "value expected" when the argument was absent, which is luaL_checkany's wording. Lua distinguishes an absent argument from an explicit nil and says "X expected, got no value", so select(), string.byte(), math.abs(), table.concat(), string.format() and any other typed check now match, while an explicit nil still reports "expected, got nil" and the untyped getter keeps "value expected". select also read its selector with the untyped getter, so pcall(select) reported "value expected" instead of the integer check's message; it now peeks and lets the number check produce the Lua message. The 5.2 differential corpus is down to the deliberate "[C#]" short source name and a name Lua itself picks non-deterministically for a function reached through pcall. All 3527 tests pass, and Lua.slnx builds with no warnings on netstandard2.1, net6.0, net8.0 and net10.0.
A tail call into a host function replaces the caller's frame to keep that caller's values collectable, so `debug.getinfo` counted one frame too few: `getinfo(1)` inside such a call reported the grandparent (or nil), and `getlocal`/`setlocal` read the grandparent's registers. Lua 5.2 keeps the frame for C callees, so this was a silent wrong answer rather than an error. The stack a script observes is now one logical frame deeper than the physical stack while the top frame is a tail-called host function, using the prototype and call site the state records at the tail call (LastCallerPrototype/LastPc, already maintained for error positions): - `getinfo` level 1 is reconstructed from that record, so `what`, `source`, `short_src`, `linedefined`, `lastlinedefined`, `currentline`, `nparams`, `isvararg`, `nups`, `istailcall` and `name`/`namewhat` report the discarded caller; higher levels shift by one and again reach the real frames. Only `f` is nil: the closure was released with the frame. - `getlocal`/`setlocal` keep correct level numbering and report no locals for that frame, because its registers were reused; previously they silently returned the grandparent's locals. - LuaDebug gained a prototype-based info path for the closure-less frame. Verified against native Lua 5.2: `getinfo(1,'Slu')` now matches field for field (what/short_src/linedefined/currentline/nparams/isvararg) through a discarded caller, as does `getinfo(2)`. Four regression tests pin the levels, the fields and the local limitation; docs/stdlib/debug.md documents it. 3531 tests pass and the solution builds on all target frameworks.
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.
Summary
error(..., 0).nextcursors, argument errors, and comparison diagnostics with Lua 5.2. Preserve valid deleted cursors and skip empty hash scans.Verification