Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect column number in stack traces repro #236

Open
TooTallNate opened this issue Dec 26, 2023 · 2 comments
Open

Incorrect column number in stack traces repro #236

TooTallNate opened this issue Dec 26, 2023 · 2 comments

Comments

@TooTallNate
Copy link
Contributor

Running minified code, I'm seeing incorrect column number on the generated stack trace, leading to source maps being incorrect traced. Example:

// min.js
if (typeof print === 'undefined') globalThis.print = console.log;
function r(){return e()}function e(){return new Error("bad")}print(r().stack)

In quickjs:

$ ./build/qjs min.js 
    at e (min.js:2:49)
    at r (min.js:2:21)
    at <eval> (min.js:2:49)

Compared to Node.js:

$ node min.js 
Error: bad
    at e (/Users/nrajlich/Code/quickjs-ng/quickjs/min.js:2:45)
    at r (/Users/nrajlich/Code/quickjs-ng/quickjs/min.js:2:21)
    at Object.<anonymous> (/Users/nrajlich/Code/quickjs-ng/quickjs/min.js:2:68)
    # other lines omitted for brevity

Notes:

  • QuickJS and V8 seem to differ in where the start of the error is - V8 starts at new, QuickJS starts at Error. This might be OK, but I figured I'd note the difference.
  • Bigger issue is that the 3rd line of the stack is totally off. It matches the first line of the stack in QuickJS, but should be pointing to column 68 which is where the r() function is invoked.

Not clear to me, but maybe this is related to #232?

@hsiaosiyuan0
Copy link

hsiaosiyuan0 commented Dec 26, 2023

JSParseState::mark is used as the start buffer offset of the current parsing token, so it should be update properly by the previous token parsing. However current impl updates mark when:

  1. newline introduced
    case '\n':
        p++;
    line_terminator:
        s->eol = &p[-1];
        s->mark = p;
  1. whitespace introduced
    case ' ':
    case '\t':
        s->mark = ++p;
        goto redo;
    case '/':
        if (p[1] == '*') {
            /* comment */
            // ...
                    s->mark = p;

it will work in most case, since human typed sources always have proper whitespace in set, but in your case, there is no whitespace between ( and r, which leads the column be miscalculate, it's easy to confirm this by put one space between theme:

if (typeof print === 'undefined') globalThis.print = console.log;
function r(){return e()}function e(){return new Error("bad")}print( r().stack)

then you will get a more reasonable error stack:

    at e (test.js:2:49)
    at r (test.js:2:21)
    at <eval> (test.js:2:69)

@bnoordhuis
Copy link
Contributor

I had a quick look and I think there are two things going on here, one simple, one less so:

  1. Simple: replacing s->token.col_num = s->mark - s->eol with s->token.col_num = s->token.ptr - s->eol gives column numbers that are much closer to their expected values, and probably obviates the need to track s->mark altogether (good!)

  2. Less simple: source locations are currently inserted into the bytecode automatically by emit_op() and then turned into stack traces by finding the OP_source_loc that's closest to the program counter1. The expression r() produces two source locations, one for r and one for the ), with the second one getting matched...

Likely the best solution to (2) is to emit source locations explicitly instead of having them created implicitly. Onerous but gives the most control.

1 I'm taking some creative liberties here. It's literally true for the first two parse phases but in the final phase they're stripped from the bytecode and stored in a side table.

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

No branches or pull requests

3 participants