Skip to content

Conversation

@sim642
Copy link
Member

@sim642 sim642 commented Mar 23, 2022

Closes #659.

Changes

  1. Add mutexEvents analysis to emit events handled by mutex, mayLocks and deadlock analyses.
  2. Replace analysis-specific ambiguous lock pointer handling with path splitting via ctx.split. One split is introduced for each possible mutex pointer. Joining of paths automagically takes care of the ambiguity (see table below).
    This also increases the precision of locking an ambiguous lock pointer in the path-sensitive mutex analysis: previously no must locks were added, but now it properly splits into multiple paths with different locksets.
  3. Add more complete handling of failing locks (includes try locks) by just having an extra path split via ctx.split if failure is allowed. Again, the joining of paths takes care of everything by considering the extra path where no lock was acquired,
  4. Add more complete handling of blob locks via IsMultiple query (see table below). This should additionally account for weakly updated locals too.
  5. Add more complete handling of unknown lock pointers using Addr.UnknownPtr (see table below). Previously lockset analyses just dropped these from the MayPointsTo query result conversion.
  6. Fix deadlock detection w.r.t. ambiguous, blob and unknown lock pointers.
  7. Extract very simple must and may lockset analysis functors and instantiate mutex, maylocks and deadlock analyses using these.

Behavior

The following tables give an overview of all the cases that are now considered.

Must lockset

Definite pointer Ambiguous pointer Blob pointer Unknown pointer Failing lock
Lock Add pointer Don't add any Don't add Don't add Don't add
Unlock Remove pointer Remove all ambiguous pointers N/A (never added) Remove all N/A

May lockset

Definite pointer Ambiguous pointer Blob pointer Unknown pointer Failing lock
Lock Add pointer Add all ambiguous pointers Add blob pointer Add unknown pointer Add
Unlock Remove pointer Don't remove any Don't remove Don't remove any N/A

TODO

@sim642 sim642 added the cleanup Refactoring, clean-up label Mar 23, 2022
@sim642 sim642 self-assigned this Mar 23, 2022
@sim642
Copy link
Member Author

sim642 commented Mar 25, 2022

  • Determine whether MustLock and MustUnlock events are still necessary.

The only difference that these still provide is that when the general Lock is emitted, MustLock is emitted conditionally:

let nls = D.add l ctx.local in
if not (D.equal ctx.local nls) then
ctx.emit (MustLock (fst l));

Removing that relocking condition causes a failure in just one test: 13-privatized/70-mm-reentrant.

At first sight it seems like a reasonable thing to do: only emit MustLock when it's not already locked. But that relocking check is itself based on must locksets. If the mutex isn't already must locked but may locked, then it is treated as if it wasn't locked at all, which doesn't account for the possibility that it might have been already locked. In such scenario we don't know whether it's the first locking or relocking.

Actually emitting MustLock events like this isn't even necessarily right, because in the case of ambiguity or failure, the particular split path still sees the MustLock event, although the following path join might remove it from the must lockset. So this event cannot even guarantee anything. It's similar to the whole branched thread creation and EnterMultithreaded event problem in that a later join would have to account for events that along some path didn't happen.

Therefore I'm unsure whether this is even the right thing to do. Proper handling of recursive locks is a whole other issue #658.


EDIT: I managed to get rid of those events by just adding the recursive locking check into mutex-oplus and mutex-meet specifically to make them sound on that test, others were already correct.

@sim642 sim642 removed the pr-dependency Depends or builds on another PR, which should be merged before label Mar 25, 2022
@michael-schwarz
Copy link
Member

EDIT: I managed to get rid of those events by just adding the recursive locking check into mutex-oplus and mutex-meet specifically to make them sound on that test, others were already correct.

Would the apron mutex meet not also require such special handling then?

@sim642 sim642 merged commit 51a61ee into master Mar 31, 2022
@sim642 sim642 deleted the locks-refactor branch March 31, 2022 12:25
sim642 added a commit that referenced this pull request Apr 1, 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Refactoring, clean-up precision unsound

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor and fix different lockset analyses

3 participants