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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Analysis/ThreadSafety.cpp:1445
+      if (!TCond && FCond) {
+        Negate = !Negate;
+        return getTrylockCallExpr(COP->getCond(), C, Negate);
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > Rather than do an assignment here, why not just pass `!Negate` directly below, since you're returning?
> The third parameter of `getTrylockCallExpr` is a (non-const) reference, so I can only pass an lvalue. It's basically a [call-by-reference](https://en.wikipedia.org/wiki/Evaluation_strategy#Call_by_reference) pattern.
Ah, I missed that when looking at the code in Phab, thanks for the explanation!


================
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:
> > 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
}
```



Repository:
  rC Clang

https://reviews.llvm.org/D52888