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

running miri in release mode should disable debug assertions #2797

Closed
lukas-code opened this issue Feb 22, 2023 · 12 comments · Fixed by #2814
Closed

running miri in release mode should disable debug assertions #2797

lukas-code opened this issue Feb 22, 2023 · 12 comments · Fixed by #2814

Comments

@lukas-code
Copy link
Member

Code

fn main() {
    debug_assert!(false);
}

This currently passes with cargo run --release, but panics with cargo miri run --release.

Since debug assertions can actually change the behavior of a program, I think it would be useful to allow running miri with debug assertions disabled.

@RalfJung
Copy link
Member

RalfJung commented Feb 22, 2023

This is currently a deliberate choice -- unfortunately undocumented. (We should definitely at least fix the docs.) The reasoning is that Miri is anyway so slow that the benefit from the extra sanity checks outweighs the costs. OTOH I do agree that it can be confusing. I think we definitely want to keep overflow checks enabled (their cost in Miri is definitely negligible and this can find real bugs), I am less sure about regular debug assertions (which can actually be quite expensive).

You can force debug assertions to be disabled with MIRIFLAGS="-Cdebug-assertions=off".

@LegionMammal978
Copy link

I just ran into this today when writing a reproducer for a reachable unreachable_unchecked() in cipher (RustCrypto/traits#1275). To be defensive, the crate precedes the unreachable_unchecked() with a debug_assert!(false). I've also seen a few other crates, e.g., parking_lot, that switch between unreachable!() and unreachable_unchecked() based on cfg!(debug_assertions).

Also, RUSTFLAGS="-Cdebug-assertions=off" seems to do the trick, not MIRIFLAGS.

The reasoning is that Miri is anyway so slow that the benefit from the extra sanity checks outweighs the costs.

Soundness can often be sensitive to the particular profile settings. I've seen some crates which are unsound only when overflow_checks are disabled, due to an unexpected overflow. And I've seen other crates which are unsound only when overflow_checks are enabled, due to poor unwind safety. It can easily make these edge cases very confusing to debug when Miri's behavior differs from the compiler's default behavior, given the same profile settings in Cargo.toml.

@saethlin
Copy link
Member

saethlin commented Mar 8, 2023

So are you advocating for Miri not setting any values here? What do you think about this consideration: #2497

@LegionMammal978
Copy link

I'm fine with Miri setting whatever it wants for std, core, etc.; it's always taken special incantations to fiddle with their RUSTFLAGS, and their behavior is unlikely to change much between them (unless the standard library is what is being debugged). What I'm more concerned about is the RUSTFLAGS of the user crate being compiled, since changes to the profile can sometimes have far more important effects to their behavior and control flow.

@RalfJung
Copy link
Member

RalfJung commented Mar 9, 2023

I just ran into this today when writing a reproducer for a reachable unreachable_unchecked() in cipher (RustCrypto/traits#1275). To be defensive, the crate precedes the unreachable_unchecked() with a debug_assert!(false). I've also seen a few other crates, e.g., parking_lot, that switch between unreachable!() and unreachable_unchecked() based on cfg!(debug_assertions).

There's no problem here though, is there? Reaching the unreachable will still be reported.

Soundness can often be sensitive to the particular profile settings. I've seen some crates which are unsound only when overflow_checks are disabled,

With overflow checks enabled, those crates then raise panics, which are still clearly pointing out there is a bug.

It can easily make these edge cases very confusing to debug when Miri's behavior differs from the compiler's default behavior, given the same profile settings in Cargo.toml.

Yeah that is fair, I can see how this can be baffling when it comes unexpected. So it's about trading off the extra test coverage from having debug assertions and overflow checks enabled in more cases, against this element of surprise.

@LegionMammal978
Copy link

LegionMammal978 commented Mar 9, 2023

There's no problem here though, is there? Reaching the unreachable will still be reported.

Only once you examine the stack trace to find that it's the maybe-unchecked unreachable that you were looking for, and not some other debug_assert!() littered throughout the library.

With overflow checks enabled, those crates then raise panics, which are still clearly pointing out there is a bug.

In general, crates don't guarantee correct behavior following a panic, so you can end up hitting a bunch of overflows and the like from trying to use the broken state. But powering through them can be necessary to demonstrate that the broken state is truly unsound. (For instance, sometimes a data structure can be in a broken-but-still-sound state following a panic, but then it can enter a truly unsound state following another panic. To expose things like that, sometimes you need to panic in the right place.)

Also, sometimes there's "nonsense" states, like with semantically incorrect implementations of traits (e.g., my example with a cryptographic block size of 0). These can rightly cause integer overflows and other incorrect behavior, but only violate an unsafe invariant further down the line.

That is to say, not all incorrect behavior is necessarily a bug, if the library does not define any correct behavior for the given input. But it's important that this expected incorrect behavior does not make the library unsound.

Yeah that is fair, I can see how this can be baffling when it comes unexpected. So it's about trading off the extra test coverage from having debug assertions and overflow checks enabled in more cases, against this element of surprise.

If someone wanted that, couldn't they just run Miri without --release? Obviously, --release can't make Miri go any faster, so I don't know what else a user could expect from passing it, apart from disabling debug assertions and overflow checks as per the profile. When I tell Miri to run --release, intuitively I'd expect that to mean "simulate the program's behavior as if you used build --release".

@RalfJung
Copy link
Member

RalfJung commented Mar 9, 2023

Obviously, --release can't make Miri go any faster

That might seem obvious to you but I don't think it is obvious in general. cargo miri run roughly means "please find some bugs" and one could reasonably expect that if Miri finds out-of-bounds accesses it can also find overflows, no matter the flags being passed later.

Anyway I explained my rationale above and that I think this is a trade-off. I am not dead set on the current behavior, but it's also not entirely clear to me that we want to change anything (except for the docs). I wonder what @saethlin and @oli-obk think.

@LegionMammal978
Copy link

LegionMammal978 commented Mar 9, 2023

cargo miri run roughly means "please find some bugs"

I suppose that's where we differ. To me, "please find some ordinary bugs" is cargo run: that will already find everying that would be caught by debug assertions or overflow checks. I'd only use cargo miri run when I'm doing something tricky with atomics, raw pointers, etc., and I specifically want to find unexpected UB (or confirm suspected UB) that cargo run doesn't catch. I suppose that might change in the future, if Miri gains more options for simulating programs under different environments, but in the meantime I just can't really see it as a generic "bug-finding" tool separate from its focus on UB.

@saethlin
Copy link
Member

saethlin commented Mar 9, 2023

My opinion is that Miri should do the least-surprising thing. Perhaps that means we should issue a warning when a user passes --release, because I have definitely seen users try passing that flag before when they realize Miri is too slow, in the hopes that Miri will still look for UB while also applying optimizations.

Maybe we could expand that to detecting if the selected profile has any non-zero opt-level 🤔

@RalfJung
Copy link
Member

I have definitely seen users try passing that flag before when they realize Miri is too slow, in the hopes that Miri will still look for UB while also applying optimizations.

I assume they didn't realize that applying optimizations means missing some UB? We certainly can't stop passing -Zmir-opt-level=0...

@saethlin
Copy link
Member

saethlin commented Mar 12, 2023

I assume they didn't realize that applying optimizations means missing some UB?

Yes, I think it is likely that they did not think through that, and that is why a sentence or two of warning could be the right thing here. At least in my experience, tools like ASan and UBsan are often used with optimizations and tend to find just as many bugs with optimizations on and off.

So I suggest we should check the optimization level, and if it is not set to off, warn the user that optimizations will hide UB and therefore Miri is not doing them.

@RalfJung
Copy link
Member

FWIW Miri could totally run with optimizations and would still find at least as many bugs as ASan and UBSan. But I don't think we want to do that.

@bors bors closed this as completed in 95c583c Mar 18, 2023
saethlin pushed a commit to saethlin/rust that referenced this issue Apr 11, 2023
Emit a warning when Miri is used with optimizations

This may address rust-lang/miri#2797, by clarifying to the user what is going on and what the consequences of their choices are.

Fixes rust-lang/miri#2797
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants