[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks for uploading this to phabricator and sorry again that i was lost for a while.

As we already discussed in the mailing lists, i agree with your point that the locked-and-possibly-destroyed state should be removed, and we also had a thought of making it explicit that the patch only applies to posix thread semantics, not to XNU semantics - in other words, there should appear different branches of code depending on the lock's semantics.

Could you also upload the patch file with the "context" (eg. `git diff -U999999` would include +/- 999999 unchanged lines around the changes in the patch file, and phabricator would use them fancily to make reviewing easier; otherwise i don't immediately see which callback you are changing in a particular spot).

Once the patch would reach completion, it'd need to be cleaned up for formatting/whitespace; we usually run `clang-format` over our changes with default settings and it does everything for us.



================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:21
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include <iostream>
 
----------------
You don't need to include `<iostream` even for your own debugging, because we have this other facility:
```
llvm::errs() << "prpr " << RetVal << '\n';
```
(yeah, you can put `SVal` and some other objects into `llvm::errs()` directly (which would be equivalent to `.dump()`ing them), which is also handy).



================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:145-151
+      ConstraintManager &CMgr = state->getConstraintManager();
+      ConditionTruthVal retZero = CMgr.isNull(state, *sym);
+      if(retZero.isConstrainedFalse()){
+        if(lstate->isSchrodingerLocked())
+          state = state->set<LockMap>(lockR, LockState::getLocked());
+        else if(lstate->isSchrodingerUnlocked())
+          state = state->set<LockMap>(lockR, LockState::getUnlocked());
----------------
This code gets duplicated multiple times - including the `checkDeadSymbols` callback as well; can we refactor it into a function?


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:154
+      else{
+        if(!lstate || lstate->isSchrodingerLocked() || lstate->isSchrodingerUnlocked())
+          state = state->set<LockMap>(lockR, LockState::getDestroyed());
----------------
I'd like you to consider various corner cases. Note that because we have the `REGISTER_MAP_WITH_PROGRAMSTATE` privately in this file, only code in this file can affect the contents of that program state trait. So we have complete knowledge of what can and cannot be in the program state.

For example, can it be that there's a symbol in the `DestroyRetVal` map,  but `lstate` is not present for the same mutex region in the `LockMap`? Or does the code ensure that the former implies the latter? If we are sure that some invariants hold, then we should remove the respective `if ()` and ideally replace it with an `assert()`.

If you find invariants that always hold, it would be great to write these down in the comments inside the code.


Repository:
  rL LLVM

https://reviews.llvm.org/D32449