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

Enable Miri to emit warnings without halting execution #797

Open
RalfJung opened this issue Jun 28, 2019 · 15 comments
Open

Enable Miri to emit warnings without halting execution #797

RalfJung opened this issue Jun 28, 2019 · 15 comments
Labels
A-ux Area: This affects the general user experience C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 28, 2019

For some things it might be nice if Miri could emit a warning, but go on executing the program. For example, Stacked Borrows / validity invariant violations could be turned into a warning. Or we could warn when a memory access is sufficiently aligned just because its integer version is sufficiently aligned -- which might be pure luck on the side of the program.

This could even replace flags like -Zmiri-disable-validation with -Avalidation-failure or so! It would be a uniform way to handle extra checks that we sometimes want and sometimes not. (Though in particular for -Zmiri-disable-validation we also want the performance benefit of not doing all these checks.)

Some thoughts from Zulip on how warnings could be emitted in a reasonable way:

S: Just put it on stderr and prefix it with [MIRI] and you're good as far as un-mixing it goes.
R: not sure if we can make the rustc diagnostic printing stuff do that
S: In that case print only once that miri has more to say and provide a key to print the entire thing

Or maybe we don't want to point to a span here, then we can just eprintln! ourselves directly, an we can easily manage the allow/warn/error flags.

S: I think the information about the situation presented to the user can be summarized as "MIRI has detected behavior that might depend on memory addresses chosen at runtime. You may need to re-run it several times to detect an error." which is a single print and should only be done once

@RalfJung RalfJung added A-ux Area: This affects the general user experience C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Jun 28, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Aug 2, 2019

Cc rust-lang/rust#62420

@RalfJung RalfJung added C-project Category: a larger project is being tracked here, usually with checkmarks for individual steps and removed C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Nov 5, 2019
bors added a commit that referenced this issue Dec 10, 2019
Add a scheme to find the place where an id was destroyed

cc #974

I'm not too happy with it, but since stacked borrows don't have access to the current call stack, I can't just report a warning as per #797

We could add some global mutex that we can throw strings at and `step` will clear out that mutex and report warnings before moving the `statement_id` or the `block_id`, not sure how well that would work. For now I think this is sufficient
@oli-obk
Copy link
Contributor

oli-obk commented Dec 15, 2019

If we want to emit backtraces for warnings, we'll have to fiddle some information through AllocExtra. One possible API would be to pass an impl Fn(InterpErrorInfo<'tcx>), so we don't expose the InterpCx to stacked borrows directly.

Less intrusive would be to have a global store of things to report that is filled before going into stacked borrows and that stacked borrows can push things into. This is similar to tls::tcx in the compiler.

@RalfJung
Copy link
Member Author

If we want to emit backtraces for warnings, we'll have to fiddle some information through AllocExtra

I assume that is specifically for parts of the code that don't have access to the ecx?

Less intrusive would be to have a global store of things to report that is filled before going into stacked borrows and that stacked borrows can push things into. This is similar to tls::tcx in the compiler.

That sounds reasonable. Also I wouldn't want full backtraces by default anyway, that seems excessive -- but it seems like a reasonable option.

bors added a commit that referenced this issue Jan 9, 2020
Add a scheme for emitting errors without halting interpretation

cc #797
@RalfJung
Copy link
Member Author

With #1127 we have the basis for a scheme like that, but there is no good "lint management" yet. Right now Miri only emits warnings/notices on explicit request, so this is fine; if we ever want to warn about something by default (like if we adapt #1074 as default behavior), we have to improve.

@RalfJung
Copy link
Member Author

In #1363, @Robbepop made the great proposal to also use the scopes of the currently running code to cotnrol Miri error reporting:

fn foo() {
    // code that is checked with default settings
    #[miri::warn(stacked_borrows)]
    {
        // code that is checked with stacked borrows turned into warnings instead of errors
    }
    // code is checked with default settings at this point again
}

One interesting question here is whether this should take into account the call stack. Like, should the following code warn or error about stacked borrows during the execution of bar?

fn bar() { ... }

#[miri::warn(stacked_borrows)]
fn foo() { bar() }

I think the answer is "yes the call that should be taken into account" and "it should warn". That seems most consistent. But it is pretty different from how rustc/clippy use the lint system, so it could be challenging to implement.

We do have MIR soruce info for every stack frame, so we can iterate up that frame and... well, maybe somehow get the lint levels from that source info?

@bjorn3
Copy link
Member

bjorn3 commented Apr 25, 2020

We do have MIR soruce info for every stack frame, so we can iterate up that frame and... well, maybe somehow get the lint levels from that source info?

Yes, but I think only for the current crate. SourceInfo.scope -> SourceScope.local_data.lint_root. (SourceScope.local_data is ClearCrossCrate)

@frengor
Copy link

