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

UI fix : Fix missing function check descriptions #1126

Merged
merged 7 commits into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 33 additions & 9 deletions scripts/cbmc_json_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ class GlobalMessages(str, Enum):
REACH_CHECK_DESC = "[KANI_REACHABILITY_CHECK]"
REACH_CHECK_KEY = "reachCheckResult"
CHECK_ID = "KANI_CHECK_ID"
ASSERTION_FALSE = "assertion false"
DEFAULT_ASSERTION = "assertion"
CHECK_ID_RE = CHECK_ID + r"_.*_([0-9])*"
UNSUPPORTED_CONSTRUCT_DESC = "is not currently supported by Kani"
UNWINDING_ASSERT_DESC = "unwinding assertion loop"
Expand Down Expand Up @@ -273,6 +275,22 @@ def extract_solver_information(cbmc_response_json_array):

return properties, solver_information

def modify_undefined_function_checks(properties):
"""
1. Searches for any check which has unknown file location or missing defition and replaces description
2. If a function with missing definition is reachable, then we turn all SUCCESS status to UNDETERMINED.
If there are no reachable functions with missing definitions, then the verification is not affected, so we retain all of the SUCCESS status.
"""
has_unknown_location_checks = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this has_reachable_undefined_functions.

for property in properties:
# Specifically trying to capture assertions that CBMC generates for functions with missing definitions
if GlobalMessages.ASSERTION_FALSE in property["description"] and extract_property_class(
Copy link
Contributor

@zhassan-aws zhassan-aws Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment that this logic is intended to capture assertions that CBMC generates for functions with missing definitions?

One way to check whether this logic matches other checks is to run the regressions without the --generate-function-body-options assert-false-assume-false that we pass to goto-instrument (which would disable the undefined function checks) and see if any other checks are captured by this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After running the suggested experiment, no unexpected tests were found to change their result so we can safely assume that this logic doesn't capture false positives.

property) == GlobalMessages.DEFAULT_ASSERTION and not hasattr(property["sourceLocation"], "file"):
property["description"] = "Function with missing definition is unreachable"
if property["status"] == "FAILURE":
has_unknown_location_checks = True
return has_unknown_location_checks

def extract_errors(solver_information):
"""
Extract errors from the CBMC output, which are messages that have the
Expand Down Expand Up @@ -309,6 +327,7 @@ def postprocess_results(properties, extra_ptr_check):

has_reachable_unsupported_constructs = has_check_failure(properties, GlobalMessages.UNSUPPORTED_CONSTRUCT_DESC)
has_failed_unwinding_asserts = has_check_failure(properties, GlobalMessages.UNWINDING_ASSERT_DESC)
has_reachable_undefined_functions = modify_undefined_function_checks(properties)
properties, reach_checks = filter_reach_checks(properties)
annotate_properties_with_reach_results(properties, reach_checks)
remove_check_ids_from_description(properties)
Expand All @@ -318,7 +337,7 @@ def postprocess_results(properties, extra_ptr_check):

for property in properties:
property["description"] = get_readable_description(property)
if has_reachable_unsupported_constructs or has_failed_unwinding_asserts:
if has_reachable_unsupported_constructs or has_failed_unwinding_asserts or has_reachable_undefined_functions:
# Change SUCCESS to UNDETERMINED for all properties
if property["status"] == "SUCCESS":
property["status"] = "UNDETERMINED"
Expand Down Expand Up @@ -481,17 +500,25 @@ def replace(self, msg):
}


def extract_property_class(prop):
"""
This function extracts the property class from the property string.
Property strings have the format of -([<function>.]<property_class_id>.<counter>)
"""
prop_class = prop["property"].rsplit(".", 3)
# Do nothing if prop_class is diff than cbmc's convention
class_id = prop_class[-2] if len(prop_class) > 1 else None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we warn/log this error?

Copy link
Contributor Author

@jaisnan jaisnan Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in some developer log but to the user, this error should make no difference at all. This convention/tag schema is mostly for our consumption at the end of it.

return class_id


def filter_ptr_checks(props):
"""This function will filter out extra pointer checks.

Our support to primitives and overflow pointer checks is unstable and
can result in lots of spurious failures. By default, we filter them out.
"""
def not_extra_check(prop):
""" Retrieve class id ([<function>.]<property_class_id>.<counter>)"""
prop_class = prop["property"].rsplit(".", 3)
class_id = prop_class[-2] if len(prop_class) > 1 else None
return class_id not in ["pointer_arithmetic", "pointer_primitives"]
return extract_property_class(prop) not in ["pointer_arithmetic", "pointer_primitives"]

return list(filter(not_extra_check, props))

Expand All @@ -503,10 +530,7 @@ def get_readable_description(prop):
temporary variable.
"""
original = prop["description"]
# Id is structured as [<function>.]<property_class>.<counter>
prop_class = prop["property"].rsplit(".", 3)
# Do nothing if prop_class is diff than cbmc's convention
class_id = prop_class[-2] if len(prop_class) > 1 else None
class_id = extract_property_class(prop)
if class_id in CBMC_DESCRIPTIONS:
# Contains a list for potential message translation [String].
prop_type = [check.replace(original) for check in CBMC_DESCRIPTIONS[class_id] if check.matches(original)]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
Status: UNREACHABLE\
Status: UNDETERMINED\
Description: "assertion failed: x == 5"

VERIFICATION:- FAILED

3 changes: 3 additions & 0 deletions tests/ui/missing-function/replaced-description/expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.assertion.1\
Status: FAILURE\
Description: "Function with missing definition is unreachable"
11 changes: 11 additions & 0 deletions tests/ui/missing-function/replaced-description/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR MIT

// This test is to check if the description for undefined functions has been updated to "Function with missing definition is unreachable"

#[kani::proof]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, comment what is this test testing

fn main() {
let x = String::from("foo");
let y = x.clone();
assert_eq!("foo", y);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.assertion.1\
Status: FAILURE\
Description: "Function with missing definition is unreachable"
16 changes: 16 additions & 0 deletions tests/ui/missing-function/rust-by-example-description/main.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0 OR MIT

// kani-flags: --enable-unstable --cbmc-args --unwind 4 --object-bits 9
// This test is to check if the description for undefined functions has been updated to "Function with missing definition is unreachable"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining what this test is testing?

#![allow(unused)]
#[kani::proof]
pub fn main() {
let strings = vec!["tofu", "93", "18"];
let numbers: Vec<_> = strings
.into_iter()
.filter_map(|s| s.parse::<i32>().ok())
.collect();
println!("Results: {:?}", numbers);
}