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
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.
|
@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? |
Agent-Logs-Url: https://github.com/G-Yong/quickjs/sessions/f6258d46-0a54-473d-9197-b85a494a7bb2 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>
Agent-Logs-Url: https://github.com/G-Yong/quickjs/sessions/a7eecad6-7beb-49a4-90cf-0a7c0ad40461 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>
The previous codegen on Windows embedded CRLF line endings from fib_module.js source into the bytecode, causing an 8-byte size difference vs upstream. Updated codegen.ps1 to normalize JS input files to LF before compilation.
|
I didn't quite understand why emit_source_loc_debug was needed before. Now I get it a little bit— the newly added emit_source_loc calls are debug-specific (for stepping through if/for/return/etc.), unlike the original 5 upstream calls which are always needed for error reporting. Having emit_source_loc_debug as a no-op without debugger support keeps the bytecode identical to upstream and avoids the performance overhead. |
* fix: wrap debug_trace test with #ifdef QJS_ENABLE_DEBUGGER Agent-Logs-Url: https://github.com/G-Yong/quickjs/sessions/c47f60f2-9eab-4e86-a75e-3755d5e81466 Co-authored-by: G-Yong <21030893+G-Yong@users.noreply.github.com> * chore: remove accidentally committed build-debug directory Agent-Logs-Url: https://github.com/G-Yong/quickjs/sessions/c47f60f2-9eab-4e86-a75e-3755d5e81466 Co-authored-by: G-Yong <21030893+G-Yong@users.noreply.github.com> --------- 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>
Yes, exactly. :-) The reason I suggested it is because it lets the debugger be a runtime option instead of a compile-time option. |
…nstead of a compile-time option
|
Thanks @bnoordhuis, I now understand the intent! I've refactored the implementation to make the debugger a runtime option instead of a compile-time option: Key changes:
This means a single QuickJS binary can serve both as a normal engine and a debug-capable engine, controlled entirely at runtime by whether |
|
@dblnz A note about the The earlier approach (from my fork's restart:
for(;;) {
#ifdef QJS_ENABLE_DEBUGGER
if (b && ctx->bytecode_trace != NULL) {
// callback here...
}
#endif
SWITCH(pc) {
CASE(OP_push_i32): ...This had a fundamental problem with #if defined(EMSCRIPTEN) || defined(_MSC_VER) || defined(QJS_ENABLE_DEBUGGER)
#define DIRECT_DISPATCH 0
The current This is why this PR's approach is superior to what I originally proposed in #119. |
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]