Closed zhassan-aws closed 2 years ago
There is a bunch of test cases from the "Rust by Example" book that show similar behaviors:
Rust by Example/Error handling/Multiple error types/Boxing errors/11.rs
Rust by Example/Error handling/Multiple error types/Defining an error type/18.rs
Rust by Example/Error handling/Multiple error types/Other uses of ?/25.rs
Rust by Example/Error handling/Multiple error types/Pulling Results out of Options/34.rs
Rust by Example/Error handling/Multiple error types/Pulling Results out of Options/6.rs
Rust by Example/Error handling/Multiple error types/Wrapping errors/5.rs
Rust by Example/Error handling/Option & unwrap/Combinators: and_then/15.rs
Rust by Example/Error handling/Option & unwrap/Combinators: map/15.rs
Rust by Example/Error handling/Option & unwrap/Unpacking options with ?/19.rs
Rust by Example/Error handling/Result/64.rs
Rust by Example/Error handling/Result/Early returns/11.rs
Rust by Example/Error handling/Result/Introducing ?/16.rs
Rust by Example/Error handling/Result/Introducing ?/46.rs
Rust by Example/Error handling/Result/aliases for Result/14.rs
Rust by Example/Error handling/Result/map for Result/15.rs
Rust by Example/Error handling/Result/map for Result/54.rs
Adding a high priority label to this since reasoning correctly about Result
types (or Enum
in general) is a must.
This disappeared with Result<u32, MyError>
, right? As a result, I strongly suspect we're mishandling the niche optimization somewhere.
We might want to get more thorough about tests surrounding that feature.
Correct, replacing ()
with u32
makes the failures disappear.
I am currently investigating if this is an issue with how we handle discriminant.
I see that our own tests have a --no-overflow-checks
to avoid the overflow issue. See https://github.com/model-checking/rmc/blob/main/src/test/rmc/SwitchInt/main.rs#L7 for an example.
I believe the issue is related to how we compute relative discriminant here: https://github.com/model-checking/rmc/blob/fdf86e5f071a0474ad5bc2f3f5594a1a403dbb9d/compiler/rustc_codegen_rmc/src/codegen/rvalue.rs#L473
I checked the code and the computation itself is correct. The overflow is per design. In this specific case, the compiler encodes Result<(), MyError>
as a u8
where:
The compiler will subtract 2 out of the value to check if it's None or not.
The issue here is that we don't handle wrapping arithmetic operations correctly. The following code will trigger an overflow failure even though users are explicitly saying it's ok:
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR MIT
//
// Check that none of these operations trigger spurious overflow checks.
pub fn main() {
let a: u8 = rmc::nondet();
let b: u8 = rmc::nondet();
a.wrapping_add(b);
}
Looking at CBMC's documentation, it looks like we should be adding pragma to skip overflow checks for statements where wrapping arithmetic is expected: http://www.cprover.org/cprover-manual/properties/
I see. So, this is pretty much a duplicate of #480?
Just wanted to mention that wrapping_add
and other are actually intrinsics which appear to be codegen'd as regular operations at the moment. This is the code in codegen_intrinsic
from compiler/rustc_codegen_rmc/src/codegen/intrinsic.rs
:
"wrapping_add" => codegen_intrinsic_binop!(plus),
"wrapping_mul" => codegen_intrinsic_binop!(mul),
"wrapping_sub" => codegen_intrinsic_binop!(sub),
Is this okay? I would expect these intrinsics to add some logic for handling overflows on their own, similar to what we do for add_with_overflow
and similar intrinsics. @celinval have you tried that already?
Just wanted to mention that
wrapping_add
and other are actually intrinsics which appear to be codegen'd as regular operations at the moment. This is the code incodegen_intrinsic
fromcompiler/rustc_codegen_rmc/src/codegen/intrinsic.rs
:"wrapping_add" => codegen_intrinsic_binop!(plus), "wrapping_mul" => codegen_intrinsic_binop!(mul), "wrapping_sub" => codegen_intrinsic_binop!(sub),
Is this okay? I would expect these intrinsics to add some logic for handling overflows on their own, similar to what we do for
add_with_overflow
and similar intrinsics. @celinval have you tried that already?
RustC actually lowers wrapping_* intrinsics during MIR transformations now and replace them by BinOp::Add.
I was initially thinking about using CBMC pragmas to remove the overflow checks from wrapping operations. However, rustc already add assertions to check for overflow by default for non-wrapping operations.
Thus, I'm thinking about disabling cbmc overflow checks since they are redundant. Unless someone can think about a scenario that it's actually adding value.
I see. So, this is pretty much a duplicate of #480?
Slightly different issue but same underlying cause. We treat all arithmetic operations as non-wrapping.
I tried this code:
using the following command line invocation:
with RMC version:
7635cfebfa5
I expected to see this happen: no failures
Instead, this happened: got 2 arithmetic overflow failures:
The
- 2
in the check:seems to be coming from the number of values in the enum. The arithmetic overflow checks are not expected in the first place.