Skip to content

Library functions: mark them as compiled #8412

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 1 commit into from
Aug 20, 2024

Conversation

tautschnig
Copy link
Collaborator

When adding library functions to the goto model we no longer need their function bodies as values in the symbol table. Marking them as "compiled" will make sure we don't re-convert them in any subsequent run (e.g., running cbmc after goto-instrument). Doing so would undo any application of "drop-unused-functions." This was most notable with contracts/dynamic frames, where we ended up reporting thousands of properties as "SUCCESS" in library functions that were never actually called. For some contracts examples this now reduces the number of properties reported from over a thousand to tens of properties.

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a 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).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@rod-chapman
Copy link
Collaborator

This PR makes a huge usability improvement in terms of reducing noise in the output of CBMC. As a user, I like this lots.

@tautschnig tautschnig force-pushed the library-is-compiled branch 3 times, most recently from d9e83d4 to 1e687f6 Compare August 14, 2024 09:48
@tautschnig tautschnig requested a review from qinheping as a code owner August 14, 2024 09:48
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.93%. Comparing base (b87d38a) to head (b42ac11).
Report is 8 commits behind head on develop.

Files Patch % Lines
src/ansi-c/goto-conversion/link_to_library.cpp 77.77% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #8412      +/-   ##
===========================================
- Coverage    77.95%   77.93%   -0.03%     
===========================================
  Files         1726     1726              
  Lines       189601   189668      +67     
  Branches     18250    18234      -16     
===========================================
+ Hits        147796   147810      +14     
- Misses       41805    41858      +53     

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

{
goto_convert(
missing_function,
goto_model.symbol_table,
library_model.goto_functions,
message_handler);
goto_model.symbol_table.get_writeable_ref(missing_function).set_compiled();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any intuition why our regression tests don't cover this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This case would only apply when system headers ship functions marked inline. I believe we run into this on FreeBSD, but seemingly not on Linux (which is the only platform we do coverage recording on).

When adding library functions to the goto model we no longer need their
function bodies as values in the symbol table. Marking them as
"compiled" will make sure we don't re-convert them in any subsequent run
(e.g., running cbmc after goto-instrument). Doing so would undo any
application of "drop-unused-functions." This was most notable with
contracts/dynamic frames, where we ended up reporting thousands of
properties as "SUCCESS" in library functions that were never actually
called. For some contracts examples this now reduces the number of
properties reported from over a thousand to tens of properties, and in
other cases this made apparent that we were spuriously reporting
"SUCCESS" when we never actually invoked those functions in the first
place.
@tautschnig tautschnig force-pushed the library-is-compiled branch from 1e687f6 to b42ac11 Compare August 20, 2024 08:21
@tautschnig tautschnig removed their assignment Aug 20, 2024
@@ -41,5 +41,8 @@ elif echo $args_inst | grep -q -- "--dump-c" ; then

rm "${name}-mod.c"
fi
if ! echo "${args_cbmc}" | grep -q -- --function ; then
$goto_instrument --drop-unused-functions "${name}-mod.gb" "${name}-mod.gb"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should --drop-unused-functions become the default when running goto-instrument with contracts ?

@tautschnig tautschnig merged commit 89a0470 into diffblue:develop Aug 20, 2024
39 of 40 checks passed
@tautschnig tautschnig deleted the library-is-compiled branch August 20, 2024 21:49
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.

6 participants