-
Notifications
You must be signed in to change notification settings - Fork 92
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
Unwind Attribute without Python Changes #846
Changes from 12 commits
9e2e1b1
89092cf
05f83b0
da43716
5ca9856
3a2b8b0
f4cd7ee
3deac4e
d870bbe
52288e3
29b2820
4de691f
f09c7f4
e4e69e9
70f5685
192d189
08a40a3
51b86ae
5715249
ddc97dd
c661d9f
e637963
a04b462
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
// RUSTFLAGS="-Zcrate-attr=feature(register_tool) -Zcrate-attr=register_tool(kanitool)" | ||
|
||
// proc_macro::quote is nightly-only, so we'll cobble things together instead | ||
extern crate proc_macro; | ||
use proc_macro::TokenStream; | ||
|
||
#[cfg(all(not(kani), not(test)))] | ||
|
@@ -36,9 +37,10 @@ pub fn proof(_attr: TokenStream, item: TokenStream) -> TokenStream { | |
|
||
#[cfg(kani)] | ||
#[proc_macro_attribute] | ||
pub fn proof(_attr: TokenStream, item: TokenStream) -> TokenStream { | ||
pub fn proof(attr: TokenStream, item: TokenStream) -> TokenStream { | ||
let mut result = TokenStream::new(); | ||
|
||
assert!(attr.to_string().len() == 0, "#[kani::proof] does not take any arguments"); | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
result.extend("#[kanitool::proof]".parse::<TokenStream>().unwrap()); | ||
// no_mangle is a temporary hack to make the function "public" so it gets codegen'd | ||
result.extend("#[no_mangle]".parse::<TokenStream>().unwrap()); | ||
|
@@ -49,3 +51,27 @@ pub fn proof(_attr: TokenStream, item: TokenStream) -> TokenStream { | |
// $item | ||
// ) | ||
} | ||
|
||
#[cfg(not(kani))] | ||
#[proc_macro_attribute] | ||
pub fn unwind(_attr: TokenStream, _item: TokenStream) -> TokenStream { | ||
// This code is never called by default as the config value is set to kani | ||
TokenStream::new() | ||
} | ||
adpaco-aws marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#[cfg(kani)] | ||
#[proc_macro_attribute] | ||
pub fn unwind(attr: TokenStream, item: TokenStream) -> TokenStream { | ||
celinval marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let mut result = TokenStream::new(); | ||
|
||
// Translate #[kani::unwind(arg)] to #[kanitool::unwind(8)] | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let insert_string = "#[kanitool::unwind(".to_owned() + &attr.clone().to_string() + ")]"; | ||
|
||
// Add the string that looks like - #[kanitool::unwindvalue] | ||
result.extend(insert_string.parse::<TokenStream>().unwrap()); | ||
// No mangle seems to be necessary as removing it prevents all the attributes in a lib from being read | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
result.extend("#[no_mangle]".parse::<TokenStream>().unwrap()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, not anymore. let me update that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update : We do need this. Without it, all unwind annotations without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No mangle has a side effect of making a function public and potentially causing name conflicts (E.g.: rust-lang/rust#28179). BTW, how did you test this behavior? Can you please make sure that the function you are annotating isn't dead code. Feel free to ping me offline. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Talked offline. We'll remove this. |
||
|
||
result.extend(item); | ||
result | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ use crate::GotocCtx; | |
use cbmc::goto_program::{Expr, Stmt, Symbol}; | ||
use cbmc::InternString; | ||
use rustc_ast::ast; | ||
use rustc_ast::{Attribute, LitKind}; | ||
use rustc_middle::mir::{HasLocalDecls, Local}; | ||
use rustc_middle::ty::{self, Instance}; | ||
use std::collections::BTreeMap; | ||
|
@@ -243,16 +244,65 @@ impl<'tcx> GotocCtx<'tcx> { | |
fn handle_kanitool_attributes(&mut self) { | ||
let instance = self.current_fn().instance(); | ||
|
||
// Vectors for storing instances of each attribute type, | ||
// TODO: This can be modifed to use Enums when more options are provided | ||
let mut attribute_vector = vec![]; | ||
let mut proof_attribute_vector = vec![]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, shouldn't this be a boolean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to maintain a count of the instances apart from the attributes themselves, hence vectors. |
||
|
||
// Loop through instances to get all "kanitool::x" attribute strings | ||
for attr in self.tcx.get_attrs(instance.def_id()) { | ||
match kanitool_attr_name(attr).as_deref() { | ||
Some("proof") => self.handle_kanitool_proof(), | ||
_ => {} | ||
// Get the string the appears after "kanitool::" in each attribute string. | ||
// Ex - "proof" | "unwind" etc. | ||
if let Some(attribute_string) = kanitool_attr_name(attr).as_deref() { | ||
// Push to proof vector | ||
if attribute_string == "proof" { | ||
proof_attribute_vector.push(attr); | ||
} | ||
// Push to attribute vector that can be expanded to a map when more options become available | ||
else { | ||
attribute_vector.push((attribute_string.to_string(), attr)); | ||
} | ||
} | ||
} | ||
|
||
// In the case when there's only one proof attribute (correct behavior), create harness and modify it | ||
// depending on each subsequent attribute that's being called by the user. | ||
if proof_attribute_vector.len() == 1 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These cases do not seem to overlap so I am not sure why you are using if-then-else statements here. I would also recommend handling them in small functions if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They do overlap as it's necessary to check to how many proof attributes have been declared per harness and depending on the number, there's different handling that needs to happen. The entire function has a single purpose so it didn't seem necessary to factor it out into smaller functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jaisnan, if I understand this code correctly, you are trying to restrict any other If that is the case, I would recommend to change this to have something like the following structure: let attrs: kani_attributes(self.tcx.get_attrs(instance.def_id())); // BTreeMap<&str, &rustc_ast::Attribute>
if attrs.contains_key("proof") {
// Handle attributes here.
} else {
if attrs.len() > 0 {
// Warning: We currently only support attributes on proof harnesses. The attribute {} will be ignored.
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I am trying to restrict it to kani attribute but also use count information for each of the attributes. I think using a BTreeMap would prevent us from checking if there are duplicate attributes in the same harness or not. If someone calls two We ideally should not just check if a key exists but also how many instances of them. Maybe I can use a multimap? https://docs.rs/btreemultimap/latest/btreemultimap/ ? I think using two vectors and a match statement serves these purposes well enough. If we add more options as attributes, we'd just have to expand the match statement and add a handler and that's it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's talk offline about this one. There are other things we can go to simplify the code. |
||
let mut harness_metadata = self.handle_kanitool_proof(); | ||
if attribute_vector.len() > 0 { | ||
// loop through all subsequent attributes | ||
for attribute_tuple in attribute_vector.iter() { | ||
// match with "unwind" attribute and provide the harness for modification | ||
match attribute_tuple.0.as_str() { | ||
"unwind" => { | ||
self.handle_kanitool_unwind(attribute_tuple.1, &mut harness_metadata) | ||
} | ||
_ => {} | ||
} | ||
} | ||
} | ||
// self.proof_harnesses contains the final metadata that's to be parsed | ||
self.proof_harnesses.push(harness_metadata); | ||
} | ||
// User error handling for when there's more than one proof attribute being called | ||
else if proof_attribute_vector.len() > 1 { | ||
self.tcx | ||
.sess | ||
.span_err(proof_attribute_vector[0].span, "Only one Proof Attribute allowed"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reduce this to warning. |
||
} | ||
// User error handling for when there's an attribute being called without #kani::tool | ||
else if proof_attribute_vector.len() == 0 && attribute_vector.len() > 0 { | ||
self.tcx.sess.span_err( | ||
attribute_vector[0].1.span, | ||
format!("Please use '#kani[proof]' above the annotation {}", attribute_vector[0].0) | ||
.as_str(), | ||
); | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When do we expect to hit this else? Should we trigger an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When neither proof or any other attributes are used. This function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, either remove the else or add a comment to explain what cases it is covering. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure |
||
} | ||
} | ||
|
||
/// Update `self` (the goto context) to add the current function as a listed proof harness | ||
fn handle_kanitool_proof(&mut self) { | ||
fn handle_kanitool_proof(&mut self) -> HarnessMetadata { | ||
let current_fn = self.current_fn(); | ||
let pretty_name = current_fn.readable_name().to_owned(); | ||
let mangled_name = current_fn.name(); | ||
|
@@ -263,20 +313,64 @@ impl<'tcx> GotocCtx<'tcx> { | |
mangled_name, | ||
original_file: loc.filename().unwrap(), | ||
original_line: loc.line().unwrap().to_string(), | ||
unwind_value: None, | ||
}; | ||
|
||
self.proof_harnesses.push(harness); | ||
harness | ||
} | ||
|
||
/// Unwind strings of the format 'unwind_x' and the mangled names are to be parsed for the value 'x' | ||
fn handle_kanitool_unwind(&mut self, attr: &Attribute, harness: &mut HarnessMetadata) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we are planning to eventually support unwind outside of the metadata, I would make this function independent from harness metadata. I.e.: fn handle_unwind(&mut self, attrs: &BTreeMap<&str, &Attribute>) -> Option<UnwindMetadata> {
} BTW, see my other comment about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I understand this comment, if we're putting unwind attributes elsewhere from proof harnesses (imo, still an 'if'), that's going to require a ton of design work, the choices of which we can't really anticipate. This approach seems simple. Trying to anticipate what we might do later is overly complex for the time being. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come? I don't think this change makes the code more complex. I actually think it is cleaner to return a value than modifying the value of an argument variable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh now I see what you mean. I'm not sure if it is cleaner though. The need to give good error messages demands we spot duplicates and things like that. If we try to make this function "cleaner" then we end up pushing a lot of unwind-specific logic (like detecting duplicate unwind attributes) up to the calling function instead of here in the 'unwind' handler. This organization lets that calling function have that nice, clean "loop over kanitool attributes, call handler function" and that's it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyway, not a biggie. Let's move on. |
||
// Check if some unwind value doesnt already exist | ||
if harness.unwind_value.is_none() { | ||
// Get Attribute value and if it's not none, assign it to the metadata | ||
if let Some(integer_value) = extract_integer_argument(attr) { | ||
harness.unwind_value = Some(integer_value); | ||
} else { | ||
// Show an Error if there is no integer value assigned to the integer or there's too many values assigned to the annotation | ||
self.tcx | ||
.sess | ||
.span_err(attr.span, "Exactly one Unwind Argument as Integer accepted"); | ||
} | ||
} else { | ||
// User warning for when there's more than one unwind attribute, in this case, only the first value will be | ||
self.tcx.sess.span_err(attr.span, "Use only one Unwind Annotation per Harness"); | ||
} | ||
} | ||
} | ||
|
||
/// If the attribute is named `kanitool::name`, this extracts `name` | ||
fn kanitool_attr_name(attr: &ast::Attribute) -> Option<String> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this should return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning Option<&str> would return a dangling pointer as the original string would be destroyed once the function returns. We need to return an Owned String so I don't think we can replace the return type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, I didn't notice that you changed the code to join the segments. For the original code, the return value would have the same lifetime as the argument BTW, why did you change it? This code might actually be doing the wrong thing. I.e., let's say you have the following attribute: #[kanitool::type::sub_type] I believe that you are going to return the string There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe so, but I don't see the need to modify it to potential future attributes when there's no clear consensus on what attributes we're planning on supporting. Imo we can always change this later to adapt to future annotations. Let me check about the scenario you've posted however. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Talked offline, this is no longer needed. Going to revert the change. |
||
match &attr.kind { | ||
ast::AttrKind::Normal(ast::AttrItem { path: ast::Path { segments, .. }, .. }, _) | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if segments.len() == 2 && segments[0].ident.as_str() == "kanitool" => | ||
if (!segments.is_empty()) && segments[0].ident.as_str() == "kanitool" => | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
Some(segments[1].ident.as_str().to_string()) | ||
let mut new_string = String::new(); | ||
for index in 1..segments.len() { | ||
new_string.push_str(segments[index].ident.as_str()); | ||
} | ||
Some(new_string) | ||
} | ||
_ => None, | ||
} | ||
} | ||
|
||
/// Extracts the integer value argument from the unwind attribute | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fn extract_integer_argument(attr: &Attribute) -> Option<u128> { | ||
// Vector of meta items , that contain metadata about the annotation | ||
let attr_args = attr.meta().and_then(|x| x.meta_item_list().map(|x| x.to_vec()))?; | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Only accept unwind attributes with one integer value as argument | ||
if attr_args.len() == 1 { | ||
// Returns the integer value that's the argument for the unwind | ||
let x = attr_args[0].literal()?; | ||
match x.kind { | ||
LitKind::Int(y, ..) => Some(y), | ||
_ => None, | ||
} | ||
} | ||
// Return none if there are no unwind attributes or if there's too many unwind attributes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unwind comment in the generic function. |
||
else { | ||
None | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ pub struct HarnessMetadata { | |
pub original_file: String, | ||
/// The line in that file where the proof harness begins | ||
pub original_line: String, | ||
/// Optional data to store unwind value | ||
pub unwind_value: Option<u128>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we agreed having a specific struct for unwind data where this would be an optional value. Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A struct didn't seem necessary for now as we're just targeting a default value for the entire harness. The struct and related changes will be added with the changes to map each loop to it's own harness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe create a type alias here that we can extend later. type UnwindMetadata = usize; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggested There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO stay an integer. Any future unwinding information (e.g. unwindset) can be provided in separate fields. We're never going to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CBMC supports min and max unwind notation as well. But I do agree that if we are going to add a type, we should also change this to Since min / max is an obvious extension here, maybe we should just add the struct and encode this as a map right away. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why any future support for those wouldn't just add an But I don't feel strongly about it. |
||
} | ||
|
||
/// The structure of `.kani-metadata.json` files, which are emitted for each crate | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,4 +9,4 @@ edition = "2021" | |
[dependencies] | ||
|
||
[kani.flags] | ||
output-format = "old" | ||
output-format = "old" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
# SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
||
[package] | ||
name = "simple-unwind-annotation" | ||
version = "0.1.0" | ||
edition = "2021" | ||
|
||
[dependencies] | ||
|
||
[kani.flags] | ||
output-format = "old" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
line 12 assertion failed: 1 == 2: FAILURE |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
||
// rmc-flags: --no-unwinding-checks | ||
|
||
// The expected file presently looks for "1 == 2" above. | ||
// But eventually this test may start to fail as we might stop regarding 'main' | ||
// as a valid proof harness, since it isn't annotated as such. | ||
// This test should be updated if we go that route. | ||
jaisnan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
fn main() { | ||
assert!(1 == 2); | ||
} | ||
|
||
#[kani::proof] | ||
#[kani::unwind(10)] | ||
fn harness() { | ||
let mut counter = 0; | ||
loop { | ||
counter += 1; | ||
assert!(counter < 10); | ||
} | ||
} | ||
|
||
#[kani::proof] | ||
fn harness_2() { | ||
let mut counter = 0; | ||
loop { | ||
counter += 1; | ||
assert!(counter < 10); | ||
} | ||
} | ||
|
||
// NOTE: These are potentially all scenarios that produce user errors. Uncomment each harness to test how the user error | ||
// looks like. | ||
|
||
// #[kani::proof] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this commented code included? If there is a reason to, add a comment explaining why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change these to be UI tests that we ensure that Kani prints the correct error message? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
// #[kani::unwind(10,5)] | ||
// fn harness_3() { | ||
// let mut counter = 0; | ||
// loop { | ||
// counter += 1; | ||
// assert!(counter < 10); | ||
// } | ||
// } | ||
|
||
// #[kani::unwind(8)] | ||
// fn harness_4() { | ||
// let mut counter = 0; | ||
// for i in 0..7 { | ||
// counter += 1; | ||
// assert!(counter < 5); | ||
// } | ||
// } | ||
|
||
// #[kani::proof] | ||
// #[kani::proof] | ||
// fn harness_5() { | ||
// let mut counter = 0; | ||
// loop { | ||
// counter += 1; | ||
// assert!(counter < 10); | ||
// } | ||
// } | ||
|
||
// #[kani::proof(some, argument2)] | ||
// fn harness_6() { | ||
// let mut counter = 0; | ||
// loop { | ||
// counter += 1; | ||
// assert!(counter < 10); | ||
// } | ||
// } | ||
|
||
// // #[kani::unwind(9)] | ||
// // fn harness_7() { | ||
// // let mut counter = 0; | ||
// // for i in 0..10 { | ||
// // counter += 1; | ||
// // assert!(counter < 8); | ||
// // } | ||
// // } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
||
// rmc-flags: --no-unwinding-checks | ||
|
||
// Added to fix me because there are no tests for the annotations themselves. | ||
// TODO : The unwind value needs to be parsed and the unwind needs to be applied to each harness, we do not test this behavior yet. | ||
|
||
fn main() { | ||
assert!(1 == 2); | ||
} | ||
|
||
#[kani::proof] | ||
#[kani::unwind(10)] | ||
fn harness() { | ||
let mut counter = 0; | ||
loop { | ||
counter += 1; | ||
assert!(counter < 10); | ||
} | ||
} |
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.
was this necessary?