-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add a test for breakpoint
#1022
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make this test a bit stronger?
tests/kani/Intrinsics/breakpoint.rs
Outdated
#[kani::proof] | ||
fn main() { | ||
unsafe { std::intrinsics::breakpoint() }; | ||
assert!(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add something more meaningful and maybe move this to the expected test suite to ensure it is reachable?
If I'm not wrong, assert!(true)
doesn't even get codegen. So this test would still succeed even if the breakpoint was translated to a return statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert!(true)
does codegen, and in fact produces the only check:
RESULTS:
Check 1: main.assertion.1
- Status: SUCCESS
- Description: "assertion failed: true"
- Location: breakpoint.rs:11:5 in function main
I'm okay with moving this into expected
, but I'm not sure how that ensures it is reachable if I remove the assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this works now because we override assert!()
. Before that, this code would be pruned by the compiler. Good to know! :)
In this case, you can just check that this assertion is reachable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted to an expected
test. Both approaches seem equivalent to me (if the assertion were not reachable, verification would fail), but if anything goes wrong I'm sure expected
will complain before 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, verification succeeds if there are no failures (even if a check is unreachable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, thanks! Then I take that back, expected
is the best option here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
* Add a test for `breakpoint` * Convert into an `expected` test * Update comment on test
* Add a test for `breakpoint` * Convert into an `expected` test * Update comment on test
* Add a test for `breakpoint` * Convert into an `expected` test * Update comment on test
* Add a test for `breakpoint` * Convert into an `expected` test * Update comment on test
Description of changes:
Adds a test for
breakpoint
. It checks that is supported (breakpoint
generates aSKIP
statement)and that it does not affect the verification result by adding an
assert!(true)
after it.If you have ideas on how to test "skip" intrinsics, please let me know.
Resolved issues:
Part of #727
Call-outs:
Testing:
How is this change tested? Adds one test.
Is this a refactor change? N/A
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.