Skip to content

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

Merged
merged 19 commits into from
Jan 9, 2023
Merged

Print hook argument values on crash #603

merged 19 commits into from
Jan 9, 2023

Conversation

Baltoli
Copy link
Contributor

@Baltoli Baltoli commented Nov 23, 2022

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 a std::format lookalike that we should aim to drop whenever C++20 is mainstream. We now just pull in fmt as it's not an onerous dependency to use.

@Baltoli Baltoli marked this pull request as ready for review November 24, 2022 12:11
@Baltoli Baltoli changed the title Better error messages from hooks Print hook argument values on crash Nov 24, 2022
Copy link
Collaborator

@dwightguth dwightguth left a 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.

return floatToString(f, suffix);
}

std::string intToString(mpz_t i) {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@Baltoli
Copy link
Contributor Author

Baltoli commented Nov 29, 2022

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.

The dependency graph in this PR currently looks like this:

download

If we were to pull numeric_strings back into the strings library, we'd end up with a circular dependency, which is what I was aiming to avoid as a question of taste (really not a big deal though, as CMake is happy to link it for you, I think). Happy to use the circular version if you'd prefer that.

@Baltoli Baltoli mentioned this pull request Jan 9, 2023
@rv-jenkins rv-jenkins merged commit a9a4918 into master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants