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

Add live streaming JSON output from CBMC to prevent Kani output hang #1278

Closed
wants to merge 22 commits into from

Conversation

jaisnan
Copy link
Contributor

@jaisnan jaisnan commented Jun 14, 2022

Description of changes:

CBMC currently reads the input from CBMC , waits for termination and writes the output to a file which goes to the parsing script. This runs into an issue when CBMC does not seemingly terminate, which makes Kani appear to be hung.

This PR changes that to show the JSON output live from CBMC which will later be consumed by the parsing to show the post processed Kani output.

For example, for the rust file -

fn rev(mut x: [i32; 4]) -> [i32; 4] {
    x.reverse();
    x
}

#[cfg(kani)]
#[kani::proof]
fn example() {
    let var = kani::any();
    let reversed = rev(var);
    let double_reversed = rev(reversed);

    assert_eq!(var, double_reversed);
}

Before -

Screen Shot 2022-06-13 at 11 46 42 PM

After -

Screen Shot 2022-06-13 at 11 49 32 PM

As we can , there is a long unwinding loop that's not being displayed to the user which can cause some confusion.
With this change, the user can be informed about the status of the verification live.

Resolved issues:

Related #493

Call-outs:

  1. This Hides the live output behind a flag --use-piped-output for now.
  2. This PR only streams the JSON output from CBMC. An incoming PR will convert the live JSON into the regular Kani output.

Testing:

  • How is this change tested?
    Hidden behind a flag to be as non-invasive as possible.

  • Is this a refactor change?
    Yes

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@jaisnan jaisnan requested a review from a team as a code owner June 14, 2022 04:02
@jaisnan jaisnan changed the title Add live streaming output from CBMC to prevent Kani output hang Add live streaming JSON output from CBMC to prevent Kani output hang Jun 14, 2022
Copy link
Contributor

@adpaco-aws adpaco-aws left a comment

Choose a reason for hiding this comment

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

You should be able to define expected tests for this, no?

kani-driver/src/args.rs Outdated Show resolved Hide resolved
kani-driver/src/args.rs Outdated Show resolved Hide resolved
kani-driver/src/call_cbmc.rs Outdated Show resolved Hide resolved
kani-driver/src/call_cbmc.rs Outdated Show resolved Hide resolved
scripts/cbmc_json_parser.py Outdated Show resolved Hide resolved
kani-driver/src/session.rs Outdated Show resolved Hide resolved
scripts/cbmc_json_parser.py Outdated Show resolved Hide resolved
usage_error(f"Unable to complete parsing CBMC Output\n")
else:
usage_error(f"CBMC Output not piped to post-processing\n")
return_code = 0
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 use an error code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a custom exit string and an error code of 1 with -

print(f"Unable to complete parsing CBMC Output\n")
sys.exit(1)

Would this work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, my comment here was not very clear.

If I'm not mistaken, this will print all lines from CBMC's output and return 0. If we change this to 1, do the regression tests still pass?

Copy link
Contributor Author

@jaisnan jaisnan Jun 20, 2022

Choose a reason for hiding this comment

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

I should have expanded more in the comments. The return code is 1 if the stream of output is interrupted , either by a broken line or broken pipe or user interrupt (Ex - keyboard interrupt). If it completes printing all the lines from CBMC, then it returns 0. I shall add this clarification to the comments on the code.

I hope this is an acceptable usage of the error code?

Copy link
Contributor

Choose a reason for hiding this comment

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

A return code with value 0 basically means that verification is always successful.

scripts/cbmc_json_parser.py Outdated Show resolved Hide resolved
@jaisnan jaisnan requested a review from adpaco-aws June 20, 2022 15:16
@jaisnan
Copy link
Contributor Author

jaisnan commented Jun 20, 2022

You should be able to define expected tests for this, no?

I was able to add in a few tests and refactored the code as suggested. Let me know if they're as expected!

@@ -42,18 +42,30 @@ impl KaniSession {
cmd.arg("--json-ui");

let now = Instant::now();
let _cbmc_result = self.run_redirect(cmd, &output_filename)?;
let elapsed = now.elapsed().as_secs_f32();
Copy link
Contributor

Choose a reason for hiding this comment

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

This call is right after the call to Instant::now(), so it's not measuring verification time anymore.

usage_error(f"Unable to complete parsing CBMC Output\n")
else:
usage_error(f"CBMC Output not piped to post-processing\n")
return_code = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

A return code with value 0 basically means that verification is always successful.

@adpaco-aws
Copy link
Contributor

Closed in favor of #1433

@adpaco-aws adpaco-aws closed this Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants