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

A Miri extern function for library-defined warnings? #2757

Open
dtolnay opened this issue Jan 15, 2023 · 10 comments
Open

A Miri extern function for library-defined warnings? #2757

dtolnay opened this issue Jan 15, 2023 · 10 comments
Labels
A-ux Area: This affects the general user experience C-proposal Category: a proposal for something we might want to do, or maybe not; details still being worked out

Comments

@dtolnay
Copy link
Member

dtolnay commented Jan 15, 2023

I am thinking about what to do with dtolnay/linkme#62.

My library has a finicky platform-specific behavior that isn't necessarily reasonable for Miri to emulate, based on extern statics and link_name/link_section attributes. Miri currently aborts when coming across it.

test readme ... error: unsupported operation: `extern` static `BENCHMARKS::LINKME_START` from crate `example` is not supported by Miri
  --> tests/example.rs:22:19
   |
22 |     for _bench in BENCHMARKS { /* ... */ }
   |                   ^^^^^^^^^^ `extern` static `BENCHMARKS::LINKME_START` from crate `example` is not supported by Miri
   |
   = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support
   = note: BACKTRACE:
   = note: inside `readme` at tests/example.rs:22:19: 22:29

I'm potentially interested in using cfg(miri) to provide a stub implementation so that Miri users can still run the rest of their program. However I'm a little hesitant about doing that silently.

What's your impression of augmenting the set of Miri extern functions to include this signature?—

#[cfg(miri)]
extern "Rust" {
    fn miri_warn(msg: &str);
}

It would emit a warning in the same style as what you currently get for isolation warnings under -Zmiri-isolation-error=warn.

warning: operation rejected by isolation
   --> .rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/unix/fs.rs:990:36
    |
990 |         let fd = cvt_r(|| unsafe { open64(path.as_ptr(), flags, opts.mode as c_int) })?;
    |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `open` was made to return an error due to isolation
    |
    = note: inside closure at .rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/sys/unix/fs.rs:990:36: 990:84
...
    = note: inside `std::fs::File::open::<&str>` at .rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/fs.rs:354:9: 354:58
note: inside `main`
   --> src/main.rs:70:5
    |
70  |     std::fs::File::open("/nonexist").unwrap_err();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I could use this to inform the user and then fall back to my stub implementation which returns a default value (empty slice in my library's case).

@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2023

I like this. We should probably make sure it's only emitted once though, even if the same warning command gets executed multiple times.

@saethlin
Copy link
Member

We should really figure out some kind of Miri utils crate if we're going to keep incentivizing library authors to do stuff like this. As-written, this is ugly to use and also the mechanism that this hooks into makes me uncomfortable because it's so permissive.

An easy follow-on to this to ask if we have warnings, why not custom errors? Why not have a miri_assert! to let library authors tell Miri about library UB?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2023

Why not have a miri_assert! to let library authors tell Miri about library UB?

isn't that just assert!? Or do you mean to avoid unwinding and catching said assertion panic?

@saethlin
Copy link
Member

saethlin commented Jan 17, 2023

I'm thinking of something that is only checked under cfg(miri) and gets an Undefined Behavior: error with a backtrace, or something substantially similar by hooking into our usual error reporting code.

But yes, one could just do

#[cfg(miri)]
assert!(condition);

But by that same logic, we don't need miri_warn, library authors can use #[cfg(miri)] with eprintln!.

As far as I understand, @dtolnay you want this supported properly in Miri so that it looks pretty and official, right? The deduplication that Oli is suggesting is the only aspect of this that I think in principle you can't do yourself in your own library.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 17, 2023

I don't care about "official" and I care only a little about "pretty". I primarily want "useful". Notably, printing the call stack and the source code snippet of the caller's code where the warning originates, which are not reasonable for my own code to perform atop eprintln.

@RalfJung
Copy link
Member

Yeah, sounds reasonable to provide a miri_warn and possibly miri_error extern hook. Also independently of that we could have a crate that wraps these extern hooks to make them easier to use (and uses #[track_caller] to get itself out of the stacktrace).

@CAD97
Copy link

CAD97 commented Feb 2, 2023

#2497 is relevant w.r.t. miri_error. By discussion in that issue (Miri is for catching language UB, not library UB; run natively compiled code with debug/careful assertions to catch library UB), miri_error should probably have an admonishment on it along the lines of "should only be used to error when an operation cannot be made to work under Miri, not for reporting misuse of unsafe API. For reporting API misuse, use debug_assert! and/or cfg(careful) from cargo-careful."

@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2023

FWIW, if there is broad interest in making Miri able to detect library UB, I'm not fundamentally opposed to that. I'm just worried that

  • it can come at the cost of finding language UB
  • library UB tends to be non-operational, so we won't be able to be nearly as good at detecting it as we are with language UB

@saethlin
Copy link
Member

saethlin commented Feb 2, 2023

Do library authors need any special support from Miri to detect library UB? Based on what @dtolnay says above, the value in miri_warn is just to get the stacktrace (well, and the code snippet. Our backtraces are so pretty). But if we're going to terminate execution anyway, library authors can already panic and we already have #[cfg(miri)].

@oli-obk
Copy link
Contributor

oli-obk commented Feb 2, 2023

Afaict this warning scheme is not for library UB, but for "we've detected we're running under miri, so now things may get weird because we can't really emulate what would happen on the OS"

@RalfJung RalfJung added A-ux Area: This affects the general user experience C-proposal Category: a proposal for something we might want to do, or maybe not; details still being worked out labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ux Area: This affects the general user experience C-proposal Category: a proposal for something we might want to do, or maybe not; details still being worked out
Projects
None yet
Development

No branches or pull requests

5 participants