[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D52888#1255862, @aaronpuchert wrote:

> Additional changes (including some non-tail recursion unfortunately) would allow the following to work:
>
>   void foo16() {
>     if (cond ? mu.TryLock() : false)
>       mu.Unlock();
>   }
>   
>   void foo17() {
>     if (cond ? true : !mu.TryLock())
>       return;
>     mu.Unlock();
>   }
>   
>
> Worth the effort, or is that too exotic?


`foo16()` looks like code I could plausibly see in the wild. Consider the case where the mutex is a pointer and you want to test it for null before calling `TryLock()` on it (`M ? M->TryLock() : false`). However, I don't have a good feeling for how much work it would be to support that case.



================
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880
+  void foo13() {
+    if (mu.TryLock() ? 1 : 0)
+      mu.Unlock();
+  }
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > aaronpuchert wrote:
> > > aaron.ballman wrote:
> > > > Can you add a test that shows we get it right even if the user does something less than practical, like:
> > > > ```
> > > > if (mu.TryLock() ? mu.TryLock() : false); // Warn about double lock
> > > > if (mu.TryLock() ? mu.Unlock(), 1 : 0)
> > > >   mu.Unlock(); // Warn about unlocking unheld lock
> > > > if (mu.TryLock() ? 1 : mu.Unlock(), 0)
> > > >   mu.Unlock(); // Warn about unlocking an unheld lock
> > > > if (mu.TryLock() ? (true ? mu.TryLock() : false) : false); // Warn about double lock
> > > > ```
> > > I'm afraid these don't work as expected if we don't branch on `?:` as before. Basically we treat this as if both conditions where evaluated unconditionally.
> > > 
> > > On calling a try-lock function, no lock is immediately acquired. Only when we branch on the result of that try-lock call is the mutex added to the capability set on the appropriate branch. Now if we branch on `?:` (the situation before this change), we are going to complain about conditionally held locks on the join point in @hokein's use case, and if we don't branch (the behavior after this change), we are not treating your examples correctly. I'm not sure we can treat both as intended.
> > > 
> > > I'm slightly leaning towards not branching: the C++ standard says that only the [used expression is actually evaluated](https://en.cppreference.com/w/cpp/language/operator_other#Conditional_operator), but I think it's not considered good practice to have side effects in a ternary operator. (I can't actually find it anywhere besides a discussion on [Hacker News](https://news.ycombinator.com/item?id=14810994). Personally I would prefer to use an if-statement if one of the expressions has side-effects.)
> > > 
> > > Tell me what you think.
> > I agree that we don't want to encourage complex code in ternary operators, but that complexity does come up often as a result of macro expansion. It feels like there isn't a good answer here because either approach will have unexpected results.
> > 
> > I'll let @delesley be the tie-breaker on whether we follow ternary branches or not, but if we decide to accept this patch and not follow branches, I think we should capture those test cases as expected false negatives with a comment about why they happen and why it's expected behavior.
> > 
> > Related, does this patch then impact the behavior of code like:
> > ```
> > void f(Mutex &M) {
> >   M.Lock();
> > }
> > 
> > void g() {
> >   Mutex mu;
> >   (void)(true ? f(mu) : (void)0);
> >   mu.Unlock(); // Ok
> > }
> > ```
> > 
> No, this is not affected. There will be a warning independently of this patch. We notice that the lock is held on only one branch after joining them.
> 
> I think my wording was a bit unfortunate: we still follow the ordinary CFG. On try-lock the lock is not acquired immediately, only when we branch on the return value. So we weaken the usual requirement of "no conditional locks" for the period between the try-lock call and the branch. That was the situation before this patch already.
> 
> With this patch `?:` branches on the return value of a try-lock are no longer considered as possible acquisition points. We postpone the acquisition even further to another (more obvious) branch like an `if` statement that uses the try-lock return result. To that end we inspect possible `ConditionalExpr`s so that we can derive which branch holds the lock even when these are used.
Ah, okay, thank you for the further explanation. That makes me a bit more comfortable with the proposed approach.


Repository:
  rC Clang

https://reviews.llvm.org/D52888