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

Malloc uniqueness #722

Merged
merged 25 commits into from
Jul 27, 2022
Merged

Malloc uniqueness #722

merged 25 commits into from
Jul 27, 2022

Conversation

TDacik
Copy link
Contributor

@TDacik TDacik commented May 4, 2022

This PR tries to increase the precision of the representation of heap-allocated variables.

  • 83e413c Each malloc can now allocate up to n unique heap variables, where n is given as the command-line parameter (by default n is 0, which means that only one non-unique heap variable is allocated for each malloc node -- this is for backward compatibility). Heap variables now also contain a thread identifier of a thread where the allocation happened to distinguish memory allocated in different threads. A heap variable is considered unique if its thread identifier is unique and the number of its alocation is lesser than n.

  • 8e68873 Heap variables that are unique can be strongly (destructively) updated if the size of a written value matches the size of a blob that represents a memory allocated for the variable.

@sim642 sim642 self-requested a review May 4, 2022 17:02
@sim642
Copy link
Member

sim642 commented May 4, 2022

The two issues this at least partially relates to are #186 and #702.

Copy link
Member

@sim642 sim642 left a comment

Choose a reason for hiding this comment

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

I guess for me the big question is whether this works for the example in #186 and possibly the first solution steps for #702.

src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/util/options.schema.json Outdated Show resolved Hide resolved
@@ -0,0 +1,33 @@
// PARAM: --set ana.malloc.unique_address_count 2

// Strong updates are not possible in thread because it is not unique.
Copy link
Member

Choose a reason for hiding this comment

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

How about with mallocFresh analysis also enabled? These alloc variables are created thread-local and never escape, so theoretically they should be strongly updateable.

Copy link
Member

Choose a reason for hiding this comment

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

The corresponding variables are still created as globals here, so freshness does not help here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, mallocFresh analysis only affects race checking, but not anything value-based, which I suppose it also could. Actually the freshness stuff could become part of escape analysis, but there was some issue at the time which prevented that (considering everything non-escaped to be fresh in general).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in the future, we should definitely make that change. I'd suggest doing it in a different PR though.

michael-schwarz and others added 2 commits May 10, 2022 14:46
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
Co-authored-by: Simmo Saan <simmo.saan@gmail.com>
@michael-schwarz michael-schwarz self-assigned this Jul 18, 2022
@michael-schwarz
Copy link
Member

I started an sv-comp run to see what sort of impact this has.

@michael-schwarz
Copy link
Member

As it stands, the PR is unsound for calloc, as the size is not properly calculated there.

@michael-schwarz
Copy link
Member

As it stands, the PR is unsound for calloc, as the size is not properly calculated there.

This was some ancient bug where when we modified the Calloc variant in LibraryFunctions to take two arguments, but did not appropriately pull in the multiplication.

src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
src/analyses/mallocWrapperAnalysis.ml Outdated Show resolved Hide resolved
@michael-schwarz
Copy link
Member

Before we can merge this, I am still waiting on the results of the sv-comp run: I seem to encounter the issue with benchexec freezing up again, so I need to babysit it.

@michael-schwarz
Copy link
Member

For sv-comp the result once again is a resounding it does not matter, but probably it is one of those things where when used selectively may form the key ingredient somewhere.

Interestingly, performance is also not too bad: the x axis is with setting 0, the y axis is with setting 3

x-0-y-3

@sim642
Copy link
Member

sim642 commented Jul 27, 2022

the y axis is with setting 3

I tried to find what Frama-C used in SV-COMP for the equivalent limit. According to their manual -eva-mlevel sounds to be the option for that, but Frama-C's SV-COMP logs don't indicate anything about that (whereas they indicate a bunch of other precision levels, e.g. loop unrolling being much higher). I suppose in it was the default 0 in SV-COMP then.

Given that the Frama-C manual describes this being useful up to certain loop bounds, it probably won't make a real difference with 3, since most loops aren't that short and thus would end up with one weak alloc variable for all the rest anyway.

This doesn't block this PR, but it might be interesting to take a closer look at ReachSafety-Heap with a much higher limit (based on loops actually occurring in those benchmarks) to see if it gets us further. It's probably a matter for #772 to bump the limit based on mallocs in loops.

@michael-schwarz
Copy link
Member

I think to really benefit in loops, we would have to enable some sort of path-sensitivity, which we currently don't do here.
Yes, may be interesting to run it on the heap benchmarks with a higher limit.

