Thread safety analysis: Handle additional cast in scoped capability construction

[aaronpuchert]

Summary

We might have a CK_NoOp cast and a further CK_ConstructorConversion.
As an optimization, drop some IgnoreParens calls: inside of the
CK_{Constructor,UserDefined}Conversion should be no more parentheses,
and inside the CXXBindTemporaryExpr should also be none.

Lastly, we factor out the unpacking so that we can reuse it for
MaterializeTemporaryExprs later on.

[aaronpuchert]

@efriedma, added you because this removes one of the IgnoreParens calls that you added in D76943, and that I think should be dead code. (The one after CXXBindTemporaryExpr.)

[aaron.ballman]

LGTM! Do you think this warrants a release note (or did it close any open issues in the tracker)?

clang/lib/Analysis/ThreadSafety.cpp

2091–2097

I almost wonder if we should just turn this into a while loop rather than two casts in a specific order. However, I can't think of a situation where we'd need the loop, so I think this is fine. At the very least, it's incremental progress!

[aaronpuchert]

In D129752#3682977, @aaron.ballman wrote:

Do you think this warrants a release note (or did it close any open issues in the tracker)?

It's probably too technical/minor to be worth mentioning. It also doesn't close any issues that I'm aware of, I mostly did it in preparation for D129755, where I'm reusing the new function.

clang/lib/Analysis/ThreadSafety.cpp

2091–2097

They should always be in that order, or at least until we get to the CXXConstructExpr or CXXMemberCallExpr that we want (there could be another standard conversion sequence for the argument inside of that, but that's not our concern here). The reason is that this constructor or conversion function call is the CK_ConstructorConversion or CK_UserDefinedConversion, respectively, while the remaining standard conversion sequence (to which the NoOp belongs) happens after/above that.

[aaron.ballman]

clang/lib/Analysis/ThreadSafety.cpp

2091–2097

Ah, thank you for confirming my intuition!

clang/lib/Analysis/ThreadSafety.cpp

static Expr *buildFakeCtorCall(CXXConstructorDecl *CD, ArrayRef<Expr *> Args,
                                SourceLocation Loc) {
   ASTContext &Ctx = CD->getASTContext();
   return CXXConstructExpr::Create(Ctx, Ctx.getRecordType(CD->getParent()), Loc,
                                   CD, true, Args, false, false, false, false,
                                   CXXConstructExpr::CK_Complete,
                                   SourceRange(Loc, Loc));
 }
 
+static Expr *UnpackConstruction(Expr *E) {
+  if (auto *CE = dyn_cast<CastExpr>(E))
+    if (CE->getCastKind() == CK_NoOp)
+      E = CE->getSubExpr()->IgnoreParens();
+  if (auto *CE = dyn_cast<CastExpr>(E))
+    if (CE->getCastKind() == CK_ConstructorConversion ||
+        CE->getCastKind() == CK_UserDefinedConversion)
+      E = CE->getSubExpr();
+  if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
+    E = BTE->getSubExpr();
+  return E;
+}
+
 void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
   // adjust the context
   LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
 
   for (auto *D : S->getDeclGroup()) {
     if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {
       Expr *E = VD->getInit();
       if (!E)
         continue;
       E = E->IgnoreParens();
 
       // handle constructors that involve temporaries
       if (auto *EWC = dyn_cast<ExprWithCleanups>(E))
         E = EWC->getSubExpr()->IgnoreParens();
-      if (auto *CE = dyn_cast<CastExpr>(E))
-        if (CE->getCastKind() == CK_NoOp ||
-            CE->getCastKind() == CK_ConstructorConversion ||
-            CE->getCastKind() == CK_UserDefinedConversion)
-          E = CE->getSubExpr()->IgnoreParens();
-      if (auto *BTE = dyn_cast<CXXBindTemporaryExpr>(E))
-        E = BTE->getSubExpr()->IgnoreParens();
+      E = UnpackConstruction(E);
 
       if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
         const auto *CtorD = dyn_cast_or_null<NamedDecl>(CE->getConstructor());
         if (!CtorD || !CtorD->hasAttrs())
           continue;
         handleCall(E, CtorD, VD);
       } else if (isa<CallExpr>(E) && E->isPRValue()) {
         // If the object is initialized by a function call that returns a

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

struct TestScopedLockable {
 
   bool getBool();
 
   void foo1() {
     MutexLock mulock(&mu1);
     a = 5;
   }
 
+#ifdef __cpp_guaranteed_copy_elision
+  void const_lock() {
+    const MutexLock mulock = MutexLock(&mu1);
+    a = 5;
+  }
+#endif
+
   void foo2() {
     ReaderMutexLock mulock1(&mu1);
     if (getBool()) {
       MutexLock mulock2a(&mu2);
       b = a + 1;
     }
     else {
       MutexLock mulock2b(&mu2);