frengor commented May 21, 2022

It would be great to have this feature! It would also be useful to have some additional attributes:

  • #[miri::ignore(leaks)], which ignores errors without printing anything to the console. In this example, ignores leaks since we might be leaking on purpose
  • #[miri::expect(leaks)], which makes the test fails with an error if something doesn't happen. Obviously, when an UB happens, the execution of the test is always halted, but if we're expecting it the test should be successful. In this example, fails if a leak doesn't happen, which might be useful to test a certain corner-behaviours of your unsafe code, like a panic in safe code called from unsafe code

@RalfJung
Copy link
Member Author

For the former, we do have -Zmiri-ignore-leaks. Attributes would be more elegant indeed, but the problem is that this is a global toggle, whereas attributes are attached to a piece of code, so this does not quite match up very well.

@frengor
Copy link

frengor commented May 21, 2022

the problem is that this is a global toggle, whereas attributes are attached to a piece of code

Yeah, this is the problem indeed. However, it would also be super-useful to be able to fail a test if a UB doesn't happen. Is there any possibility of seeing this added in the future?

@RalfJung
Copy link
Member Author

The trouble is, Miri doesn't even have the concept of a test. Miri just runs a binary. That that binary happens to be a test runner is entirely oblivious to Miri.

@frengor
Copy link

frengor commented May 23, 2022

The trouble is, Miri doesn't even have the concept of a test. Miri just runs a binary. That that binary happens to be a test runner is entirely oblivious to Miri.

Oh, I see the problem here. Since there is the possibility to annotate a test with #[should_panic], wouldn't it be possible to make something like #[miri::panic(leaks)] instead of #[miri::expect(leaks)]?
(or even #[miri::panic(leaks, message="custom panic message")])

So using it will be like:

#[test]
#[should_panic(expected = "oh no leaks")]
fn foo() {
    #[miri::panic(leaks, message="oh no leaks")]
    {
        // code that leaks
    } // Here we check for leaks
}

@RalfJung
Copy link
Member Author

RalfJung commented May 23, 2022

A "leak" is defined as "after the program finished executing, there is still some memory left that has not been deallocated".

So it is not even clear what it means for a single function to have a leak. Presumably something involving "allocations done by this function or its callees", but (a) we don't have any infrastructure to track that, and (b) what if a function spawns another thread that keeps running; do the allocations made by that thread count? What if the thread is joined at some point?

Basically what I am saying is, someone needs to do a bunch of design work to even figure out what it should mean to apply such an attribute to a function. It would be useful, but it is far from easy. :)

Since there is the possibility to annotate a test with #[should_panic]

Miri doesn't know this attribute. That is handled by the test runner in libtest and it translates to what the test runner does after detecting (via catch_unwind) that a test panicked.

As I said, Miri just runs the test runner as it would any other main function you might have written yourself.

@frengor
Copy link

frengor commented May 23, 2022

Sorry, I expressed myself badly 😅 I used leaks for my example since I used them in my first message. This might be easier with other UBs, like accessing freed memory: if freed memory is accessed and there is #[miri::panic(freed-memory-access)], then panic. This way Miri doesn't have to know #[should_panic], it just panics. This might also cause bugs though, and there's still the problem of knowing where #[miri::panic(..)] attributes apply.

it is far from easy. :)

I do agree on that, this needs some design work.

However, in the case of leaks, I don't know if this is even possible, given your example.

@RalfJung
Copy link
Member Author

Leaks are not UB. :)

We do have a flag to turn some "unsupported" messages into a panic. Presumably we could do something similar for some kinds of UB, though (a) the code might be written assuming (correctly) that memory accesses do not panic, and (b) how to best do this without making our code harder to maintain -- I do not know.

@frengor
Copy link

frengor commented May 24, 2022

Leaks are not UB. :)

The fun part is that I knew it and decided to use the term UB for leaks anyway 🙃. I definitely need more sleep.

(a) the code might be written assuming (correctly) that memory accesses do not panic, and (b) how to best do this without making our code harder to maintain -- I do not know.

Yeah, I also think that's the problem. Maybe something like the following might works (?)

fn panic_on_ub<F, R>(f: F) -> R
where
    F: FnOnce() -> R,
{
    // Miri built-in which panics, but still have problem (a) since panic_on_ub returns R
}

// Or even
fn try_and_report_ub<F, R>(f: F) -> R
where
    F: FnOnce() -> Result<R, UBType>,
{
    // Miri built-in which doesn't have problem (a) since it returns a Result instead of panicking
}

(names should be definitely improved)

The problem is that this way Miri would have to add library functions, but at least now it would know what to do in case of UB.

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-project Category: a larger project is being tracked here, usually with checkmarks for individual steps
Projects
None yet
Development

No branches or pull requests

4 participants