Add support for panicking in the emulated application when unsupported functionality is encountered #1818
Conversation
Would it be desired to expand this PR to encompass these areas, or perhaps make the flag more generic for future expansion? |
|
Seems reasonable to me. :) Besides the TODOs you'd mention, I think it might make sense to create a helper function that encapsulates the |
|
Btw, you probably also want to adjust this here to panic when the flag is set: miri/src/shims/foreign_items.rs Line 167 in 4fa9363 |
|
☔ The latest upstream changes (presumably #1791) made this pull request unmergeable. Please resolve the merge conflicts. |
landaire
force-pushed
the
feature/panic-on-unsupported-syscalls
branch
from
a3808f9
to
c3d235d
Compare
|
Perhaps someone with a keen eye can spot why the test I added is failing. The diff doesn't actually show a delta. |
landaire
changed the title
|
☔ The latest upstream changes (presumably #1776) made this pull request unmergeable. Please resolve the merge conflicts. |
landaire
force-pushed
the
feature/panic-on-unsupported-syscalls
branch
from
20da2f1
to
4291aba
Compare
…d syscalls are encountered
landaire
force-pushed
the
feature/panic-on-unsupported-syscalls
branch
from
510580a
to
ae23709
Compare
|
Looking great, thanks a lot for working with me through the review. :-) |
|
📌 Commit ae23709 has been approved by |
Seriously, thank you for your patience and great feedback. Hopefully this will not be my last contribution! |
|
☀️ Test successful - checks-actions |
| id => throw_unsup_format!("Miri does not support syscall ID {}", id), | ||
| id => { | ||
| this.handle_unsupported(format!("can't execute syscall with ID {}", id))?; | ||
| return Ok(EmulateByNameResult::NotSupported); |
There was a problem hiding this comment.
Should this be EmulateByNameResult::AlreadyJumped?
There was a problem hiding this comment.
Hmmm, I think you are right. This is one of the scenarios I did not test and may have been a merge error.
There was a problem hiding this comment.
Ah right, NotSupported means "look up exported function symbol".
I wonder if we should look up the exported symbol first, and only then run our own shim? Then we might not even need the NotSupported variant.
There was a problem hiding this comment.
I wonder if we should look up the exported symbol first, and only then run our own shim? Then we might not even need the
NotSupportedvariant.
I think the variant is still needed -- the exported symbol will only be run if no shim implementation can be found, and NotSupported is required to represent that.
ghost
mentioned this pull request
Fix the wrong `EmulateByNameResult::NotSupported` in `syscall` shim Without the change, the newly added test will fail with: ```diff -thread 'main' panicked at 'unsupported Miri functionality: can't execute syscall with ID 0', $DIR/unsupported_syscall.rs:10:9 +thread 'main' panicked at 'unsupported Miri functionality: can't call foreign function: syscall', $DIR/unsupported_syscall.rs:10:9 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` cc #1818 (comment)
This PR fixes #1807 and allows an optional flag to be specified to panic when an unsupported syscall is encountered. In essence, instead of bubbling up an error in the context of the Miri application Miri will panic within the context of the emulated application. This feature is desired to allow CI pipelines to determine if a Miri failure is unsupported functionality or actual UB. Please read this comment for the rationale behind this change.
Note: this change does not cover all cases where unsupported functionality errors may be raised. If you search the repo for
throw_unsup_format!there are many cases that I think are less likely to occur and may still be problematic for some folks.TODO: