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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
