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

debug_assert is disabled in the standard library due to poor assertion messages #1740

Open
celinval opened this issue Oct 3, 2022 · 5 comments
Assignees
Labels
[C] Feature / Enhancement A new feature request or enhancement to an existing feature. [E] User Experience An UX enhancement for an existing feature. Including deprecation of an existing one.

Comments

@celinval
Copy link
Contributor

celinval commented Oct 3, 2022

The custom sysroot build for the MIR Linker introduced by #1717 builds the standard library in debug mode. This enables a bunch of safety checks and debug assertions from the standard library. Some of these checks are for intrinsics safety checks. The issues here are:

  1. A lot of these checks make our intrinsics checks redundant.
  2. The std checks are added before actually calling the intrinsic and they are not super user friendly.

For example, the following testcase from the expected suite:

#[kani::proof]
fn test_copy_nonoverlapping_unaligned() {
let arr: [i32; 3] = [0, 1, 0];
let src: *const i32 = arr.as_ptr();
// Passing `max_count` is guaranteed to overflow
// the count in bytes for `i32` pointers
let max_count = usize::MAX / 4 + 1;
unsafe {
let dst = src.add(1) as *mut i32;
core::intrinsics::copy_nonoverlapping(src, dst, max_count);
}
}

Without the MIR Linker, it fails with the following error:

Failed Checks: copy_nonoverlapping: attempt to compute number in bytes which would overflow

With the MIR Linker, the error is:

Failed Checks: called `Option::unwrap()` on a `None` value 

This happens because of the std check is_nonoverlapping which has a call to checked_mul().unwrap(). The checked_mul() is None in the overflow scenario.

@celinval
Copy link
Contributor Author

celinval commented Oct 3, 2022

Another interesting one is that the checks are not encoded as normal debug_assert!() which panic!(), they call abort instead to optimize the compilation size. So other failures will look like:

Failed Checks: reached intrinsic::abort

celinval added a commit to celinval/kani-dev that referenced this issue Oct 17, 2022
This is a possible workaround for now to improve the UX around UB
detection in intrinsics when using the MIR Linker. The issue is tracked
here:
model-checking#1740

Basically, the rust toolchain uses a release build that removes all
debug assertions from the standard library. When we switched to using
our custom build of the `std` we decided to enable the debug assertions
in order to catch more potential UBs. However, the UX is not always
great. The assertions don't have any clear descriptions and they may
fail in unexpected places. E.g.: The violation of an intrinsic safety
condition triggers the following failures:
```
> Failed checks: Called `Option::unwrap()` on a `None` value
```
adpaco-aws pushed a commit that referenced this issue Oct 18, 2022
This is a possible workaround for now to improve the UX around UB
detection in intrinsics when using the MIR Linker. The issue is tracked
here:
#1740

Basically, the rust toolchain uses a release build that removes all
debug assertions from the standard library. When we switched to using
our custom build of the `std` we decided to enable the debug assertions
in order to catch more potential UBs. However, the UX is not always
great. The assertions don't have any clear descriptions and they may
fail in unexpected places. E.g.: The violation of an intrinsic safety
condition triggers the following failures:
```
> Failed checks: Called `Option::unwrap()` on a `None` value
```
@celinval celinval self-assigned this Oct 26, 2022
@tedinski tedinski added [C] Feature / Enhancement A new feature request or enhancement to an existing feature. [E] User Experience An UX enhancement for an existing feature. Including deprecation of an existing one. labels Nov 14, 2022
@tedinski tedinski changed the title Intrinsics safety failures are not as friendly with --mir-linker debug_assert is disabled in the standard library due to poor assertion messages Nov 14, 2022
@celinval
Copy link
Contributor Author

The current behavior implemented for MIR Linker keeps the same behavior as before the MIR Linker. Enabling these checks are desirable but they are orthogonal to the MIR Linker work. Hence, I'm removing the link with the MIR Linker milestone.

@cameron1024
Copy link

Hi, I've just run into this issue trying to verify a program that depends on unicode-bidi, which has this snippet:

#[cfg(feature = "std")]
std::debug_assert!(
    false,
    "Found broken indices in level run: found indices {}..{} for string of length {}",
    level_run.start,
    level_run.end,
    text.len()
);

This fails with an error:

error: cannot find macro `__kani__workaround_core_assert` in this scope

I'm very new to kani, so I'm not exactly sure if this is actually the same issue, or just user error. I just stumbled upon this issue when googling.

Are there steps a user can take to work around this error? (Apologies if this is the wrong place to ask these sorts of questions 😅 )

@zhassan-aws
Copy link
Contributor

Thanks for the bug report @cameron1024. Can you please file a separate issue and include information on how to reproduce the error, and the Kani version? Thanks!

@cameron1024
Copy link

Opened #2187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C] Feature / Enhancement A new feature request or enhancement to an existing feature. [E] User Experience An UX enhancement for an existing feature. Including deprecation of an existing one.
Projects
No open projects
Status: No status
Development

No branches or pull requests

4 participants