-
Notifications
You must be signed in to change notification settings - Fork 103
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
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 give some before/after examples of the UI
// SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
||
// kani-flags: --enable-unstable --cbmc-args --unwind 4 --object-bits 9 | ||
|
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 a comment explaining what this test is testing?
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
||
#[kani::proof] |
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.
Same here, comment what is this test testing
""" | ||
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 |
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.
Should we warn/log this error?
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.
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.
1. Searches for any check which has unknown file location or missing defition and replaces description | ||
2. If any of these checks has a failure status, then we turn all the sucesses into undetermined. | ||
""" | ||
has_unknown_location_checks = False |
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.
I would call this has_reachable_undefined_functions
.
""" | ||
has_unknown_location_checks = False | ||
for property in properties: | ||
if GlobalMessages.ASSERTION_FALSE in property["description"] and extract_property_class( |
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 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.
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.
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.
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.
LGTM other than one small typo. Thanks!
typo fixed Co-authored-by: Zyad Hassan <88045115+zhassan-aws@users.noreply.github.com>
Description of changes:
Fixes output for cases where there are checks with unreachable functions with missing definitions.
Before -
After -
UNDETERMINED
if any such check has aFAILURE
status.Resolved issues:
Addresses -
&str
from&[u8]
#241Call-outs:
Only a temporary fix , in the future, we'll need to resolve the mangled names.
Testing:
How is this change tested?
Is this a refactor change?
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.