[PATCH] D98747: Thread safety analysis: Don't warn about managed locks on join points

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added a comment.

Thanks for roping me into the conversation, Aaron, and sorry about the delay.  I have mixed feelings about this patch.  With respect to the purpose of thread safety analysis, finding race conditions is obviously a major concern, because race conditions are hard to find in tests.  However, preventing deadlocks is almost as important, and can be even more important in certain critical applications.  The consequence of a double unlock is usually a crash or an exception with an error message.  The consequence of a double lock is often a deadlock, depending on the underlying thread library, which is a more serious problem that is harder to fix.  Thus, I am concerned about the change in the test case on line 2895, where a potential deadlock has gone from detected, to no warning.  Deadlocks are often not found in tests, because test coverage is never perfect over all possible control-flow paths.

The use cases here are also slightly different.  A MutexLock object uses the RAII design pattern to ensure that the underlying mutex is unlocked on every control flow path, and presumably includes logic to prevent double releases/unlocks, as is usually the case with RAII.  The RAII pattern is sufficiently robust that we can "trust the implementation", and relax static checks.  A MutexUnlock object inverts the RAII pattern.  Because a lock is being acquired on different control-flow paths, rather than being released, it may make sense to keep the stronger checks in order to guard against potential deadlocks.  On the other hand, we are still using RAII, so the implementer of MutexUnlock could presumably insert logic to guard against deadlocks if that was a concern.  Thus, I could be persuaded that deadlocks are less important in this particular case.

Is there a compelling reason for this change, other than consistency/symmetry concerns?  In other words, do you have real-world code where you are getting false positives when using MutexUnlock?  If so, then I am in favor of the change.  If not, then I tend to err on the side of "if it ain't broke don't fix it."  :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98747/new/

https://reviews.llvm.org/D98747