Skip to content

Tidying up the options for goto-instrument #6475

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

Merged
merged 14 commits into from
Dec 1, 2021

Conversation

martin-cs
Copy link
Collaborator

@martin-cs martin-cs commented Nov 24, 2021

I'm working through the options of goto-instrument seeing:

  1. Is it implemented?
  2. If its implementation is basically a call to a single other header, are the flags described there with OPT_ and HELP_ macros?
  3. Does it have at least one test case?

This is the first handful of these. It is best to review commit by commit as each one is short and mostly separate.

Let me know if you think its worth me doing more of these!

martin added 10 commits November 23, 2021 22:46
This option has long been deprecated and then removed from cbmc.  I guess
it only stayed in these tests as they have also long been marked as
KNOWNBUG.
These are not implemented, not used by any test, removed from other tools
and marked deprecated almost 5 years ago.
Note that this is different to --xml-ui which shift the tool's general
output into XML; --xml only affects a few options that produce analysis
results.
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #6475 (b71a830) into develop (2a9e3e2) will increase coverage by 0.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #6475      +/-   ##
===========================================
+ Coverage    76.02%   76.23%   +0.20%     
===========================================
  Files         1546     1546              
  Lines       165352   165351       -1     
===========================================
+ Hits        125711   126051     +340     
+ Misses       39641    39300     -341     
Impacted Files Coverage Δ
src/goto-diff/goto_diff_parse_options.cpp 59.04% <ø> (+0.16%) ⬆️
.../goto-instrument/goto_instrument_parse_options.cpp 69.14% <ø> (+0.85%) ⬆️
...rc/goto-instrument/goto_instrument_parse_options.h 100.00% <ø> (ø)
src/pointer-analysis/goto_program_dereference.h 100.00% <ø> (ø)
src/goto-instrument/uninitialized.cpp 88.17% <100.00%> (+88.17%) ⬆️
src/goto-programs/goto_program.h 90.05% <0.00%> (+0.29%) ⬆️
src/analyses/ai.h 62.01% <0.00%> (+4.65%) ⬆️
src/goto-programs/goto_program.cpp 80.03% <0.00%> (+10.06%) ⬆️
src/analyses/uninitialized_domain.h 56.25% <0.00%> (+56.25%) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6b3118...b71a830. Read the comment docs.

@martin-cs
Copy link
Collaborator Author

Please can someone who understands CTest and the build on Mac help me with this:

https://github.com/diffblue/cbmc/runs/4314110590?check_suite_focus=true#step:11:4571

It looks like the uninitialized-check test I added is failing but no reason is given because the goto-instrument tests where listed as passing:

20:   Running typedef4/test.desc  [OK] in 0 seconds
20:   Running uninitialized-check/test.desc  [FAILED]
15:   Running windows_h_VS_2013/test.desc  [OK] in 10 seconds
15: 
15: All tests were successful, 15 tests skipped
 7/68 Test  #15: ansi-c-CORE ..........................................   Passed  102.60 sec
test 28
      Start  28: cbmc-concurrency-CORE

@martin-cs
Copy link
Collaborator Author

Is it possible to access the *.log files from the runs of the regression test?

@martin-cs martin-cs force-pushed the tidy/goto-instrument-options branch from 8d8714a to a46064d Compare November 24, 2021 22:42
martin added 4 commits November 25, 2021 08:17
This also adds:
 --read-first
 --write-first
 --no-loop-duplication
 --hide-internals
which are handled by goto-instrument but were not options so couldn't be
used.
This was tested for but does not appear as an option in
goto_diff_parse_options.h as a valid option so this code can never be used.
Worse, goto-diff doesn't include any of the code that uses it, nor can I
think what the memory model might have to do with diffing goto-programs.
@martin-cs martin-cs force-pushed the tidy/goto-instrument-options branch from a46064d to b71a830 Compare November 25, 2021 08:18
@martin-cs
Copy link
Collaborator Author

Weirdly, running it again got me an error message. I guess there is something fragile about how CTest is set up?

@NlightNFotis
Copy link
Contributor

Hi @martin-cs , looks like everything is passing now.

Next time, if you need any help with a mac build feel free to ping me directly - I have access to an intel machine and an m1 machine if that helps.

Copy link
Contributor

@NlightNFotis NlightNFotis left a comment

Choose a reason for hiding this comment

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

Solid cleanup, minor comment.

@@ -1851,6 +1851,7 @@ void goto_instrument_parse_optionst::help()
"Other options:\n"
" --version show version and exit\n"
HELP_FLUSH
" --xml output files in XML where supported\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to add some documentation in the docs/ folder for this? I wasn't aware of the difference between --xml and --xml-ui

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is but...

A. The current state of the goto-instrument documentation ( https://github.com/diffblue/cbmc/blob/develop/doc/cprover-manual/goto-instrument.md ), apart from function body generation and contracts is pretty abysmal. I do want to improve it but it is after "tidying up, figuring out and testing" the options on my TODO list.

B. I am not sure that it should even be an option or called that. At the moment I am going with preserving functionality but I am also building a set of "I think this would be sensible" changes. As it stands it is a formerly undocumented option that works with only two of the flags. I do wonder if it would make more sense if it followed the same convention as goto-analyzer. Or maybe it would be better to move all of the functionality over to goto-analyzer?

@feliperodri feliperodri added the aws Bugs or features of importance to AWS CBMC users label Nov 29, 2021
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.

❤️

@tautschnig tautschnig merged commit e9e3659 into diffblue:develop Dec 1, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants