Skip to content

Add Debugging Interface#1421

Open
G-Yong wants to merge 60 commits intoquickjs-ng:masterfrom
G-Yong:master
Open

Add Debugging Interface#1421
G-Yong wants to merge 60 commits intoquickjs-ng:masterfrom
G-Yong:master

Conversation

@G-Yong
Copy link
Copy Markdown

@G-Yong G-Yong commented Mar 26, 2026

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]
debugInVSCode

添加位置变动(操作变动)回调接口,为外部实现调试功能实现可能
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)
@saghul
Copy link
Copy Markdown
Contributor

saghul commented Mar 26, 2026

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?

@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Mar 28, 2026

Thanks for the feedback @saghul! I've added a compile-time macro QJS_ENABLE_DEBUGGER to gate all the debug-related code. Here's a summary of the changes:

Compile-time gating (QJS_ENABLE_DEBUGGER)

  • All debug fields in JSContext, all debug API implementations (JS_SetOPChangedHandler, JS_GetStackDepth , JS_GetLocalVariablesAtLevel, JS_FreeLocalVariables ) , and the per-opcode callback in JS_CallInternal are now wrapped in #ifdef QJS_ENABLE_DEBUGGER.
  • The declarations in quickjs.h are similarly guarded.
  • When QJS_ENABLE_DEBUGGER is defined, DIRECT_DISPATCH is automatically set to 0 (alongside the existing EMSCRIPTEN and _MSC_VER conditions), so the switch-based dispatch is used and the opcode callback fires correctly.
  • A new xoption(QJS_ENABLE_DEBUGGER ...) has been added to CMakeLists.txt , defaulting to OFF. When not enabled, there is zero overhead — no extra struct fields, no callback checks in the hot path, and DIRECT_DISPATCH remains unaffected.

Other cleanups

  • Renamed JSLocalVar to JSDebugLocalVar to avoid confusion with the internal JSVarDef.
  • Fixed a potential memory leak where funcname was only freed inside if (filename).
  • Removed leftover debug fprintf comments and unnecessary braces in the hot path.

Let me know if there's anything else you'd like adjusted.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Mar 28, 2026

@bnoordhuis WDYT?

Copy link
Copy Markdown
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

G-Yong added 3 commits March 30, 2026 09:50
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.
@saghul
Copy link
Copy Markdown
Contributor

saghul commented Apr 9, 2026

@bnoordhuis any chance you can take a look?

G-Yong and others added 4 commits April 13, 2026 09:08
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>
@bnoordhuis
Copy link
Copy Markdown
Contributor

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?

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Apr 13, 2026

Ah sorry I think that's my bad, I misunderstood what you meant 😢

@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Apr 14, 2026

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?

Okay, I will first conduct a comparative test based on the tools you provided to see the actual impact.

@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Apr 14, 2026

Web Tooling Benchmark Performance Test Results

I ran web-tooling-benchmark (17 real-world JavaScript workloads) to measure the performance impact of this PR's OP_debug changes.

Test Environment

Item Detail
OS Windows 10.0.26200
Compiler MSVC 19.29.30159.0 (Visual Studio 2019)
Build CMake Release mode
Upstream quickjs-ng/quickjs master (v0.14.0)
Fork G-Yong/quickjs (this PR branch, with unconditional OP_debug)
Runs 3 per version, results averaged

Detailed Results (runs/s, higher is better)

Benchmark Upstream (avg) Fork (avg) Diff
acorn 0.59 0.57 -3.4%
babel 1.70 1.62 -4.7%
babel-minify 2.38 2.24 -5.9%
babylon 1.20 1.11 -7.5%
buble 1.99 1.95 -2.0%
chai 2.45 2.40 -2.0%
espree 0.44 0.42 -4.5%
esprima 0.81 0.78 -3.7%
jshint 1.95 1.90 -2.6%
lebab 1.52 1.54 +1.3%
postcss 1.55 1.47 -5.2%
prepack 2.08 1.93 -7.2%
prettier 0.81 0.78 -3.7%
source-map 1.04 1.02 -1.9%
terser 4.59 4.42 -3.7%
typescript 1.86 1.80 -3.2%
uglify-js 1.29 1.24 -3.9%
Geometric mean 1.43 1.38 -3.5%

Raw Run Data

Upstream: 1.43, 1.42, 1.45 (avg: 1.433)
Fork: 1.36, 1.39, 1.39 (avg: 1.380)

Summary

The unconditional OP_debug emission causes a ~3.5% overall performance regression (geometric mean). The impact is not uniform — parser-heavy benchmarks like babylon (-7.5%) and prepack (-7.2%) are hit harder, likely due to higher bytecode volume and more frequent OP_debug dispatch in tight loops.

@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Apr 14, 2026

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.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Apr 14, 2026

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?

G-Yong and others added 9 commits April 15, 2026 09:34
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.
@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Apr 15, 2026

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>
@saghul saghul mentioned this pull request Apr 15, 2026
@bnoordhuis
Copy link
Copy Markdown
Contributor

Having emit_source_loc_debug as a no-op without debugger support keeps the bytecode identical to upstream and avoids the performance overhead.

Yes, exactly. :-)

The reason I suggested it is because it lets the debugger be a runtime option instead of a compile-time option.

@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Apr 16, 2026

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:

  • Removed all #ifdef QJS_ENABLE_DEBUGGER guards and the CMake option entirely
  • OP_debug is now always defined in the opcode table
  • emit_source_loc_debug() checks ctx->debug_trace at runtime — when no trace handler is set, it's a no-op, so the emitted bytecode is identical to upstream with zero performance overhead
  • When a user calls JS_SetDebugTraceHandler(ctx, cb), subsequent JS compilation will emit OP_debug opcodes and the debugger becomes active
  • All interpreter/optimizer code that handles OP_debug is always compiled in (no #ifdef guards needed — if OP_debug isn't emitted, those code paths are never reached)

This means a single QuickJS binary can serve both as a normal engine and a debug-capable engine, controlled entirely at runtime by whether JS_SetDebugTraceHandler has been called before JS code is compiled.

@G-Yong
Copy link
Copy Markdown
Author

G-Yong commented Apr 16, 2026

@dblnz A note about the DIRECT_DISPATCH issue you ran into in #119:

The earlier approach (from my fork's QJS_ENABLE_DEBUGGER branch) placed the debug callback at the top of the for(;;) loop, before SWITCH(pc):

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 DIRECT_DISPATCH: when enabled (Linux/GCC), BREAK is defined as SWITCH(pc) which uses computed goto (goto *dispatch_table[*pc++]) to jump directly to the next opcode's label — completely bypassing the loop top where the callback lived. Only the very first instruction would trigger the callback. That's why the old approach had to force DIRECT_DISPATCH=0:

#if defined(EMSCRIPTEN) || defined(_MSC_VER) || defined(QJS_ENABLE_DEBUGGER)
#define DIRECT_DISPATCH  0

The current OP_debug approach from @bnoordhuis is fundamentally better — the debug logic lives inside CASE(OP_debug):, which is a proper entry in the dispatch table. Whether using switch/case or computed goto, OP_debug is dispatched just like any other opcode. No need to disable DIRECT_DISPATCH, so there is zero performance impact on the dispatch mechanism when debugging is not active.

This is why this PR's approach is superior to what I originally proposed in #119.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants