Conversation
添加位置变动（操作变动）回调接口，为外部实现调试功能实现可能
Introduces JS_SetOPChangedHandler to allow setting a callback for operation changes in the JSContext. Also adds calls to emit_source_loc in various statement parsing locations to improve source location tracking during parsing.
假如没有，位置跟踪会发生异常。
解决在函数内出现静态错误时，返回的堆栈信息中的列号错误的bug。
Introduces functions to get stack depth and retrieve local variables at a specific stack frame level, along with a struct for local variable info and a function to free the allocated array. Also updates the JSOPChangedHandler signature to include JSContext for improved debugging capabilities.
假如采用旧的代码，会发生下面的错误：
function add(a, b){
return a + b;
var b // OP_return会出现在这里
while(1){}
}
add(1, 2)
|
At a quick glance, this looks better than the other approaches I've seen, kudos! Now, since this will have a performance impact, I'd say we want it gated with a compile time time macro. @bnoordhuis thoughts? |
|
Thanks for the feedback @saghul! I've added a compile-time macro Compile-time gating (
|
|
@bnoordhuis WDYT? |
bnoordhuis
left a comment
bnoordhuis
left a comment
There was a problem hiding this comment.
Behind a compile-time flag would be good. The diff mostly looks good to me but there are a couple of changes I'd like to see:
-
the API functions should always be available but return an error when compiled without debugger support
-
can you rename the new emit_source_loc calls to something like emit_source_loc_debug and turn that into a no-op when there's no debugger support?
-
I don't love the name JSOPChangedHandler. Suggestions for a better one? Something like JSBytecodeTraceFunc perhaps?
I'm assuming this good enough as a building block to assemble a functional debugger from? I can see how it lets you single-step through code, inspect stack frames, set breakpoints, etc., so I'm guessing... yes?
Rename the old operation_changed/JSOPChangedHandler to bytecode_trace/JSBytecodeTraceFunc and replace JS_SetOPChangedHandler with JS_SetBytecodeTraceHandler. Add conditional compilation guards so debugger-related code is compiled only when QJS_ENABLE_DEBUGGER is set (including stack depth, local-variable APIs, and freeing logic). Introduce emit_source_loc_debug no-op macro when debugger is disabled and make JS_GetStackDepth return -1 without the debugger. Update public header comments to reflect the new API and behavior.
Replace the old JS_NewDebugContext API with JS_SetDebugBreakHandler(ctx, cb) so callers can set or clear a debug-break callback on any existing context. Update header docs to clarify that OP_debug opcodes are always emitted at statement boundaries and the callback is only invoked when set. Remove the emit_debug flag from JSParseState and always emit OP_debug in emit_source_loc; initialization no longer toggles emit_debug. This decouples bytecode emission of debug traps from whether a handler was present at context creation, allowing handlers to be attached later.
OP_debug is a transparent no-op (0 pop, 0 push) used solely for debugger breakpoints. Setting last_opcode_pos to point at OP_debug breaks all peephole optimizations that rely on get_prev_opcode(), because they see OP_debug instead of the real preceding opcode. Affected code paths include: - js_is_live_code(): misidentifies dead code as live - set_object_name(): fails to match OP_set_name / OP_set_class_name - lvalue parsing: falls into the default (invalid lvalue) branch - set_object_name_computed(): fails to rewrite opcodes This caused CI test failures when OP_debug was always emitted. Fix: stop updating last_opcode_pos in emit_source_loc(), making OP_debug transparent to the peephole optimizer, just like OP_source_loc already is.
|
Since the bytecode needs to be regeneratred, can you bump the BC_VERSION please? You'll need to update the fuxxing tests too. |
|
Done — bumped |
Rename debug break callback and related symbols to use "trace" naming: JSDebugBreakFunc -> JSDebugTraceFunc, ctx->debug_break -> ctx->debug_trace, and JS_SetDebugBreakHandler -> JS_SetDebugTraceHandler; update call sites in the bytecode interpreter accordingly. Bump BC_VERSION from 25 to 26 to reflect the bytecode changes and update tests/test_bjson.js base64 fixtures (Gf -> Gv) to match the new bytecode format. Also adjust the JS_GetStackDepth comment wording.
|
Any chance you can add a small test in api-test.c ? |
|
Added a
|
|
Looks like you didn't regenerate the bundled bytecode after bumping it, and tests are failing due to that. |
…mismatch (25 -> 26) (#4) Agent-Logs-Url: https://github.com/G-Yong/quickjs/sessions/22cd13bd-1c2e-44b2-af85-79b7cf479498 Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: G-Yong <21030893+G-Yong@users.noreply.github.com>
|
Fixed — ran The reason I didn't include the codegen step earlier was intentional: regenerating these files produces a large diff across multiple generated files, which would have buried the actual debugging interface changes and made the PR much harder to review. Now that the core changes have been reviewed, I've added the regenerated files in a separate commit. |
|
@bnoordhuis any chance you can take a look? |
The merge commit ef48f55 incorrectly resolved a conflict, leaving the debug_trace() function without its closing cleanup code and brace. This caused new_symbol() and main() to be parsed as nested functions, breaking compilation in the tsan and valgrind CI workflows. Agent-Logs-Url: https://github.com/G-Yong/quickjs/sessions/0d229a58-5855-48f9-874d-3c9468227bad Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: G-Yong <21030893+G-Yong@users.noreply.github.com>
When a bare 'return' statement triggers Automatic Semicolon Insertion, the parser has already advanced to the next token (which may be on a different line). emit_return() then calls emit_source_loc() using that token's position, causing the debugger to report execution on a line after the return statement (e.g. unreachable 'var b = 10'). Fix by saving the 'return' keyword's source position before advancing the parser, and temporarily restoring it before calling emit_return().
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: G-Yong <21030893+G-Yong@users.noreply.github.com>
|
I intentionally suggested emitting OP_debug conditionally because doing it unconditionally a) makes the bytecode bigger, and b) slows down the interpreter dispatch loop. Can you at least check that https://github.com/quickjs-ng/web-tooling-benchmark doesn't regress? |
|
Ah sorry I think that's my bad, I misunderstood what you meant 😢 |
Okay, I will first conduct a comparative test based on the tools you provided to see the actual impact. |
Web Tooling Benchmark Performance Test ResultsI ran web-tooling-benchmark (17 real-world JavaScript workloads) to measure the performance impact of this PR's Test Environment
Detailed Results (runs/s, higher is better)
Raw Run DataUpstream: 1.43, 1.42, 1.45 (avg: 1.433) SummaryThe unconditional |
|
I'm a bit confused. @bnoordhuis, do you mean we should keep OP_debug but compile it conditionally? Also, regarding the emit_source_loc_debug issue mentioned earlier, there might be a problem. If we adopt emit_source_loc_debug, we not only need to add it at specific locations, but also replace the existing emit_source_loc with emit_source_loc_debug. Otherwise, the statements represented by the original emit_source_loc won't respond to OP_debug. |
|
Sorry @G-Yong i added to the confusion. As your benchmarks show, just by having the debug opcodes things get slower so it should be opt-in. Not sure if compile time or config, @bnoordhuis what's on your mind? |
Add a debugging interface to the existing architecture.
Using the added debugging interface, we implemented debugging for QuickJS in VSCode. The project address is:[QuickJS-Debugger]