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

Record stack trace for non-object exceptions #805

Merged
merged 4 commits into from
Jan 24, 2025

Conversation

bnoordhuis
Copy link
Contributor

@bnoordhuis bnoordhuis commented Jan 8, 2025

Consider this script that throws an uncaught exception:

function f() {
    throw "fail"
}
f()

Running with qjs --script t.js used to print merely:

fail

But now prints:

fail
    at f (t.js:2:11)
    at f (t.js:4:1)

qjs has been updated but no public API yet because I still need to think
through the corner cases.

Refs: #799


Some caveats:

  1. There's a small chance for ctx->error_back_trace and rt->current_exception to get out of sync when another exception is thrown without calling build_backtrace(), meaning you might see the previous exception's stack trace, but only if a lot of preconditions are met.

I'd say that's good enough for qjs for now? No stack trace is even less helpful.

  1. ctx->error_back_trace is whatever Error.prepareStackTrace returns. No different from new Error().stack but I'd be remiss if I didn't point it out.

  2. I just realized this needs some tweaks to interact properly with Error.captureStackTrace. Tomorrow, not tonight.

@saghul
Copy link
Contributor

saghul commented Jan 9, 2025

@bnoordhuis Do you reckon we could perhaps wrap that in some special object (with toString() returning the wrapped value) so we can attach the stack property instead?

@bnoordhuis
Copy link
Contributor Author

I'm not quite sure what you're asking. What does "that" in "wrap that" refer to?

@saghul
Copy link
Contributor

saghul commented Jan 9, 2025

The primitive, so basically if you throw "foo" we wrap foo into some object class so we can attach the stack, but the objet's toString returns "foo".

@bnoordhuis
Copy link
Contributor Author

You mean in a way that's observable to JS? No. It needs to be some kind of side table/lookup/thingy.

@saghul
Copy link
Contributor

saghul commented Jan 9, 2025

I though we could keep it in the engine and not surface it but use it as a way to store the stack a bit more "cleanly".

I haven't really thought this all the way through 😅 More of an out-loud thinking.

If / once you think this is ready let's go with it.

@ABBAPOH
Copy link
Contributor

ABBAPOH commented Jan 10, 2025

What about public API? I added a function similar to JS_GetException that "steals" current backtrace. Any other options?

quickjs.c Outdated
Comment on lines 6752 to 6755
// FIXME(bnoordhuis) Missing `sf->cur_pc = pc` in bytecode
// handler in JS_CallInternal. Almost never user observable
// except with intercepting JS proxies that throw exceptions.
dbuf_printf(&dbuf, " (missing)");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saghul ☝️ is a pre-existing bug that is more visible now

An example is:

  1. the OP_define_field handler in JS_CallInternal, that
  2. calls JS_SetProperty, that
  3. calls a proxy method, that
  4. calls JS_CallInternal again, that
  5. raises an exception, that then
  6. calls build_backtrace

The JSStackFrame from (4) has cur_pc set but not the one from (1).

For that particular bytecode handler it's an easy fix of doing sf->cur_pc = pc right before the JS_DefineProperty call but we should do a more rigorous review because it's definitely not the only affected bytecode handler.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! I went through a bunch and fixed them. The abort was there to help us fix whatever else we might have missed 😅

Consider this script that throws an uncaught exception:

    function f() {
        throw "fail"
    }
    f()

Running with `qjs --script t.js` used to print merely:

    fail

But now prints:

    fail
        at f (t.js:2:11)
        at f (t.js:4:1)

qjs has been updated but no public API yet because I still need to think
through the corner cases.

Refs: quickjs-ng#799
@bnoordhuis
Copy link
Contributor Author

@ABBAPOH

What about public API?

Eventually, maybe. Not right now but you can of course expose it in your local fork.

@bnoordhuis bnoordhuis merged commit 4c32c53 into quickjs-ng:master Jan 24, 2025
61 checks passed
@bnoordhuis bnoordhuis deleted the fix799 branch January 24, 2025 22:47
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.

3 participants