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

Panic and abort should stop the program execution #543

Closed
celinval opened this issue Oct 7, 2021 · 4 comments · Fixed by #687
Closed

Panic and abort should stop the program execution #543

celinval opened this issue Oct 7, 2021 · 4 comments · Fixed by #687
Assignees
Labels
[C] Bug This is a bug. Something isn't working.

Comments

@celinval
Copy link
Contributor

celinval commented Oct 7, 2021

I tried this code:

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR MIT
//
// rmc-verify-fail

pub fn main() {
    for i in 0..4 {
        if i == 1 {
            panic!("This comes first");
        }
        if i == 2 {
            panic!("This should never happen");
        }
    }
}

using the following command line invocation:

rmc loop.rs

I expected to see this happen: The first assertion should fail but the second one should be unreachable. The panic call should abort the execution in the second iteration of the loop.

Instead, this happened: Both assertions fail. The trace for the second assertion includes the first panic call.

Note that the same behavior happens if we replace the panic!() statement by process::abort().

@celinval celinval added the [C] Bug This is a bug. Something isn't working. label Oct 7, 2021
@celinval celinval self-assigned this Oct 7, 2021
@celinval
Copy link
Contributor Author

An easy fix right now would be to condegen these failures as:

assert(cond);
assume(cond);

However, this is not actually sound since we are ignoring the unwind path. I created #545 to handle unwind logic.

@zhassan-aws
Copy link
Contributor

This is also how CBMC behaves. For example, for:

#include <assert.h>

int main() {
    int x = 5;
    assert(x < 3);
    assert(x < 4);
    return 0;
}

Both assertions fail:

** Results:
test.c function main
[main.assertion.1] line 5 assertion x < 3: FAILURE
[main.assertion.2] line 6 assertion x < 4: FAILURE

** 2 of 2 failed (2 iterations)
VERIFICATION FAILED

Perhaps we can add an option, e.g. --assert-fail-aborts that changes to the behavior you described? We can document that using this option could lead to missing assertion failures during stack unwinding.

@celinval
Copy link
Contributor Author

celinval commented Dec 1, 2021

I think we should always abort for user asserts, since that is the semantics of assert in rust. Note that if you add an assert!(false); or a panic statement, the rust compiler considers everything else that is dominated by that statement as dead code and removes it.

For debug_assert and other checks that we add, we should not abort since the code after the failure may still reachable during a release execution.

@celinval
Copy link
Contributor Author

celinval commented Dec 1, 2021

Note that assertions in C have different semantics since they can be enabled / disabled according to the value of NDEBUG. https://en.cppreference.com/w/cpp/error/assert

celinval added a commit to celinval/kani-dev that referenced this issue Dec 10, 2021
I refactored how we codegen panic statements so it now it terminate the
program per its spec. I have also removed the hack we had to try to get
the assert location.

Since we currently do not support stack unwinding, the panic codegen
will always terminate immediately and it will not try to unwind the stack.
I added an option to RMC to force the compiler to use abort as the panic
strategy and a check to rmc codegen that will fail if the user tries to
override that.

Another note is that we currently do not support `#[track_caller]` and I
have not changed that.

This change fixes #67, fixes model-checking#466, fixes model-checking#543, and fixes model-checking#636. This
change also mitigates model-checking#545.
celinval added a commit that referenced this issue Dec 14, 2021
I refactored how we codegen panic statements so it now it terminate the
program per its spec. I have also removed the hack we had to try to get
the assert location.

Since we currently do not support stack unwinding, the panic codegen
will always terminate immediately and it will not try to unwind the stack.
I added an option to RMC to force the compiler to use abort as the panic
strategy and a check to rmc codegen that will fail if the user tries to
override that.

Another note is that we currently do not support `#[track_caller]` and I
have not changed that.

This change fixes #67, fixes #466, fixes #543, and fixes #636. This
change also mitigates #545.
tedinski pushed a commit to tedinski/rmc that referenced this issue Apr 22, 2022
I refactored how we codegen panic statements so it now it terminate the
program per its spec. I have also removed the hack we had to try to get
the assert location.

Since we currently do not support stack unwinding, the panic codegen
will always terminate immediately and it will not try to unwind the stack.
I added an option to RMC to force the compiler to use abort as the panic
strategy and a check to rmc codegen that will fail if the user tries to
override that.

Another note is that we currently do not support `#[track_caller]` and I
have not changed that.

This change fixes model-checking#67, fixes model-checking#466, fixes model-checking#543, and fixes model-checking#636. This
change also mitigates model-checking#545.
tedinski pushed a commit to tedinski/rmc that referenced this issue Apr 25, 2022
I refactored how we codegen panic statements so it now it terminate the
program per its spec. I have also removed the hack we had to try to get
the assert location.

Since we currently do not support stack unwinding, the panic codegen
will always terminate immediately and it will not try to unwind the stack.
I added an option to RMC to force the compiler to use abort as the panic
strategy and a check to rmc codegen that will fail if the user tries to
override that.

Another note is that we currently do not support `#[track_caller]` and I
have not changed that.

This change fixes model-checking#67, fixes model-checking#466, fixes model-checking#543, and fixes model-checking#636. This
change also mitigates model-checking#545.
tedinski pushed a commit to tedinski/rmc that referenced this issue Apr 26, 2022
I refactored how we codegen panic statements so it now it terminate the
program per its spec. I have also removed the hack we had to try to get
the assert location.

Since we currently do not support stack unwinding, the panic codegen
will always terminate immediately and it will not try to unwind the stack.
I added an option to RMC to force the compiler to use abort as the panic
strategy and a check to rmc codegen that will fail if the user tries to
override that.

Another note is that we currently do not support `#[track_caller]` and I
have not changed that.

This change fixes model-checking#67, fixes model-checking#466, fixes model-checking#543, and fixes model-checking#636. This
change also mitigates model-checking#545.
tedinski pushed a commit that referenced this issue Apr 27, 2022
I refactored how we codegen panic statements so it now it terminate the
program per its spec. I have also removed the hack we had to try to get
the assert location.

Since we currently do not support stack unwinding, the panic codegen
will always terminate immediately and it will not try to unwind the stack.
I added an option to RMC to force the compiler to use abort as the panic
strategy and a check to rmc codegen that will fail if the user tries to
override that.

Another note is that we currently do not support `#[track_caller]` and I
have not changed that.

This change fixes #67, fixes #466, fixes #543, and fixes #636. This
change also mitigates #545.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C] Bug This is a bug. Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants