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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: lib/Analysis/ThreadSafety.cpp:1445
+      if (!TCond && FCond) {
+        Negate = !Negate;
+        return getTrylockCallExpr(COP->getCond(), C, Negate);
----------------
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.


================
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880
+  void foo13() {
+    if (mu.TryLock() ? 1 : 0)
+      mu.Unlock();
+  }
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D52888