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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks! Your code looks very clear now, and it seems correct to me.

One last thing we definitely should do here would be add regression tests for the new functionality. I guess you already have your tests, otherwise you wouldn't know if your code works, so you'd just need to append them to the patch, somewhere at the bottom of `test/Analysis/pthreadlock.c`, and make sure that `make -j4 check-clang-analysis` passes. Ideally, we should cover as many branches as possible.

A few ideas of what to test (you might have thought about most of them already, and i expect them to actually work by just looking at what your code accomplishes):

- What we can/cannot do with the mutex in the failed-to-be-destroyed state, depending on the state of the mutex before destruction was attempted.
- What we can/cannot do with the mutex in each of the "Schrodinger" states - in particular, do we display the double-destroy warning in such cases?
- How return-symbol death races against resolving success of the destroy operation: what if the programmer first tries to destroy mutex, then uses the mutex, then checks the return value?
- Are you sure we cannot `assert(lstate)` on line 137? - a test could be added that would cause such assertion to fail if someone tries to impose it.

Apart from that, i guess it'd be good to use more informative variable names in a few places (see inline comments), and fix the formatting, i.e. spaces and line breaks (should be easy with `clang-format`). Also you shouldn't add the `.DS_Store` files :) And then we'd accept and commit this patch.



================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:28
 struct LockState {
-  enum Kind { Destroyed, Locked, Unlocked } K;
+  enum Kind { Destroyed, Locked, Unlocked, SchrodingerUntouched, SchrodingerUnlocked } K;
 
----------------
I still think these names, no matter if a good metaphor or not and no matter how much i enjoyed them, should be toned down :) Suggesting `UntouchedAndPossiblyDestroyed` and `UnlockedAndPossiblyDestroyed`.


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:77
   void reportUseDestroyedBug(CheckerContext &C, const CallExpr *CE) const;
+  ProgramStateRef setAppropriateLockState(ProgramStateRef state, const MemRegion* lockR, const SymbolRef* sym, bool fromCheckDeadSymbols) const;
 };
----------------
I suggest renaming to something like "`resolvePossiblyDestroyedMutex()`".

Also, i'm for passing the symbol by value (with `*` dereference at most call sites) because it's less surprising/confusing to the reader.

I also suggest a comment explaining what the function does. Eg., "When a lock is destroyed, in some semantics we are not sure if the destroy call has succeeded or failed, and the lock enters one of the 'possibly destroyed' state. There is a short time frame for the programmer to check the return value to see if the lock was successfully destroyed. Before we model the next operation over that lock, we call this function to see if the return value was checked by now and set the lock state - either to destroyed state or back to its previous state."


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:81-85
 // GDM Entry for tracking lock state.
 REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)
 
 REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)
+REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
----------------
Because there's only one comment per three traits, it'd be great to clean this up a bit together with commenting up your new trait:

```
// A stack of locks for tracking lock-unlock order.
REGISTER_LIST_WITH_PROGRAMSTATE(LockSet, const MemRegion *)

// An entry for tracking lock states.
REGISTER_MAP_WITH_PROGRAMSTATE(LockMap, const MemRegion *, LockState)

// Return values for unresolved destroy calls.
REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef)
```


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:143-146
+    if(lstate->isSchrodingerUntouched())
+      state = state->remove<LockMap>(lockR);
+    else if(lstate->isSchrodingerUnlocked())
+      state = state->set<LockMap>(lockR, LockState::getUnlocked());
----------------
I think we can be certain that the lock is in one of these states, and assert that.


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:149-151
+    if(lstate->isSchrodingerUntouched() || lstate->isSchrodingerUnlocked()){
+      state = state->set<LockMap>(lockR, LockState::getDestroyed());
+    }
----------------
Assert the lock state here as well?


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:302-308
+      SVal X = State->getSVal(CE, C.getLocationContext());
+      if (X.isUnknownOrUndef()){
+        return;
+      }
+
+      DefinedSVal retVal = X.castAs<DefinedSVal>();
+      SymbolRef sym = retVal.getAsSymbol();
----------------
A bit shorter and more stylish:
```
SymbolRef RetSym = C.getSVal(CE).getAsSymbol();
if (!RetSym)
  return;
```

Also, we may want to see what happens when the value is not a symbol. This would be surprising, but it may happen if the body of `pthread_mutex_destroy()` is suddenly available in our translation unit and we modeled the call, or another checker has assisted with modeling it. In this case the return value may be a concrete integer (say `0` or `1`) or an `UnknownVal` or an `UndefinedVal`. It may also be something completely weird such as a pointer or a structure if the user defines his own function that is called "`pthread_mutex_destroy()`" but does something completely different.

I suggest to stop tracking the lock region in case `RetSym` is null (remove `LockR` from the `LockMap`), because in the really weird case when this code actually gets triggered, we'd at least not say that the mutex is still locked later on the path.

Additionally, by adding the transition before that return, we ensure that `setAppropriateLockState` above has taken effect.


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:399
+  ProgramStateRef State = C.getState();
+
+  DestroyRetValTy TrackedSymbols = State->get<DestroyRetVal>();
----------------
Could you add here a
```
// TODO: Clean LockMap when a mutex region dies.
```
That's not something you should instantly do, but it's definitely something that needs to be done for this checker some day.


Repository:
  rL LLVM

https://reviews.llvm.org/D32449