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

Integrate goto-synthesizer into Kani #2204

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

qinheping
Copy link
Contributor

Description of changes:

This PR integrate goto-synthesizer into Kani, and add an option to synthesize loop contracts for all loops.

Resolved issues:

Resolves #2156

Call-outs:

The purpose of this PR is to allow us to experiment the loop-contracts synthesizer on Kani benchmarks. However, there are still a few issues we need to address with later PRs:

  • The synthesizer may not terminate. When there is no loop invariants in the candidate space, the enumerator in the synthesizer will run forever trying to find a solution. To address this problem, we might want to allow users to limit the resource used by the synthesizer. On the goto-synthesizer side, we will add an option to limit the size of candidates we enumerate, so that it will not keep enumerating when there is no solution in the candidate space. On the Kani side, we can also add an option to limit the running time of the synthesizer.
  • The synthesizer is not compatible with unwinding numbers. Now the synthesizer ignores unwinding numbers, and will synthesize loop-contracts for all loops no mater if there are loop numbers specified for them. However, we might sometimes want to unwind some loops while synthesize loop contracts for other loops. We will allow users to specify unwinding numbers in goto-synthesizer. Then users can choose to synthesize loop contracts only for those loops without unwinding number specified.

Testing:

  • How is this change tested?

This PR include a regression test.

  • Is this a refactor change?

No.

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.

@qinheping qinheping requested a review from a team as a code owner February 9, 2023 17:48
@qinheping qinheping self-assigned this Feb 9, 2023
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

That's awesome! Thanks for adding this. I have a few questions:

  1. How does the loop synthesizer interacts with loop unwind argument?
  2. Is there anyway we can avoid the non-termination? Even if that means failing the synthesis?
  3. We need to add some documentation around this. I would suggest writing an RFC explaining the user experience, the value it adds and and its limitations.

kani-driver/src/harness_runner.rs Outdated Show resolved Hide resolved
kani-driver/src/harness_runner.rs Outdated Show resolved Hide resolved
@qinheping
Copy link
Contributor Author

Yes, in the Call-outs part of the PR description I introduced my plan to handle non-termination and how to deal with unwinding numbers. More comments about them are appropriated.

And yes, I will create a RFC about it.

@celinval
Copy link
Contributor

celinval commented Feb 9, 2023

I think an RFC will help. From what you described, there is a conflict between unwind and loop synthesis, at least for now. In this case, we should figure out what the message should be and how we handle a harness that is annotated with unwind. There's also a question whether this should be applied to the entire system or to specific harnesses.

@zhassan-aws
Copy link
Contributor

Is it possible to add tests?

@qinheping
Copy link
Contributor Author

Is it possible to add tests?

I added two simple tests to check the synthesizer is correctly called and enumerates required loop invariants. More experiments are needed to see how the synthesizer works on more complicated tests.

@qinheping qinheping force-pushed the kani-synthesizer branch 2 times, most recently from 565205b to fee143d Compare February 18, 2023 14:47
Copy link
Contributor

@celinval celinval left a comment

Choose a reason for hiding this comment

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

Awesome! Are you planning to add more tests to this feature? I think it would be nice to at least add tests where there are assertions inside the loop and test cases where the proof fails. I'm OK if you want to just create an issue for now and work on it later.

kani-driver/src/call_goto_synthesizer.rs Show resolved Hide resolved
kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/harness_runner.rs Outdated Show resolved Hide resolved
kani-driver/src/harness_runner.rs Outdated Show resolved Hide resolved
tests/kani/LoopContractsSynthesizer/main_signed.rs Outdated Show resolved Hide resolved
kani-driver/src/args.rs Outdated Show resolved Hide resolved
@qinheping
Copy link
Contributor Author

Thank you Celina! Yes, I plan to add more tests. I am doing the experiment of running the synthesizer on all current Kani benchmarks, which will provide us better insight about what we can and cannot synthesize. I will create an issue and add tests when the experiment is done.

Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

Thanks @qinheping!

kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/call_goto_synthesizer.rs Show resolved Hide resolved
tests/ui/LoopContractsSynthesizer/main_signed/expected Outdated Show resolved Hide resolved
tests/ui/LoopContractsSynthesizer/main_unsigned/expected Outdated Show resolved Hide resolved
@qinheping qinheping force-pushed the kani-synthesizer branch 2 times, most recently from 28424bd to 6dff259 Compare March 3, 2023 23:41
Copy link
Contributor

@zhassan-aws zhassan-aws left a comment

Choose a reason for hiding this comment

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

Thanks @qinheping! Please address any remaining comments from @celinval and wait for her approval before merging.

@qinheping qinheping force-pushed the kani-synthesizer branch 4 times, most recently from 7aadedb to 144faa0 Compare March 6, 2023 16:13
kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/call_goto_synthesizer.rs Outdated Show resolved Hide resolved
kani-driver/src/args.rs Outdated Show resolved Hide resolved
@qinheping qinheping force-pushed the kani-synthesizer branch 3 times, most recently from a97d072 to 329c764 Compare March 7, 2023 03:12
@qinheping qinheping merged commit a9d9c3f into model-checking:main Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Intergrate goto-synthesizer into Kani
3 participants