-
Notifications
You must be signed in to change notification settings - Fork 23
Print hook argument values on crash #603
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple comments. Also, the string library already depends on the int and float libraries; I don't think we probably need to create a whole separate library just for the conversion functions, which ought to significantly simplify this diff.
runtime/strings/numeric.cpp
Outdated
return floatToString(f, suffix); | ||
} | ||
|
||
std::string intToString(mpz_t i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You reused the existing code for converting floats to strings, but not the existing code for converting ints to strings. I would rather we reuse the code that already exists as it has been thoroughly tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem; will address this in the appropriate way depending on your opinion on the dependency graph question below.
The dependency graph in this PR currently looks like this: If we were to pull |
This PR implements hook argument printing for all hook arguments that aren't K terms (i.e. numeric and string arguments). Combined with #599, this means that when a hook crashes in the LLVM backend, you can see precisely which hook it was, as well as the actual argument values that caused the crash. This should save a lot of debugging and investigating time.
To implement the change without adding additional dependencies to the runtime components of the backend, this PR contains a tiny implementation of aWe now just pull instd::format
lookalike that we should aim to drop whenever C++20 is mainstream.fmt
as it's not an onerous dependency to use.