I'll merge this for now though, such that it doesn't become stale.

@michael-schwarz michael-schwarz merged commit cf09320 into goblint:master Jul 27, 2022
@sim642 sim642 added this to the v2.0.0 milestone Aug 12, 2022
sim642 added a commit to sim642/opam-repository that referenced this pull request Aug 18, 2022
CHANGES:

Goblint "lean" release after a lot of cleanup.

* Remove unmaintained analyses: OSEK, ARINC, shapes, containment, deadlocksByRaces (goblint/analyzer#460, goblint/analyzer#736, goblint/analyzer#812, goblint/analyzer#474).
* Add interactive analysis (goblint/analyzer#391).
* Add server mode (goblint/analyzer#522).
* Add Compilation Database support (goblint/analyzer#406, goblint/analyzer#448).
* Add floating point domain, unrolled array domain and improved struct domains (goblint/analyzer#734, goblint/analyzer#761, goblint/analyzer#577, goblint/analyzer#419).
* Add static loop unrolling and heap variable unrolling (goblint/analyzer#563, goblint/analyzer#722).
* Improve race detection with may-happen-in-parallel analysis (goblint/analyzer#529, goblint/analyzer#518, goblint/analyzer#595).
* Reimplement lockset and deadlock analyses (goblint/analyzer#659, goblint/analyzer#662, goblint/analyzer#650, goblint/analyzer#655).
* Add pthread extraction to Promela (goblint/analyzer#220).
* Add location spans to output (goblint/analyzer#428, goblint/analyzer#449).
* Improve race reporting (goblint/analyzer#550, goblint/analyzer#551).
* Improve dead code reporting (goblint/analyzer#94, goblint/analyzer#353, goblint/analyzer#785).
* Refactor warnings (goblint/analyzer#55, goblint/analyzer#783).
* Add JSON schema for configuration (goblint/analyzer#476, goblint/analyzer#499).
* Refactor option names (goblint/analyzer#28, goblint/analyzer#192, goblint/analyzer#516, goblint/analyzer#675).
* Add bash completion (goblint/analyzer#669).
* Add OCaml 4.13 and 4.14 support, remove OCaml 4.09 support (goblint/analyzer#503, goblint/analyzer#672).
sim642 added a commit to sim642/opam-repository that referenced this pull request Aug 18, 2022
CHANGES:

Goblint "lean" release after a lot of cleanup.

* Remove unmaintained analyses: OSEK, ARINC, shapes, containment, deadlocksByRaces (goblint/analyzer#460, goblint/analyzer#736, goblint/analyzer#812, goblint/analyzer#474).
* Add interactive analysis (goblint/analyzer#391).
* Add server mode (goblint/analyzer#522).
* Add Compilation Database support (goblint/analyzer#406, goblint/analyzer#448).
* Add floating point domain, unrolled array domain and improved struct domains (goblint/analyzer#734, goblint/analyzer#761, goblint/analyzer#577, goblint/analyzer#419).
* Add static loop unrolling and heap variable unrolling (goblint/analyzer#563, goblint/analyzer#722).
* Improve race detection with may-happen-in-parallel analysis (goblint/analyzer#529, goblint/analyzer#518, goblint/analyzer#595).
* Reimplement lockset and deadlock analyses (goblint/analyzer#659, goblint/analyzer#662, goblint/analyzer#650, goblint/analyzer#655).
* Add pthread extraction to Promela (goblint/analyzer#220).
* Add location spans to output (goblint/analyzer#428, goblint/analyzer#449).
* Improve race reporting (goblint/analyzer#550, goblint/analyzer#551).
* Improve dead code reporting (goblint/analyzer#94, goblint/analyzer#353, goblint/analyzer#785).
* Refactor warnings (goblint/analyzer#55, goblint/analyzer#783).
* Add JSON schema for configuration (goblint/analyzer#476, goblint/analyzer#499).
* Refactor option names (goblint/analyzer#28, goblint/analyzer#192, goblint/analyzer#516, goblint/analyzer#675).
* Add bash completion (goblint/analyzer#669).
* Add OCaml 4.13 and 4.14 support, remove OCaml 4.09 support (goblint/analyzer#503, goblint/analyzer#672).
@michael-schwarz michael-schwarz mentioned this pull request Oct 23, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants