[clang][CFG] Cleanup functions

[tbaeder]

Summary

This might be a little light on the testing side.

[tbaeder]

Ping

[aaronpuchert]

For me this looks good, but I'd like @NoQ to sign off on it.

clang/lib/Analysis/CFG.cpp

1874

I'm wondering if you should rename this accordingly.

1887–1889

Here I'd suggest to deduplicate Ty->getAsCXXRecordDecl(). Implicit conversion of pointers to bool is idiomatic in LLVM.

5307–5308

The unindent doesn't look right to me.

5850

Braces shouldn't be needed if you don't declare any variables.

clang/lib/Analysis/ThreadSafety.cpp

2429–2438

Should this be part of a follow-up? (For which you might revive D152504.)

clang/test/Analysis/scopes-cfg-output.cpp

1428–1429

For our purposes, a pure declaration might be enough.

1433

Ideas for more tests (apart from imitating destructor tests):

• A variable in a block, so that more statements run before the function returns.
• A function with multiple return paths, each of which has to run the cleanup.

[tbaeder]

clang/lib/Analysis/ThreadSafety.cpp

2429–2438

Ah, yes, probably.

[aaronpuchert]

clang/test/Analysis/scopes-cfg-output.cpp

1473–1474

Interesting test! But it seems CodeGen has them swapped: compiling this snippet with clang -c -S -emit-llvm I get

define dso_local void @_Z4testv() #0 personality ptr @__gxx_personality_v0 {
  %1 = alloca %class.F, align 1
  %2 = alloca ptr, align 8
  %3 = alloca i32, align 4
  invoke void @_Z9cleanup_FP1F(ptr noundef %1)
          to label %4 unwind label %5

4:                                                ; preds = %0
  call void @_ZN1FD2Ev(ptr noundef nonnull align 1 dereferenceable(1) %1) #3
  ret void

; ...
}

So first cleanup, then destructor. This is with 17.0.0-rc2.

1480

As with the cleanup function, a definition shouldn't be necessary.

[tbaeder]

clang/test/Analysis/scopes-cfg-output.cpp

1473–1474

Interesting, I thought I checked this and used the correct order. Will re-check, thanks.

1480

Is there a way to test whether the contents of the cleanup function are being checked as well? From these tests, I only know we consider them called, but not whether we (properly) analyze their bodies in the context as well. Or is that separate from this patch?

[aaronpuchert]

clang/test/Analysis/scopes-cfg-output.cpp

1480

For now we're just adding a new element to the CFG and adapting the respective tests. The CFG is generated on a per-function basis, and the generation of one function's CFG will never look into another function's body. It might use some (declaration) properties of course, like whether it has [[noreturn]] or noexcept. Of course we should also generate a CFG for the cleanup function, but that's independent of this change.

Users of the CFG will naturally need to be taught about this new element type to “understand” it. Otherwise they should simply skip it. Since the CFG previously did not contain such elements, there should be no change for them. So we can also initially just add the element and not tell anybody about it.

We could also add understanding of the new element type to other CFG users, but you don't have to do this. If you only care about Thread Safety Analysis, then it's totally fine to handle it only there.

But let's move all changes to Thread Safety Analysis into a follow-up, so that we don't have to bother the CFG maintainers with that.

[aaronpuchert]

Looks good to me, but let's wait for the CFG maintainers to approve it.

clang/lib/Analysis/ThreadSafety.cpp

2429

Could you remove the added line?

[steakhal]

This is a great improvement. When I saw that clang now supports it and e.g. the CSA operates on the CFG, I also considered adding this.
Now I don't need to do it, so many thanks!

The code looks correct to me, except that the cleanup should be before the dtor if it has any. Other than that, the code coverage also looks great, thus I would not oppose.

clang/lib/Analysis/CFG.cpp

1902–1905

This condition looks new. Is it an orthogonal improvement?

1902–1906

Shouldn't the cleanup function run first, and then the dtor of the variable?
This way the object is already "dead" even before reaching the cleanup handler.
https://godbolt.org/z/sT65boooW

2114

Can't this accept const VarDecl* instead? That way we could get rid of that const_cast above.

clang/test/Analysis/scopes-cfg-output.cpp

1473–1474

I believe this demonstrates the wrong order I also spotted earlier.

1480

Speaking of noreturn, I think it is worth demonstrating that if the cleanup function has noreturn attribute, then the dtor is not called.

[tbaeder]

clang/lib/Analysis/CFG.cpp

2114

Yes, I just didn't want to modify the existing function signature.

clang/test/Analysis/scopes-cfg-output.cpp

1473–1474

The current code generates the given CFG, i.e. the cleanup function comes first, followed by the dtor:

[B1]
  1: CFGScopeBegin(f)
  2:  (CXXConstructExpr, [B1.3], F)
  3: F f __attribute__((cleanup(f_)));
  4: CleanupFunction (f_)
  5: [B1.3].~F() (Implicit destructor)
  6: CFGScopeEnd(f)
  Preds (1): B2
  Succs (1): B0

(The comment from Aaron is from before when I had the order swapped.)

1480

Yeah, that does not work correctly right now.

[tbaeder]

Not sure I have enough CFG knowledge. Do I just need to create another noreturn block for the cleanup function?

This is the CFG I get when both the cleanup function and the destructor are noreturn:

int main()
 [B4 (ENTRY)]
   Succs (1): B3

 [B1]
   1: CFGScopeEnd(f)
   Succs (1): B0

 [B2 (NORETURN)]
   1: [B3.3].~F() (Implicit destructor)
   Succs (1): B0

 [B3 (NORETURN)]
   1: CFGScopeBegin(f)
   2:  (CXXConstructExpr, [B3.3], F)
   3: F f __attribute__((cleanup(f_)));
   4: CleanupFunction (f_)
   Preds (1): B4
   Succs (1): B0

 [B0 (EXIT)]
   Preds (3): B1 B2 B3

[tbaeder]

clang/lib/Analysis/CFG.cpp

1902–1905

This check is needed because it's not implied anymore by the variable being in the list. It might be in there because it has a cleanup function.

[steakhal]

When I added -analyzer-config cfg-lifetime=true to clang/test/Analysis/scopes-cfg-output.cpp, suddenly duplicated lifetime ends entries appeared where we have CleanupFunctions.
My output is:

void test_cleanup_functions()
 [B2 (ENTRY)]
   Succs (1): B1

 [B1]
   1: CFGScopeBegin(i)
   2: int i __attribute__((cleanup(cleanup_int)));
   3: CleanupFunction (cleanup_int)
   4: [B1.2] (Lifetime ends)
   5: [B1.2] (Lifetime ends)
   6: CFGScopeEnd(i)
   Preds (1): B2
   Succs (1): B0

 [B0 (EXIT)]
   Preds (1): B1

void test_cleanup_functions2(int m)
 [B4 (ENTRY)]
   Succs (1): B3

 [B1]
   1: 10
   2: i
   3: [B1.2] = [B1.1]
   4: return;
   5: CleanupFunction (cleanup_int)
   6: [B3.2] (Lifetime ends)
   7: [B3.2] (Lifetime ends)
   8: CFGScopeEnd(i)
   Preds (1): B3
   Succs (1): B0

 [B2]
   1: return;
   2: CleanupFunction (cleanup_int)
   3: [B3.2] (Lifetime ends)
   4: [B3.2] (Lifetime ends)
   5: CFGScopeEnd(i)
   Preds (1): B3
   Succs (1): B0

 [B3]
   1: CFGScopeBegin(i)
   2: int i __attribute__((cleanup(cleanup_int)));
   3: m
   4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
   5: 1
   6: [B3.4] == [B3.5]
   T: if [B3.6]
   Preds (1): B4
   Succs (2): B2 B1

 [B0 (EXIT)]
   Preds (2): B1 B2

void test()
 [B2 (ENTRY)]
   Succs (1): B1

 [B1]
   1: CFGScopeBegin(f)
   2:  (CXXConstructExpr, [B1.3], F)
   3: F f __attribute__((cleanup(cleanup_F)));
   4: CleanupFunction (cleanup_F)
   5: [B1.3].~F() (Implicit destructor)
   6: [B1.3] (Lifetime ends)
   7: CFGScopeEnd(f)
   Preds (1): B2
   Succs (1): B0

 [B0 (EXIT)]
   Preds (1): B1

Notice the [B3.2] (Lifetime ends) lines for example.

The order in which the Lifetime, Scope and Cleanup elements appear looks correct; my only concern is the duplicate Lifetime ends marker.

About the noreturn cleanup function, well, GCC says: It is undefined what happens if cleanup_function does not return normally. here, thus I'm not sure what to do in that case. GCC seems to optimize accordingly, but clang does not. See https://godbolt.org/z/z8s6bPPjv.

FYI Unfortunately, I don't have much experience with CFG either.

[tbaeder]

@steakhal Double lifetime ends should be fixed now.

[steakhal]

In D157385#4634591, @tbaeder wrote:

@steakhal Double lifetime ends should be fixed now.

I haven't verified, but it should be good now.

[aaronpuchert]

clang/test/Analysis/scopes-cfg-output.cpp

1472

The test failure in D152504 suggests that this needs to be written as

__attribute__((cleanup(cleanup_F))) F f

Maybe something changed upstream?

[tbaeder]

clang/test/Analysis/scopes-cfg-output.cpp

1472

Right, not sure what changed.

clang/include/clang/Analysis/CFG.h

 //===- CFG.h - Classes for representing and building CFGs -------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the CFG and CFGBuilder classes for representing and
 //  building Control-Flow Graphs (CFGs) from ASTs.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_ANALYSIS_CFG_H
 #define LLVM_CLANG_ANALYSIS_CFG_H
 
-#include "clang/Analysis/Support/BumpVector.h"
-#include "clang/Analysis/ConstructionContext.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
+#include "clang/Analysis/ConstructionContext.h"
+#include "clang/Analysis/Support/BumpVector.h"
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/GraphTraits.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/raw_ostream.h"
 #include <bitset>
enum Kind {
     STMT_END = CXXRecordTypedCall,
     // dtor kind
     AutomaticObjectDtor,
     DeleteDtor,
     BaseDtor,
     MemberDtor,
     TemporaryDtor,
     DTOR_BEGIN = AutomaticObjectDtor,
-    DTOR_END = TemporaryDtor
+    DTOR_END = TemporaryDtor,
+    CleanupFunction,
   };
 
 protected:
   // The int bits are used to mark the kind.
   llvm::PointerIntPair<void *, 2> Data1;
   llvm::PointerIntPair<void *, 2> Data2;
 
   CFGElement(Kind kind, const void *Ptr1, const void *Ptr2 = nullptr)
private:
   friend class CFGElement;
 
   static bool isKind(const CFGElement &E) {
     Kind kind = E.getKind();
     return kind >= DTOR_BEGIN && kind <= DTOR_END;
   }
 };
 
+class CFGCleanupFunction final : public CFGElement {
+public:
+  CFGCleanupFunction() = default;
+  CFGCleanupFunction(const VarDecl *VD)
+      : CFGElement(Kind::CleanupFunction, VD) {
+    assert(VD->hasAttr<CleanupAttr>());
+  }
+
+  const VarDecl *getVarDecl() const {
+    return static_cast<VarDecl *>(Data1.getPointer());
+  }
+
+  /// Returns the function to be called when cleaning up the var decl.
+  const FunctionDecl *getFunctionDecl() const {
+    const CleanupAttr *A = getVarDecl()->getAttr<CleanupAttr>();
+    return A->getFunctionDecl();
+  }
+
+private:
+  friend class CFGElement;
+
+  static bool isKind(const CFGElement E) {
+    return E.getKind() == Kind::CleanupFunction;
+  }
+};
+
 /// Represents C++ object destructor implicitly generated for automatic object
 /// or temporary bound to const reference at the point of leaving its local
 /// scope.
 class CFGAutomaticObjDtor: public CFGImplicitDtor {
 public:
   CFGAutomaticObjDtor(const VarDecl *var, const Stmt *stmt)
       : CFGImplicitDtor(AutomaticObjectDtor, var, stmt) {}
 
public:
   void appendTemporaryDtor(CXXBindTemporaryExpr *E, BumpVectorContext &C) {
     Elements.push_back(CFGTemporaryDtor(E), C);
   }
 
   void appendAutomaticObjDtor(VarDecl *VD, Stmt *S, BumpVectorContext &C) {
     Elements.push_back(CFGAutomaticObjDtor(VD, S), C);
   }
 
+  void appendCleanupFunction(const VarDecl *VD, BumpVectorContext &C) {
+    Elements.push_back(CFGCleanupFunction(VD), C);
+  }
+
   void appendLifetimeEnds(VarDecl *VD, Stmt *S, BumpVectorContext &C) {
     Elements.push_back(CFGLifetimeEnds(VD, S), C);
   }
 
   void appendLoopExit(const Stmt *LoopStmt, BumpVectorContext &C) {
     Elements.push_back(CFGLoopExit(LoopStmt), C);
   }
 

clang/lib/Analysis/CFG.cpp

private:
   void appendTemporaryDtor(CFGBlock *B, CXXBindTemporaryExpr *E) {
     B->appendTemporaryDtor(E, cfg->getBumpVectorContext());
   }
 
   void appendAutomaticObjDtor(CFGBlock *B, VarDecl *VD, Stmt *S) {
     B->appendAutomaticObjDtor(VD, S, cfg->getBumpVectorContext());
   }
 
+  void appendCleanupFunction(CFGBlock *B, VarDecl *VD) {
+    B->appendCleanupFunction(VD, cfg->getBumpVectorContext());
+  }
+
   void appendLifetimeEnds(CFGBlock *B, VarDecl *VD, Stmt *S) {
     B->appendLifetimeEnds(VD, S, cfg->getBumpVectorContext());
   }
 
   void appendLoopExit(CFGBlock *B, const Stmt *LoopStmt) {
     B->appendLoopExit(LoopStmt, cfg->getBumpVectorContext());
   }
 
TryResult evaluateAsBooleanConditionNoCache(Expr *E) {
 
     bool Result;
     if (E->EvaluateAsBooleanCondition(Result, *Context))
       return Result;
 
     return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
 
 Expr *
 clang::extractElementInitializerFromNestedAILE(const ArrayInitLoopExpr *AILE) {
   if (!AILE)
     return nullptr;
void CFGBuilder::addAutomaticObjDestruction(LocalScope::const_iterator B,
 
   if (B == E)
     return;
 
   SmallVector<VarDecl *, 10> DeclsNonTrivial;
   DeclsNonTrivial.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-    if (!hasTrivialDestructor(D))
+    if (needsAutomaticDestruction(D))
       DeclsNonTrivial.push_back(D);
 
   for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) {
     if (BuildOpts.AddImplicitDtors) {
       // If this destructor is marked as a no-return destructor, we need to
       // create a new block for the destructor which does not have as a
       // successor anything built thus far: control won't flow out of this
       // block.
       QualType Ty = VD->getType();
       if (Ty->isReferenceType())
         Ty = getReferenceInitTemporaryType(VD->getInit());
       Ty = Context->getBaseElementType(Ty);
 
-      if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
+      bool IsCXXRecordType = (Ty->getAsCXXRecordDecl() != nullptr);
+      if (IsCXXRecordType &&
+          Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
         Block = createNoReturnBlock();
     }
 
     autoCreateBlock();
 
+    bool HasCleanupAttr = VD->hasAttr<CleanupAttr>();
+
     // Add LifetimeEnd after automatic obj with non-trivial destructors,
     // as they end their lifetime when the destructor returns. For trivial
     // objects, we end lifetime with scope end.
     if (BuildOpts.AddLifetime)
       appendLifetimeEnds(Block, VD, S);
-    if (BuildOpts.AddImplicitDtors)
+    if (HasCleanupAttr)
+      appendCleanupFunction(Block, VD);
+    if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD))
       appendAutomaticObjDtor(Block, VD, S);
   }
 }
 
 /// Add CFG elements corresponding to leaving a scope.
 /// Assumes that range [B, E) corresponds to single scope.
 /// This add following elements:
 ///   * LifetimeEnds for all variables with non-trivial destructor
 ///   * ScopeEnd for each scope left
 void CFGBuilder::addScopeExitHandling(LocalScope::const_iterator B,
if (!BuildOpts.AddImplicitDtors && !BuildOpts.AddLifetime &&
     return Scope;
 
   for (auto *DI : DS->decls())
     if (VarDecl *VD = dyn_cast<VarDecl>(DI))
       Scope = addLocalScopeForVarDecl(VD, Scope);
   return Scope;
 }
 
-bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) {
+bool CFGBuilder::needsAutomaticDestruction(const VarDecl *VD) const {
+  return !hasTrivialDestructor(const_cast<VarDecl *>(VD)) ||
+         VD->hasAttr<CleanupAttr>();
+}
+
+bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) const {
   // Check for const references bound to temporary. Set type to pointee.
   QualType QT = VD->getType();
   if (QT->isReferenceType()) {
     // Attempt to determine whether this declaration lifetime-extends a
     // temporary.
     //
     // FIXME: This is incorrect. Non-reference declarations can lifetime-extend
     // temporaries, and a single declaration can extend multiple temporaries.
if (!BuildOpts.AddImplicitDtors && !BuildOpts.AddLifetime &&
       !BuildOpts.AddScopes)
     return Scope;
 
   // Check if variable is local.
   if (!VD->hasLocalStorage())
     return Scope;
 
   if (!BuildOpts.AddLifetime && !BuildOpts.AddScopes &&
-      hasTrivialDestructor(VD)) {
+      !needsAutomaticDestruction(VD)) {
     assert(BuildOpts.AddImplicitDtors);
     return Scope;
   }
 
   // Add the variable to scope
   Scope = createOrReuseLocalScope(Scope);
   Scope->addVar(VD);
   ScopePos = Scope->begin();
switch (getKind()) {
     case CFGElement::NewAllocator:
     case CFGElement::LoopExit:
     case CFGElement::LifetimeEnds:
     case CFGElement::Statement:
     case CFGElement::Constructor:
     case CFGElement::CXXRecordTypedCall:
     case CFGElement::ScopeBegin:
     case CFGElement::ScopeEnd:
-      llvm_unreachable("getDestructorDecl should only be used with "
-                       "ImplicitDtors");
+    case CFGElement::CleanupFunction:
     case CFGElement::AutomaticObjectDtor: {
       const VarDecl *var = castAs<CFGAutomaticObjDtor>().getVarDecl();
       QualType ty = var->getType();
 
       // FIXME: See CFGBuilder::addLocalScopeForVarDecl.
       //
       // Lifetime-extending constructs are handled here. This works for a single
       // temporary in an initializer expression.
if (T->isReferenceType())
       T = getReferenceInitTemporaryType(VD->getInit(), nullptr);
 
     OS << ".~";
     T.getUnqualifiedType().print(OS, PrintingPolicy(Helper.getLangOpts()));
     OS << "() (Implicit destructor)\n";
     break;
   }
 
+  case CFGElement::Kind::CleanupFunction: {
+    OS << "CleanupFunction ("
+       << E.castAs<CFGCleanupFunction>().getFunctionDecl()->getName() << ")\n";
+    break;
+  }
+
   case CFGElement::Kind::LifetimeEnds:
     Helper.handleDecl(E.castAs<CFGLifetimeEnds>().getVarDecl(), OS);
     OS << " (Lifetime ends)\n";
     break;
 
   case CFGElement::Kind::LoopExit:
     OS << E.castAs<CFGLoopExit>().getLoopStmt()->getStmtClassName() << " (LoopExit)\n";
     break;

clang/lib/Analysis/ThreadSafety.cpp

for (const auto &BI : *CurrBlock) {
           if (!DD->hasAttrs())
             break;
 
           LocksetBuilder.handleCall(nullptr, DD,
                                     SxBuilder.createVariable(AD.getVarDecl()),
                                     AD.getTriggerStmt()->getEndLoc());
           break;
         }
+
+        case CFGElement::CleanupFunction: {
+          const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>();
+
+          LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+                                    SxBuilder.createVariable(CF.getVarDecl()),
+                                    CF.getVarDecl()->getLocation());
+          break;
+        }
+
         case CFGElement::TemporaryDtor: {
           auto TD = BI.castAs<CFGTemporaryDtor>();
 
           // Clean up constructed object even if there are no attributes to
           // keep the number of objects in limbo as small as possible.
           if (auto Object = LocksetBuilder.ConstructedObjects.find(
                   TD.getBindTemporaryExpr()->getSubExpr());
               Object != LocksetBuilder.ConstructedObjects.end()) {

clang/test/Analysis/scopes-cfg-output.cpp

if (UV) goto label;
     if (UV) goto label;
     {
       int n2t;
       if (UV) goto label;
 label:
     }
   }
 }
+
+// CHECK:      [B1]
+// CHECK-NEXT:   1: CFGScopeBegin(i)
+// CHECK-NEXT:   2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:   3: CleanupFunction (cleanup_int)
+// CHECK-NEXT:   4: CFGScopeEnd(i)
+void cleanup_int(int *i) {
+}
+
+void test_cleanup_functions() {
+  int i __attribute__((cleanup(cleanup_int)));
+}