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

SYNTHESIZER: Allow goto-synthesizer accept all CBMC options #7900

Merged

Conversation

qinheping
Copy link
Collaborator

@qinheping qinheping commented Sep 18, 2023

With this PR, goto-synthesizer will accept all of CBMC options so that we don't need to update the backend options in goto-synthesizer when they are updated in CBMC_OPTION.

  • Each commit message has a non-empty body, explaining why the change was made.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@qinheping qinheping added aws Bugs or features of importance to AWS CBMC users Synthesis Invariant synthesis labels Sep 18, 2023
@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (faa3144) 78.62% compared to head (31b22af) 78.62%.

❗ Current head 31b22af differs from pull request most recent head b1e19b9. Consider uploading reports for the commit b1e19b9 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7900   +/-   ##
========================================
  Coverage    78.62%   78.62%           
========================================
  Files         1701     1700    -1     
  Lines       195974   195974           
========================================
  Hits        154086   154086           
  Misses       41888    41888           
Files Coverage Δ
...oto-synthesizer/goto_synthesizer_parse_options.cpp 76.19% <100.00%> (+0.94%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@qinheping qinheping force-pushed the features/synthesizer-cbmc-args branch 4 times, most recently from 7ea1ad2 to 7408ce3 Compare September 19, 2023 17:08
if(strcmp(cbmc_args[0], "cbmc") != 0)
{
throw invalid_command_line_argument_exceptiont(
"cbmc-cmd must starts with cbmc, e.g., --cbmc-cmd \"cbmc [args]\"",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have much background, but why not make the option --cbmc-options instead to remove the need to specify cbmc (which seems redundant?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

goto-synthesizer checks its arguments of that start with -- against the SYNTHEZIER_OPT, and errors out if the argument is not matched any defined option. It will happen if some CBMC option is not one of the synthesizer options.
So we let the argument start with cbmc instead to avoid the check.

@tautschnig tautschnig self-assigned this Oct 2, 2023
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Doing string tokenisation and possibly having to deal with quoting/unquoting feels fragile. Could we please try a rather different approach:

  1. Move the implementation of goto_synthesizer_parse_optionst::goto_synthesizer_parse_optionst(int argc, const char **argv) to goto_synthesizer_parse_options.cpp.
  2. Keep #include <cbmc/cbmc_parse_options.h> as proposed in the current commit.
  3. Use GOTO_SYNTHESIZER_OPTIONS CBMC_OPTIONS (no operator required between these) as the first argument to goto_synthesizer_parse_optionst::goto_synthesizer_parse_optionst.
  4. Now goto-synthesizer should just accept all of CBMC's options, and will continue to do so even when those change. We just need to find some good solution for the --help output, perhaps just say "Accepts all of cbmc's options plus the following: ..."

@tautschnig tautschnig assigned qinheping and unassigned tautschnig Oct 3, 2023
@qinheping qinheping force-pushed the features/synthesizer-cbmc-args branch from 7408ce3 to 65524d5 Compare October 4, 2023 16:43
@qinheping qinheping changed the title SYNTHESIZER: Allow user to specify the command to run CBMC SYNTHESIZER: Allow goto-synthesizer accept all CBMC options Oct 4, 2023
@qinheping
Copy link
Collaborator Author

Doing string tokenisation and possibly having to deal with quoting/unquoting feels fragile. Could we please try a rather different approach:

1. Move the implementation of `goto_synthesizer_parse_optionst::goto_synthesizer_parse_optionst(int argc, const char **argv)` to `goto_synthesizer_parse_options.cpp`.

2. Keep `#include <cbmc/cbmc_parse_options.h>` as proposed in the current commit.

3. Use `GOTO_SYNTHESIZER_OPTIONS CBMC_OPTIONS` (no operator required between these) as the first argument to `goto_synthesizer_parse_optionst::goto_synthesizer_parse_optionst`.

4. Now `goto-synthesizer` should just accept all of CBMC's options, and will continue to do so even when those change. We just need to find some good solution for the `--help` output, perhaps just say "Accepts all of cbmc's options plus the following: ..."

Thank you! I also updated the title and description of this PR.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

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

Could you please re-add a test, if that's at all possible (i.e., if there is any new behaviour observable)?

@qinheping qinheping enabled auto-merge October 5, 2023 18:32
With this commit, goto-synthesizer will accept all of CBMC options so that we don't need to update the backend options in goto-synthesizer when they are updated in CBMC_OPTION.
@qinheping qinheping force-pushed the features/synthesizer-cbmc-args branch from 31b22af to b1e19b9 Compare October 5, 2023 18:41
@qinheping qinheping merged commit e0656e2 into diffblue:develop Oct 5, 2023
33 of 34 checks passed
@qinheping qinheping deleted the features/synthesizer-cbmc-args branch October 18, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws Bugs or features of importance to AWS CBMC users Synthesis Invariant synthesis
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants