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

Handle x86 errors more gracefully #1194

Merged
merged 3 commits into from
Apr 19, 2021
Merged

Handle x86 errors more gracefully #1194

merged 3 commits into from
Apr 19, 2021

Conversation

atomb
Copy link
Contributor

@atomb atomb commented Apr 19, 2021

And provide a template for catching other varieties of exception more
consistently.

Fixes #678.

And provide a template for catching other varieties of exception more
consistently.
@RyanGlScott
Copy link
Contributor

Is the idea to consolidate all of the exception-handling code into the io function? I ask since there are other places where liftIO is used in conjunction with catch:

liftIO $ resolveSetupVal cc m tyenv nameEnv sval' `X.catch` handleException opts

lval <- liftIO $ resolveSetupVal cc mem m tyenv nameEnv sval' `X.catch` handleException opts

Would it make sense to consolidate these bits of functionality into io as well?

@atomb
Copy link
Contributor Author

atomb commented Apr 19, 2021

I think it would make sense, unless anyone else knows of issues that might come up as a result of that. And there are almost 300 instances of liftIO in the saw-script repo that could probably be migrated. My current thought is that this PR lays the groundwork for doing that, but that we make the more sweeping changes after the 0.8 release.

@RyanGlScott
Copy link
Contributor

That sounds reasonable to me. I suppose the only other thing I might desire from this PR is a test case that demonstrates the new error message, assuming that that is feasible to set up. If that's not feasible, then perhaps showing an example of the new error message in a GitHub comment would suffice.

@atomb
Copy link
Contributor Author

atomb commented Apr 19, 2021

Our current test infrastructure would make it a bit difficult to check the error message in this case. It would be nice to set something up to make that straightforward in the future, though. For the moment, here's an example of the error message I get from the code in this PR when I change the ELF text in an executable header to EGF:

[19:11:50.679] Loading file "/Users/atomb/galois/saw-script/intTests/test_llvm_x86_01/test.saw"
[19:11:50.684] Finding symbol for "foo"
[19:11:50.684] Stack trace:
"llvm_verify_x86" (/Users/atomb/galois/saw-script/intTests/test_llvm_x86_01/test.saw:14:20-14:35):
Error in x86 code: Invalid ELF header at offset 0: Invalid magic number for ELF: ("\DELEGF","\DELELF")

Copy link
Contributor

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Excellent. We should open a separate issue about consolidating exception-handler–related code in io, but beyond that, LGTM.

@atomb atomb added the PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run label Apr 19, 2021
@mergify mergify bot merged commit 309ae98 into master Apr 19, 2021
@mergify mergify bot deleted the at-x86-error-handling branch April 19, 2021 22:12
@RyanGlScott RyanGlScott added the subsystem: x86 Issues related to verifying x86 binaries via Macaw label Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready to merge Magic flag for pull requests to ask Mergify to merge given an approval and a successful CI run subsystem: x86 Issues related to verifying x86 binaries via Macaw
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Invalid ELF header" error doesn't print location information
2 participants