Skip to content

Check for memory leaks on exit and abort#3462

Merged
tautschnig merged 2 commits intodiffblue:developfrom
peterschrammel:memory-leak-abort
Nov 22, 2022
Merged

Check for memory leaks on exit and abort#3462
tautschnig merged 2 commits intodiffblue:developfrom
peterschrammel:memory-leak-abort

Conversation

@peterschrammel
Copy link
Member

@peterschrammel peterschrammel commented Nov 25, 2018

This has become relevant in the SV-COMP MemCleanup category.

  • 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.

tautschnig
tautschnig approved these changes Nov 25, 2018
View reviewed changes
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.

Looks good as far as I can tell, but is failing CI.

@kroening
Copy link
Collaborator

kroening commented Nov 25, 2018

There's SV-COMP and there's the real world. In the real-world, memory leaks at exit() don't matter, and most engineers won't fix.
If we need exit() memory leaks, I would have a separate categories for these.

@peterschrammel
Copy link
Member Author

peterschrammel commented Nov 25, 2018 •
edited
Loading

We could do a classification similar to Valgrind: http://valgrind.org/docs/manual/faq.html#faq.deflost
E.g. distinguishing 'definitely lost' and 'still reachable' would make sense.

@peterschrammel peterschrammel force-pushed the memory-leak-abort branch 2 times, most recently from 84fff90 to 507d221 Compare November 25, 2018 14:46
allredj
allredj reviewed Nov 25, 2018
View reviewed changes
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

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

✔️
Passed Diffblue compatibility checks (cbmc commit: 507d221).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/92469216

@tautschnig tautschnig mentioned this pull request Dec 1, 2018
Closed
13 tasks
@NlightNFotis
Copy link
Contributor

NlightNFotis commented Mar 8, 2021

Hi @peterschrammel, how should we proceed with this? It looks valuable but looks like it's going to need some work.

@peterschrammel
Copy link
Member Author

peterschrammel commented Apr 16, 2021

This is part of the SV-COMP patchset and needs more work before it can go into develop. Please keep it open.

@tautschnig tautschnig self-assigned this May 13, 2021
@tautschnig tautschnig requested review from a team and chris-ryder as code owners November 21, 2022 13:09
@tautschnig tautschnig mentioned this pull request Nov 21, 2022
Merged
@tautschnig tautschnig removed their assignment Nov 21, 2022
@codecov
Copy link

codecov bot commented Nov 21, 2022 •
edited
Loading

Codecov Report

Base: 78.34% // Head: 78.35% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (1ac66e7) compared to base (a53fa0f).
Patch coverage: 94.06% of modified lines in pull request are covered.

❗ Current head 1ac66e7 differs from pull request most recent head b4b815f. Consider uploading reports for the commit b4b815f to get more accurate results

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #3462   +/-   ##
========================================
  Coverage    78.34%   78.35%           
========================================
  Files         1644     1645    +1     
  Lines       190313   190372   +59     
========================================
+ Hits        149097   149159   +62     
+ Misses       41216    41213    -3
Impacted Files Coverage Δ
jbmc/src/janalyzer/janalyzer_parse_options.cpp 48.58% <ø> (ø)
.../src/java_bytecode/character_refine_preprocess.cpp 54.27% <ø> (ø)
...mc/src/java_bytecode/character_refine_preprocess.h 100.00% <ø> (ø)
jbmc/src/java_bytecode/ci_lazy_methods.h 100.00% <ø> (ø)
jbmc/src/java_bytecode/ci_lazy_methods_needed.cpp 98.41% <ø> (ø)
jbmc/src/java_bytecode/ci_lazy_methods_needed.h 100.00% <ø> (ø)
jbmc/src/java_bytecode/code_with_references.cpp 84.61% <ø> (ø)
jbmc/src/java_bytecode/java_bmc_util.cpp 100.00% <ø> (ø)
...mc/src/java_bytecode/java_bytecode_convert_class.h 0.00% <ø> (ø)
...src/java_bytecode/java_bytecode_convert_method.cpp 97.69% <ø> (ø)
... and 411 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@peterschrammel @tautschnig
Add memory leak instrumentation to abort and exit
b4b815f 
SV-COMP (also) requires that all dynamically allocated memory is freed
via `free` before terminating the program.
@tautschnig tautschnig merged commit b1d99cb into diffblue:develop Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tautschnig tautschnig tautschnig approved these changes

@chrisr-diffblue chrisr-diffblue Awaiting requested review from chrisr-diffblue

@kroening kroening Awaiting requested review from kroening kroening is a code owner

@martin-cs martin-cs Awaiting requested review from martin-cs

@pkesseli pkesseli Awaiting requested review from pkesseli

@smowton smowton Awaiting requested review from smowton

@thk123 thk123 Awaiting requested review from thk123

@chris-ryder chris-ryder Awaiting requested review from chris-ryder

+1 more reviewer

@allredj allredj allredj left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@peterschrammel peterschrammel

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@peterschrammel @kroening @NlightNFotis @tautschnig @allredj