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

CHC support #183

Merged
merged 13 commits into from
Jul 3, 2023
Merged

CHC support #183

merged 13 commits into from
Jul 3, 2023

Conversation

s0mark
Copy link
Contributor

@s0mark s0mark commented Dec 9, 2022

This is the result of my BSc degree project: a CHC frontend implemented in Theta. It can be enabled with the --chc flag, and different transformation methods can be chosen using the --chc-transformation FORWARD/BACKWARD flag. See the documentation for details.

@s0mark s0mark marked this pull request as ready for review December 9, 2022 17:08
@leventeBajczi leventeBajczi self-requested a review February 8, 2023 11:42
Copy link
Contributor

@as3810t as3810t left a comment

Choose a reason for hiding this comment

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

All in all, I think this is a very nice work. I left some minor comments. The only major thing missing (and we do not set a good example on this matter 😅) is to add the copyright to the new source files you created.

See an example here. Please do update the year though.

private static SmtLibTypeTransformer typeTransformer = new GenericSmtLibTypeTransformer(null);
private static SmtLibTermTransformer termTransformer = new GenericSmtLibTermTransformer(initialSymbolTable);
private static CharStream charStream;

Copy link
Contributor

@as3810t as3810t Jun 29, 2023

Choose a reason for hiding this comment

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

Missing private constructor.

Suggested change
private ChcUtils() {}

@@ -83,7 +83,7 @@ public class XcfaPassManager {
// new AssignmentChainRemoval(),
// new NoReadVarRemovalPass(),
// new GlobalVarsToStoreLoad(),
new UnusedVarRemovalPass(),
// new UnusedVarRemovalPass(),
Copy link
Contributor

@as3810t as3810t Jun 29, 2023

Choose a reason for hiding this comment

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

This is a global modification that will also affect C source codes. My questions are:

  1. Why was this modification needed?
  2. If the change is needed for CHCs, can we somehow make XcfaPassManager configurable to include the pass for C codes and remove it for CHCs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modification was necessary because the pass removed variables that would've been necessary for correct verification, resulting in incorrect answers. I added methods to XcfaPassManager that can remove passes and moved the removal to CHC handling part of XcfaCli.

@s0mark
Copy link
Contributor Author

s0mark commented Jul 2, 2023

All in all, I think this is a very nice work. I left some minor comments.

I implemented the requested changes.

The only major thing missing (and we do not set a good example on this matter 😅) is to add the copyright to the new source files you created. See an example here. Please do update the year though.

I added the license text to all .java files in the CHC frontend. I wasn't sure about what to do with the ANTLR file for CHCs, because it is only a minor extension of the SMT-LIBv2 grammar file created by Julian Thome. I ended up leaving the original MIT license, please let me know if a change is necessary.

@s0mark s0mark requested a review from as3810t July 2, 2023 15:39
Copy link
Contributor

@as3810t as3810t left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! I think this can be merged @leventeBajczi .

Copy link
Contributor

@leventeBajczi leventeBajczi left a comment

Choose a reason for hiding this comment

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

I agree! I'll go ahead and merge this PR. Thanks for the work @s0mark and the review @as3810t!

@leventeBajczi leventeBajczi merged commit 839a4c9 into ftsrg:master Jul 3, 2023
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.

3 participants