miri icon indicating copy to clipboard operation
miri copied to clipboard

A Miri extern function for library-defined warnings?

Open dtolnay opened this issue 2 years ago • 10 comments

I am thinking about what to do with https://github.com/dtolnay/linkme/issues/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).

dtolnay avatar Jan 15 '23 04:01 dtolnay

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

oli-obk avatar Jan 17 '23 08:01 oli-obk

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?

saethlin avatar Jan 17 '23 16:01 saethlin

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?

oli-obk avatar Jan 17 '23 16:01 oli-obk

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.

saethlin avatar Jan 17 '23 16:01 saethlin

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.

dtolnay avatar Jan 17 '23 17:01 dtolnay

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).

RalfJung avatar Jan 17 '23 17:01 RalfJung

#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."

CAD97 avatar Feb 02 '23 06:02 CAD97

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

RalfJung avatar Feb 02 '23 10:02 RalfJung

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)].

saethlin avatar Feb 02 '23 15:02 saethlin

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"

oli-obk avatar Feb 02 '23 16:02 oli-obk