Skip to content

Fix more error cases #332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 26, 2024
Merged

Fix more error cases #332

merged 2 commits into from
Mar 26, 2024

Conversation

chqrlie
Copy link
Collaborator

@chqrlie chqrlie commented Mar 25, 2024

  • fix more cases of missing sf->cur_pc.
  • use more precise error messages for number conversion methods
  • add test cases in test_builtin.js

chqrlie added 2 commits March 25, 2024 21:55
@chqrlie
Fix more error cases
35039a8 
- fix more cases of missing `sf->cur_pc`.
- use more precise error messages for number conversion methods
- add test cases in test_builtin.js
@chqrlie
unbreak v8 tests
60c3cc6
saghul
saghul approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

LGTM with a question. Now that you covered all cases, does it make sense to still set cur_pc in the exception: label? If not, shall we skip it and add a check-abort?

v8.txt
@@ -762,6 +762,12 @@ Failure (testClassNames doesn't contain expected[1] stack = at MyObj (stack-
at <eval> (stack-traces.js:291:49)
): expected <true> found <false>
Failure (UnintendedCallerCensorship didn't contain new ReferenceError): expected <true> found <false>
Failure: expected <"abc"> found <undefined>
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, how did this come back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know. I am investigating this, but won't have much time until tonight.
We should have a more complete emulation of the v8 error handling:

  • the stack trace should be computed by a v8 specific function Error.captureStackTrace, which is non-standard but is quite handy for introspection. It takes an object as argument and sets its stack property to a the stack trace string, and an optional second argument which is an array of frames.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually already implemented that here in this fork! :-)

555d837
4c929c5

tests/test_builtin.js
@@ -966,6 +966,54 @@ function test_proxy_is_array()
}
}

function test_cur_pc()
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice!

@chqrlie
Copy link
Collaborator Author

chqrlie commented Mar 26, 2024

LGTM with a question. Now that you covered all cases, does it make sense to still set cur_pc in the exception: label? If not, shall we skip it and add a check-abort?

Yes, sf->cur_pc must still be set in the exception: label because it is needed for the non recursive cases where an exception is thrown directly from JS_CallInternal or indirectly by a c function without recursing to a javascript function.

Conversely, we might want to initialize sf->cur_pc to pc instead of null to avoid crashing in production on a case where the synchronisation is missing and keep the sf->cur_pc = NULL as an ifdef's debugging alternative for a specific CI target.

@saghul
Copy link
Contributor

saghul commented Mar 26, 2024

Conversely, we might want to initialize sf->cur_pc to pc instead of null to avoid crashing in production on a case where the synchronisation is missing and keep the sf->cur_pc = NULL as an ifdef's debugging alternative for a specific CI target.

TBH I'd rather abort hard so it's easy to catch, fix and be done with it...

@chqrlie
Copy link
Collaborator Author

chqrlie commented Mar 26, 2024

Conversely, we might want to initialize sf->cur_pc to pc instead of null to avoid crashing in production on a case where the synchronisation is missing and keep the sf->cur_pc = NULL as an ifdef's debugging alternative for a specific CI target.

TBH I'd rather abort hard so it's easy to catch, fix and be done with it...

In the debug target, that's fine, but in the production version, why abort when there is no user visible downside?

@chqrlie chqrlie merged commit f02ed18 into quickjs-ng:master Mar 26, 2024
@chqrlie chqrlie deleted the more-error-fixes branch March 26, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saghul saghul saghul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chqrlie @saghul