[PATCH] D51187: [RFC] Thread safety analysis: Track status of scoped capability

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Thanks for pointing out my error! Ignoring the implementation for a moment, do you think this is a good idea or will it produce to many false positives? Releasable/relockable scoped capabilities that I have seen keep track of the status, so it makes sense, but maybe there are others out there.



================
Comment at: lib/Analysis/ThreadSafety.cpp:929
+      return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc());
+    Locked = true;
+
----------------
delesley wrote:
> It's been a while since I last looked at this code, but I don't think you can use mutable fields in a FactEntry.  The analysis creates a FactSet for each program point, but each FactSet simply has pointers (FactIDs) for the underlying FactEntries.  If you change the definition of a FactEntry, it will change that definition for every program point.  So if you do an unlock on one side of a branch, and change the underlying FactEntry to reflect that, then analysis will then think you have also done the unlock on the other side of the branch.  
> 
> Any changes should always be done by adding or removing entries from the FactSet, not by mutating the underlying FactEntries.
> 
That explains why there are problems with back edges. Now that you say it I'm actually aware of how the `FactManager` and `FactSet` work, but apparently I didn't consider that here.

Would you mind if I make a separate change that returns only `const FactEntry*` from the `FactManager`? Maybe that makes it harder to write wrong code in the future.


Repository:
  rC Clang

https://reviews.llvm.org/D51187