Thread safety analysis: Handle conditional expression in getTrylockCallExpr

[aaronpuchert]

Summary

We unwrap conditional expressions containing try-lock functions.

Additionally we don't branch on conditionals, since that is usually not
helpful. When joining the branches we would almost certainly get a
warning otherwise.

Hopefully fixes an issue that was raised in D52398.

[aaronpuchert]

By the way, the tests also work with __builtin_expect, but I didn't want to blow up the code too much.

[hokein]

Thanks for the quick fix! I'm not the right person to do the review, but I verified that the warnings are gone with your patch, thanks!

[aaron.ballman]

lib/Analysis/ThreadSafety.cpp

1445 ↗

(On Diff #168295)

Rather than do an assignment here, why not just pass !Negate directly below, since you're returning?

test/SemaCXX/warn-thread-safety-analysis.cpp

1879–1880 ↗

(On Diff #168295)

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

[aaronpuchert]

lib/Analysis/ThreadSafety.cpp

1445 ↗

(On Diff #168295)

The third parameter of getTrylockCallExpr is a (non-const) reference, so I can only pass an lvalue. It's basically a call-by-reference pattern.

test/SemaCXX/warn-thread-safety-analysis.cpp

1879–1880 ↗

(On Diff #168295)

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, 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. Personally I would prefer to use an if-statement if one of the expressions has side-effects.)

Tell me what you think.

[aaron.ballman]

lib/Analysis/ThreadSafety.cpp

1445 ↗

(On Diff #168295)

Ah, I missed that when looking at the code in Phab, thanks for the explanation!

test/SemaCXX/warn-thread-safety-analysis.cpp

1879–1880 ↗

(On Diff #168295)

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
}

[aaronpuchert]

test/SemaCXX/warn-thread-safety-analysis.cpp

1879–1880 ↗

(On Diff #168295)

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 ConditionalExprs so that we can derive which branch holds the lock even when these are used.

[aaronpuchert]

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?

[aaron.ballman]

In 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.

test/SemaCXX/warn-thread-safety-analysis.cpp

1879–1880 ↗

(On Diff #168295)

Ah, okay, thank you for the further explanation. That makes me a bit more comfortable with the proposed approach.

[aaronpuchert]

In D52888#1256395, @aaron.ballman wrote:

In 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.

It's relatively short, but not exactly straightforward, because we have to check if both branches are "compatible". However, thinking about this again I think most programmers would write foo16 as

void foo16() {
  if (cond && mu.TryLock())
    mu.Unlock();
}

which is functionally equivalent, and which we already support. So I'd propose to add support for this only when someone asks for it, and leave it as it is for now.

[aaron.ballman]

In D52888#1256625, @aaronpuchert wrote:

In D52888#1256395, @aaron.ballman wrote:

In 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.

It's relatively short, but not exactly straightforward, because we have to check if both branches are "compatible". However, thinking about this again I think most programmers would write foo16 as

void foo16() {
  if (cond && mu.TryLock())
    mu.Unlock();
}

which is functionally equivalent, and which we already support. So I'd propose to add support for this only when someone asks for it, and leave it as it is for now.

I think that's reasonable. LGTM!

cfe/trunk/lib/Analysis/ThreadSafety.cpp

else if (const auto *BOP = dyn_cast<BinaryOperator>(Cond)) {
     }
     if (BOP->getOpcode() == BO_LAnd) {
       // LHS must have been evaluated in a different block.
       return getTrylockCallExpr(BOP->getRHS(), C, Negate);
     }
     if (BOP->getOpcode() == BO_LOr)
       return getTrylockCallExpr(BOP->getRHS(), C, Negate);
     return nullptr;
+  } else if (const auto *COP = dyn_cast<ConditionalOperator>(Cond)) {
+    bool TCond, FCond;
+    if (getStaticBooleanValue(COP->getTrueExpr(), TCond) &&
+        getStaticBooleanValue(COP->getFalseExpr(), FCond)) {
+      if (TCond && !FCond)
+        return getTrylockCallExpr(COP->getCond(), C, Negate);
+      if (!TCond && FCond) {
+        Negate = !Negate;
+        return getTrylockCallExpr(COP->getCond(), C, Negate);
+      }
+    }
   }
   return nullptr;
 }
 
 /// Find the lockset that holds on the edge between PredBlock
 /// and CurrBlock.  The edge set is the exit set of PredBlock (passed
 /// as the ExitSet parameter) plus any trylocks, which are conditionally held.
 void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
                                           const FactSet &ExitSet,
                                           const CFGBlock *PredBlock,
                                           const CFGBlock *CurrBlock) {
   Result = ExitSet;
 
   const Stmt *Cond = PredBlock->getTerminatorCondition();
-  if (!Cond)
+  // We don't acquire try-locks on ?: branches, only when its result is used.
+  if (!Cond || isa<ConditionalOperator>(PredBlock->getTerminator()))
     return;
 
   bool Negate = false;
   const CFGBlockInfo *PredBlockInfo = &BlockInfo[PredBlock->getBlockID()];
   const LocalVarContext &LVarCtx = PredBlockInfo->ExitContext;
   StringRef CapDiagKind = "mutex";
 
   const auto *Exp = getTrylockCallExpr(Cond, LVarCtx, Negate);

cfe/trunk/test/SemaCXX/warn-thread-safety-analysis.cpp

if (!mu.ReaderTryLock())
        return;
    }
    else {
      mu.ReaderLock();
    }
    int i = a;
    mu.Unlock();
   }
+
+  // Test with conditional operator
+  void foo13() {
+    if (mu.TryLock() ? 1 : 0)
+      mu.Unlock();
+  }
+
+  void foo14() {
+    if (mu.TryLock() ? 0 : 1)
+      return;
+    mu.Unlock();
+  }
+
+  void foo15() {
+    if (mu.TryLock() ? 0 : 1) // expected-note{{mutex acquired here}}
+      mu.Unlock();            // expected-warning{{releasing mutex 'mu' that was not held}}
+  }                           // expected-warning{{mutex 'mu' is not held on every path through here}}
 };  // end TestTrylock
 
 } // end namespace TrylockTest
 
 
 namespace TestTemplateAttributeInstantiation {
 
 class Foo1 {