[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGad4a51302777: [clang][CFG] Cleanup functions (authored by 
tbaeder).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/PathDiagnostic.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,68 @@
 }
   }
 }
+
+// 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)));
+}
+
+// CHECK:  [B1]
+// CHECK-NEXT:1: 10
+// CHECK-NEXT:2: i
+// CHECK-NEXT:3: [B1.2] = [B1.1]
+// CHECK-NEXT:4: return;
+// CHECK-NEXT:5: CleanupFunction (cleanup_int)
+// CHECK-NEXT:6: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B2]
+// CHECK-NEXT:1: return;
+// CHECK-NEXT:2: CleanupFunction (cleanup_int)
+// CHECK-NEXT:3: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B3]
+// CHECK-NEXT:1: CFGScopeBegin(i)
+// CHECK-NEXT:2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:3: m
+// CHECK-NEXT:4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:5: 1
+// CHECK-NEXT:6: [B3.4] == [B3.5]
+// CHECK-NEXT:T: if [B3.6]
+// CHECK-NEXT:Preds (1): B4
+// CHECK-NEXT:Succs (2): B2 B1
+void test_cleanup_functions2(int m) {
+  int i __attribute__((cleanup(cleanup_int)));
+
+  if (m == 1) {
+return;
+  }
+
+  i = 10;
+  return;
+}
+
+// CHECK:   [B1]
+// CHECK-NEXT:1: CFGScopeBegin(f)
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: __attribute__((cleanup(cleanup_F))) F f;
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)
+// CHECK-NEXT:5: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:6: CFGScopeEnd(f)
+// CHECK-NEXT:Preds (1): B2
+// CHECK-NEXT:Succs (1): B0
+class F {
+public:
+  ~F();
+};
+void cleanup_F(F *f);
+
+void test() {
+  F f __attribute((cleanup(cleanup_F)));
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -993,6 +993,7 @@
   ProcessLoopExit(E.castAs().getLoopStmt(), Pred);
   return;
 case CFGElement::LifetimeEnds:
+case CFGElement::CleanupFunction:
 case CFGElement::ScopeBegin:
 case CFGElement::ScopeEnd:
   return;
Index: clang/lib/Analysis/PathDiagnostic.cpp
===
--- clang/lib/Analysis/PathDiagnostic.cpp
+++ clang/lib/Analysis/PathDiagnostic.cpp
@@ -567,6 +567,7 @@
   }
   case CFGElement::ScopeBegin:
   case CFGElement::ScopeEnd:
+  case CFGElement::CleanupFunction:
 llvm_unreachable("not yet implemented!");
   case CFGElement::LifetimeEnds:
   case CFGElement::LoopExit:
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(const VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1861,14 +1866,14 @@
   if (B == E)
 return;
 
-  SmallVector DeclsNonTrivial;
-  DeclsNonTrivial.reserve(B.distance(E));
+  SmallVector DeclsNeedDestruction;
+  DeclsNeedDestruction.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-if (!hasTrivialDestructor(D))
-  DeclsNonTrivial.push_back(D);
+if (needsAutomaticDestruction(D))
+  DeclsNeedDestruction.push_back(D);
 
-  for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) {
+  for (VarDecl *VD : llvm::reverse(DeclsNeedDestruction)) {
 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
@@ -1879,7 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1472
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)

aaronpuchert wrote:
> The test failure in D152504 suggests that this needs to be written as
> ```
> __attribute__((cleanup(cleanup_F))) F f
> ```
> Maybe something changed upstream?
Right, not sure what changed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 556836.
tbaeder marked an inline comment as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/PathDiagnostic.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,68 @@
 }
   }
 }
+
+// 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)));
+}
+
+// CHECK:  [B1]
+// CHECK-NEXT:1: 10
+// CHECK-NEXT:2: i
+// CHECK-NEXT:3: [B1.2] = [B1.1]
+// CHECK-NEXT:4: return;
+// CHECK-NEXT:5: CleanupFunction (cleanup_int)
+// CHECK-NEXT:6: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B2]
+// CHECK-NEXT:1: return;
+// CHECK-NEXT:2: CleanupFunction (cleanup_int)
+// CHECK-NEXT:3: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B3]
+// CHECK-NEXT:1: CFGScopeBegin(i)
+// CHECK-NEXT:2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:3: m
+// CHECK-NEXT:4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:5: 1
+// CHECK-NEXT:6: [B3.4] == [B3.5]
+// CHECK-NEXT:T: if [B3.6]
+// CHECK-NEXT:Preds (1): B4
+// CHECK-NEXT:Succs (2): B2 B1
+void test_cleanup_functions2(int m) {
+  int i __attribute__((cleanup(cleanup_int)));
+
+  if (m == 1) {
+return;
+  }
+
+  i = 10;
+  return;
+}
+
+// CHECK:   [B1]
+// CHECK-NEXT:1: CFGScopeBegin(f)
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: __attribute__((cleanup(cleanup_F))) F f;
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)
+// CHECK-NEXT:5: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:6: CFGScopeEnd(f)
+// CHECK-NEXT:Preds (1): B2
+// CHECK-NEXT:Succs (1): B0
+class F {
+public:
+  ~F();
+};
+void cleanup_F(F *f);
+
+void test() {
+  F f __attribute((cleanup(cleanup_F)));
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -993,6 +993,7 @@
   ProcessLoopExit(E.castAs().getLoopStmt(), Pred);
   return;
 case CFGElement::LifetimeEnds:
+case CFGElement::CleanupFunction:
 case CFGElement::ScopeBegin:
 case CFGElement::ScopeEnd:
   return;
Index: clang/lib/Analysis/PathDiagnostic.cpp
===
--- clang/lib/Analysis/PathDiagnostic.cpp
+++ clang/lib/Analysis/PathDiagnostic.cpp
@@ -567,6 +567,7 @@
   }
   case CFGElement::ScopeBegin:
   case CFGElement::ScopeEnd:
+  case CFGElement::CleanupFunction:
 llvm_unreachable("not yet implemented!");
   case CFGElement::LifetimeEnds:
   case CFGElement::LoopExit:
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(const VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1861,14 +1866,14 @@
   if (B == E)
 return;
 
-  SmallVector DeclsNonTrivial;
-  DeclsNonTrivial.reserve(B.distance(E));
+  SmallVector DeclsNeedDestruction;
+  DeclsNeedDestruction.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-if (!hasTrivialDestructor(D))
-  DeclsNonTrivial.push_back(D);
+if (needsAutomaticDestruction(D))
+  DeclsNeedDestruction.push_back(D);
 
-  for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) {
+  for (VarDecl *VD : llvm::reverse(DeclsNeedDestruction)) {
 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
@@ -1879,7 +1884,8 @@
 Ty = getReferenceInitTemporaryType(VD->getInit());
   Ty = 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1472
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)

The test failure in D152504 suggests that this needs to be written as
```
__attribute__((cleanup(cleanup_F))) F f
```
Maybe something changed upstream?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

In D157385#4634591 , @tbaeder wrote:

> @steakhal Double lifetime ends should be fixed now.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked an inline comment as done.
tbaeder added a comment.

@steakhal Double lifetime ends should be fixed now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 555403.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/PathDiagnostic.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,68 @@
 }
   }
 }
+
+// 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)));
+}
+
+// CHECK:  [B1]
+// CHECK-NEXT:1: 10
+// CHECK-NEXT:2: i
+// CHECK-NEXT:3: [B1.2] = [B1.1]
+// CHECK-NEXT:4: return;
+// CHECK-NEXT:5: CleanupFunction (cleanup_int)
+// CHECK-NEXT:6: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B2]
+// CHECK-NEXT:1: return;
+// CHECK-NEXT:2: CleanupFunction (cleanup_int)
+// CHECK-NEXT:3: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B3]
+// CHECK-NEXT:1: CFGScopeBegin(i)
+// CHECK-NEXT:2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:3: m
+// CHECK-NEXT:4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:5: 1
+// CHECK-NEXT:6: [B3.4] == [B3.5]
+// CHECK-NEXT:T: if [B3.6]
+// CHECK-NEXT:Preds (1): B4
+// CHECK-NEXT:Succs (2): B2 B1
+void test_cleanup_functions2(int m) {
+  int i __attribute__((cleanup(cleanup_int)));
+
+  if (m == 1) {
+return;
+  }
+
+  i = 10;
+  return;
+}
+
+// CHECK:   [B1]
+// CHECK-NEXT:1: CFGScopeBegin(f)
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)
+// CHECK-NEXT:5: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:6: CFGScopeEnd(f)
+// CHECK-NEXT:Preds (1): B2
+// CHECK-NEXT:Succs (1): B0
+class F {
+public:
+  ~F();
+};
+void cleanup_F(F *f);
+
+void test() {
+  F f __attribute((cleanup(cleanup_F)));
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -993,6 +993,7 @@
   ProcessLoopExit(E.castAs().getLoopStmt(), Pred);
   return;
 case CFGElement::LifetimeEnds:
+case CFGElement::CleanupFunction:
 case CFGElement::ScopeBegin:
 case CFGElement::ScopeEnd:
   return;
Index: clang/lib/Analysis/PathDiagnostic.cpp
===
--- clang/lib/Analysis/PathDiagnostic.cpp
+++ clang/lib/Analysis/PathDiagnostic.cpp
@@ -565,6 +565,7 @@
   }
   case CFGElement::ScopeBegin:
   case CFGElement::ScopeEnd:
+  case CFGElement::CleanupFunction:
 llvm_unreachable("not yet implemented!");
   case CFGElement::LifetimeEnds:
   case CFGElement::LoopExit:
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(const VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1861,14 +1866,14 @@
   if (B == E)
 return;
 
-  SmallVector DeclsNonTrivial;
-  DeclsNonTrivial.reserve(B.distance(E));
+  SmallVector DeclsNeedDestruction;
+  DeclsNeedDestruction.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-if (!hasTrivialDestructor(D))
-  DeclsNonTrivial.push_back(D);
+if (needsAutomaticDestruction(D))
+  DeclsNeedDestruction.push_back(D);
 
-  for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) {
+  for (VarDecl *VD : llvm::reverse(DeclsNeedDestruction)) {
 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
@@ -1879,19 +1884,24 @@
 Ty = getReferenceInitTemporaryType(VD->getInit());
   Ty = Context->getBaseElementType(Ty);
 
-  if 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:1901-1902
   appendLifetimeEnds(Block, VD, S);
-if (BuildOpts.AddImplicitDtors)
+if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD))
   appendAutomaticObjDtor(Block, VD, S);
+if (HasCleanupAttr)

steakhal wrote:
> steakhal wrote:
> > This condition looks new. Is it an orthogonal improvement?
> 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
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 555251.
tbaeder marked an inline comment as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/PathDiagnostic.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,68 @@
 }
   }
 }
+
+// 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)));
+}
+
+// CHECK:  [B1]
+// CHECK-NEXT:1: 10
+// CHECK-NEXT:2: i
+// CHECK-NEXT:3: [B1.2] = [B1.1]
+// CHECK-NEXT:4: return;
+// CHECK-NEXT:5: CleanupFunction (cleanup_int)
+// CHECK-NEXT:6: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B2]
+// CHECK-NEXT:1: return;
+// CHECK-NEXT:2: CleanupFunction (cleanup_int)
+// CHECK-NEXT:3: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B3]
+// CHECK-NEXT:1: CFGScopeBegin(i)
+// CHECK-NEXT:2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:3: m
+// CHECK-NEXT:4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:5: 1
+// CHECK-NEXT:6: [B3.4] == [B3.5]
+// CHECK-NEXT:T: if [B3.6]
+// CHECK-NEXT:Preds (1): B4
+// CHECK-NEXT:Succs (2): B2 B1
+void test_cleanup_functions2(int m) {
+  int i __attribute__((cleanup(cleanup_int)));
+
+  if (m == 1) {
+return;
+  }
+
+  i = 10;
+  return;
+}
+
+// CHECK:   [B1]
+// CHECK-NEXT:1: CFGScopeBegin(f)
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)
+// CHECK-NEXT:5: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:6: CFGScopeEnd(f)
+// CHECK-NEXT:Preds (1): B2
+// CHECK-NEXT:Succs (1): B0
+class F {
+public:
+  ~F();
+};
+void cleanup_F(F *f);
+
+void test() {
+  F f __attribute((cleanup(cleanup_F)));
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -993,6 +993,7 @@
   ProcessLoopExit(E.castAs().getLoopStmt(), Pred);
   return;
 case CFGElement::LifetimeEnds:
+case CFGElement::CleanupFunction:
 case CFGElement::ScopeBegin:
 case CFGElement::ScopeEnd:
   return;
Index: clang/lib/Analysis/PathDiagnostic.cpp
===
--- clang/lib/Analysis/PathDiagnostic.cpp
+++ clang/lib/Analysis/PathDiagnostic.cpp
@@ -565,6 +565,7 @@
   }
   case CFGElement::ScopeBegin:
   case CFGElement::ScopeEnd:
+  case CFGElement::CleanupFunction:
 llvm_unreachable("not yet implemented!");
   case CFGElement::LifetimeEnds:
   case CFGElement::LoopExit:
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(const VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1861,14 +1866,14 @@
   if (B == E)
 return;
 
-  SmallVector DeclsNonTrivial;
-  DeclsNonTrivial.reserve(B.distance(E));
+  SmallVector DeclsNeedDestruction;
+  DeclsNeedDestruction.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-if (!hasTrivialDestructor(D))
-  DeclsNonTrivial.push_back(D);
+if (needsAutomaticDestruction(D))
+  DeclsNeedDestruction.push_back(D);
 
-  for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) {
+  for (VarDecl *VD : llvm::reverse(DeclsNeedDestruction)) {
 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
@@ -1879,19 +1884,24 @@
 Ty = getReferenceInitTemporaryType(VD->getInit());
   Ty = 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked 2 inline comments as done.
tbaeder added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:2113
+
+bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) const {
   // Check for const references bound to temporary. Set type to pointee.

steakhal wrote:
> Can't this accept `const VarDecl*` instead? That way we could get rid of that 
> `const_cast` above.
Yes, I just didn't want to modify the existing function signature.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1473-1474
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:5: CleanupFunction (cleanup_F)
+// CHECK-NEXT:6: CFGScopeEnd(f)

steakhal wrote:
> tbaeder wrote:
> > aaronpuchert wrote:
> > > Interesting test! But it seems CodeGen has them swapped: compiling this 
> > > snippet with `clang -c -S -emit-llvm` I get
> > > ```lang=LLVM
> > > 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.
> > Interesting, I thought I checked this and used the correct order. Will 
> > re-check, thanks.
> I believe this demonstrates the wrong order I also spotted earlier.
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.)



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};

steakhal wrote:
> aaronpuchert wrote:
> > tbaeder wrote:
> > > aaronpuchert wrote:
> > > > As with the cleanup function, a definition shouldn't be necessary.
> > > 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?
> > 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.
> Speaking of `noreturn`, I think it is worth demonstrating that if the cleanup 
> function has `noreturn` attribute, then the dtor is not called.
Yeah, that does not work correctly right now.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

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.




Comment at: clang/lib/Analysis/CFG.cpp:1901-1902
   appendLifetimeEnds(Block, VD, S);
-if (BuildOpts.AddImplicitDtors)
+if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD))
   appendAutomaticObjDtor(Block, VD, S);
+if (HasCleanupAttr)

This condition looks new. Is it an orthogonal improvement?



Comment at: clang/lib/Analysis/CFG.cpp:1901-1904
+if (BuildOpts.AddImplicitDtors && !hasTrivialDestructor(VD))
   appendAutomaticObjDtor(Block, VD, S);
+if (HasCleanupAttr)
+  appendCleanupFunction(Block, VD);

steakhal wrote:
> This condition looks new. Is it an orthogonal improvement?
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



Comment at: clang/lib/Analysis/CFG.cpp:2113
+
+bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) const {
   // Check for const references bound to temporary. Set type to pointee.

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



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1473-1474
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:5: CleanupFunction (cleanup_F)
+// CHECK-NEXT:6: CFGScopeEnd(f)

tbaeder wrote:
> aaronpuchert wrote:
> > Interesting test! But it seems CodeGen has them swapped: compiling this 
> > snippet with `clang -c -S -emit-llvm` I get
> > ```lang=LLVM
> > 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.
> Interesting, I thought I checked this and used the correct order. Will 
> re-check, thanks.
I believe this demonstrates the wrong order I also spotted earlier.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};

aaronpuchert wrote:
> tbaeder wrote:
> > aaronpuchert wrote:
> > > As with the cleanup function, a definition shouldn't be necessary.
> > 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?
> 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.
Speaking of `noreturn`, I think it is worth demonstrating that if the cleanup 
function has `noreturn` attribute, then the dtor is not called.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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




Comment at: clang/lib/Analysis/ThreadSafety.cpp:2429
 }
+
 case CFGElement::TemporaryDtor: {

Could you remove the added line?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 552608.
tbaeder marked 2 inline comments as done.
Herald added subscribers: steakhal, martong.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/PathDiagnostic.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,68 @@
 }
   }
 }
+
+// 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)));
+}
+
+// CHECK:  [B1]
+// CHECK-NEXT:1: 10
+// CHECK-NEXT:2: i
+// CHECK-NEXT:3: [B1.2] = [B1.1]
+// CHECK-NEXT:4: return;
+// CHECK-NEXT:5: CleanupFunction (cleanup_int)
+// CHECK-NEXT:6: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B2]
+// CHECK-NEXT:1: return;
+// CHECK-NEXT:2: CleanupFunction (cleanup_int)
+// CHECK-NEXT:3: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B3]
+// CHECK-NEXT:1: CFGScopeBegin(i)
+// CHECK-NEXT:2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:3: m
+// CHECK-NEXT:4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:5: 1
+// CHECK-NEXT:6: [B3.4] == [B3.5]
+// CHECK-NEXT:T: if [B3.6]
+// CHECK-NEXT:Preds (1): B4
+// CHECK-NEXT:Succs (2): B2 B1
+void test_cleanup_functions2(int m) {
+  int i __attribute__((cleanup(cleanup_int)));
+
+  if (m == 1) {
+return;
+  }
+
+  i = 10;
+  return;
+}
+
+// CHECK:   [B1]
+// CHECK-NEXT:1: CFGScopeBegin(f)
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: CleanupFunction (cleanup_F)
+// CHECK-NEXT:5: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:6: CFGScopeEnd(f)
+// CHECK-NEXT:Preds (1): B2
+// CHECK-NEXT:Succs (1): B0
+class F {
+public:
+  ~F();
+};
+void cleanup_F(F *f);
+
+void test() {
+  F f __attribute((cleanup(cleanup_F)));
+}
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -993,6 +993,7 @@
   ProcessLoopExit(E.castAs().getLoopStmt(), Pred);
   return;
 case CFGElement::LifetimeEnds:
+case CFGElement::CleanupFunction:
 case CFGElement::ScopeBegin:
 case CFGElement::ScopeEnd:
   return;
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2426,6 +2426,7 @@
 AD.getTriggerStmt()->getEndLoc());
   break;
 }
+
 case CFGElement::TemporaryDtor: {
   auto TD = BI.castAs();
 
Index: clang/lib/Analysis/PathDiagnostic.cpp
===
--- clang/lib/Analysis/PathDiagnostic.cpp
+++ clang/lib/Analysis/PathDiagnostic.cpp
@@ -565,6 +565,7 @@
   }
   case CFGElement::ScopeBegin:
   case CFGElement::ScopeEnd:
+  case CFGElement::CleanupFunction:
 llvm_unreachable("not yet implemented!");
   case CFGElement::LifetimeEnds:
   case CFGElement::LoopExit:
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1861,14 +1866,14 @@
   if (B == E)
 return;
 
-  SmallVector DeclsNonTrivial;
-  DeclsNonTrivial.reserve(B.distance(E));
+  SmallVector DeclsNeedDestruction;
+  DeclsNeedDestruction.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-if (!hasTrivialDestructor(D))
-  DeclsNonTrivial.push_back(D);
+if (needsAutomaticDestruction(D))
+ 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};

tbaeder wrote:
> aaronpuchert wrote:
> > As with the cleanup function, a definition shouldn't be necessary.
> 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?
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked an inline comment as done.
tbaeder added inline comments.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1473-1474
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:5: CleanupFunction (cleanup_F)
+// CHECK-NEXT:6: CFGScopeEnd(f)

aaronpuchert wrote:
> Interesting test! But it seems CodeGen has them swapped: compiling this 
> snippet with `clang -c -S -emit-llvm` I get
> ```lang=LLVM
> 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.
Interesting, I thought I checked this and used the correct order. Will 
re-check, thanks.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};

aaronpuchert wrote:
> As with the cleanup function, a definition shouldn't be necessary.
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1473-1474
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:5: CleanupFunction (cleanup_F)
+// CHECK-NEXT:6: CFGScopeEnd(f)

Interesting test! But it seems CodeGen has them swapped: compiling this snippet 
with `clang -c -S -emit-llvm` I get
```lang=LLVM
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.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1480
+public:
+  ~F() {}
+};

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/Analysis/ThreadSafety.cpp:2429-2438
+
+case CFGElement::CleanupFunction: {
+  const CFGCleanupFunction  = BI.castAs();
+
+  LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+SxBuilder.createVariable(CF.getVarDecl()),
+CF.getVarDecl()->getLocation());

aaronpuchert wrote:
> Should this be part of a follow-up? (For which you might revive D152504.)
Ah, yes, probably.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 552018.
tbaeder marked 4 inline comments as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,68 @@
 }
   }
 }
+
+// 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)));
+}
+
+// CHECK:  [B1]
+// CHECK-NEXT:1: 10
+// CHECK-NEXT:2: i
+// CHECK-NEXT:3: [B1.2] = [B1.1]
+// CHECK-NEXT:4: return;
+// CHECK-NEXT:5: CleanupFunction (cleanup_int)
+// CHECK-NEXT:6: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B2]
+// CHECK-NEXT:1: return;
+// CHECK-NEXT:2: CleanupFunction (cleanup_int)
+// CHECK-NEXT:3: CFGScopeEnd(i)
+// CHECK-NEXT:Preds (1): B3
+// CHECK-NEXT:Succs (1): B0
+// CHECK:  [B3]
+// CHECK-NEXT:1: CFGScopeBegin(i)
+// CHECK-NEXT:2: int i __attribute__((cleanup(cleanup_int)));
+// CHECK-NEXT:3: m
+// CHECK-NEXT:4: [B3.3] (ImplicitCastExpr, LValueToRValue, int)
+// CHECK-NEXT:5: 1
+// CHECK-NEXT:6: [B3.4] == [B3.5]
+// CHECK-NEXT:T: if [B3.6]
+// CHECK-NEXT:Preds (1): B4
+// CHECK-NEXT:Succs (2): B2 B1
+void test_cleanup_functions2(int m) {
+  int i __attribute__((cleanup(cleanup_int)));
+
+  if (m == 1) {
+return;
+  }
+
+  i = 10;
+  return;
+}
+
+// CHECK:   [B1]
+// CHECK-NEXT:1: CFGScopeBegin(f)
+// CHECK-NEXT:2:  (CXXConstructExpr, [B1.3], F)
+// CHECK-NEXT:3: F f __attribute__((cleanup(cleanup_F)));
+// CHECK-NEXT:4: [B1.3].~F() (Implicit destructor)
+// CHECK-NEXT:5: CleanupFunction (cleanup_F)
+// CHECK-NEXT:6: CFGScopeEnd(f)
+// CHECK-NEXT:Preds (1): B2
+// CHECK-NEXT:Succs (1): B0
+class F {
+public:
+  ~F() {}
+};
+void cleanup_F(F *f);
+
+void test() {
+  F f __attribute((cleanup(cleanup_F)));
+}
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2426,6 +2426,16 @@
 AD.getTriggerStmt()->getEndLoc());
   break;
 }
+
+case CFGElement::CleanupFunction: {
+  const CFGCleanupFunction  = BI.castAs();
+
+  LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+SxBuilder.createVariable(CF.getVarDecl()),
+CF.getVarDecl()->getLocation());
+  break;
+}
+
 case CFGElement::TemporaryDtor: {
   auto TD = BI.castAs();
 
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1861,14 +1866,14 @@
   if (B == E)
 return;
 
-  SmallVector DeclsNonTrivial;
-  DeclsNonTrivial.reserve(B.distance(E));
+  SmallVector DeclsNeedDestruction;
+  DeclsNeedDestruction.reserve(B.distance(E));
 
   for (VarDecl* D : llvm::make_range(B, E))
-if (!hasTrivialDestructor(D))
-  DeclsNonTrivial.push_back(D);
+if (needsAutomaticDestruction(D))
+  DeclsNeedDestruction.push_back(D);
 
-  for (VarDecl *VD : llvm::reverse(DeclsNonTrivial)) {
+  for (VarDecl *VD : llvm::reverse(DeclsNeedDestruction)) {
 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
@@ -1879,18 +1884,23 @@
 Ty = getReferenceInitTemporaryType(VD->getInit());
   Ty = Context->getBaseElementType(Ty);
 
-  if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
+  const CXXRecordDecl *CRD = Ty->getAsCXXRecordDecl();
+  if (CRD && CRD->isAnyDestructorNoReturn())
 Block = 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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




Comment at: clang/lib/Analysis/CFG.cpp:1874
+if (needsAutomaticDestruction(D))
   DeclsNonTrivial.push_back(D);
 

I'm wondering if you should rename this accordingly.



Comment at: clang/lib/Analysis/CFG.cpp:1887-1889
+  bool IsCXXRecordType = (Ty->getAsCXXRecordDecl() != nullptr);
+  if (IsCXXRecordType &&
+  Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())

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



Comment at: clang/lib/Analysis/CFG.cpp:5307-5308
+case CFGElement::CleanupFunction:
+llvm_unreachable("getDestructorDecl should only be used with "
+ "ImplicitDtors");
 case CFGElement::AutomaticObjectDtor: {

The unindent doesn't look right to me.



Comment at: clang/lib/Analysis/CFG.cpp:5850
 
+  case CFGElement::Kind::CleanupFunction: {
+OS << "CleanupFunction ("

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



Comment at: clang/lib/Analysis/ThreadSafety.cpp:2429-2438
+
+case CFGElement::CleanupFunction: {
+  const CFGCleanupFunction  = BI.castAs();
+
+  LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+SxBuilder.createVariable(CF.getVarDecl()),
+CF.getVarDecl()->getLocation());

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



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1428-1429
+// CHECK-NEXT:   4: CFGScopeEnd(i)
+void cleanup_int(int *i) {
+}
+

For our purposes, a pure declaration might be enough.



Comment at: clang/test/Analysis/scopes-cfg-output.cpp:1433
+  int i __attribute__((cleanup(cleanup_int)));
+}

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.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 551131.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,15 @@
 }
   }
 }
+
+// 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)));
+}
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2426,6 +2426,16 @@
 AD.getTriggerStmt()->getEndLoc());
   break;
 }
+
+case CFGElement::CleanupFunction: {
+  const CFGCleanupFunction  = BI.castAs();
+
+  LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+SxBuilder.createVariable(CF.getVarDecl()),
+CF.getVarDecl()->getLocation());
+  break;
+}
+
 case CFGElement::TemporaryDtor: {
   auto TD = BI.castAs();
 
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1346,7 +1350,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1865,7 +1870,7 @@
   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)) {
@@ -1879,18 +1884,24 @@
 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();
+
 // 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);
   }
 }
@@ -2095,7 +2106,12 @@
   return Scope;
 }
 
-bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) {
+bool CFGBuilder::needsAutomaticDestruction(const VarDecl *VD) const {
+  return !hasTrivialDestructor(const_cast(VD)) ||
+ VD->hasAttr();
+}
+
+bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) const {
   // Check for const references bound to temporary. Set type to pointee.
   QualType QT = VD->getType();
   if (QT->isReferenceType()) {
@@ -2149,7 +2165,7 @@
 return Scope;
 
   if (!BuildOpts.AddLifetime && !BuildOpts.AddScopes &&
-  hasTrivialDestructor(VD)) {
+  !needsAutomaticDestruction(VD)) {
 assert(BuildOpts.AddImplicitDtors);
 return Scope;
   }
@@ -5287,8 +5303,9 @@
 case CFGElement::CXXRecordTypedCall:
 case CFGElement::ScopeBegin:
 case CFGElement::ScopeEnd:
-  llvm_unreachable("getDestructorDecl should only be used with "
-   "ImplicitDtors");
+case CFGElement::CleanupFunction:
+llvm_unreachable("getDestructorDecl should only be used with "
+ "ImplicitDtors");
 case CFGElement::AutomaticObjectDtor: {
   const VarDecl *var = castAs().getVarDecl();
   QualType ty = var->getType();
@@ -5830,6 +5847,12 @@
 break;
   }
 
+  case CFGElement::Kind::CleanupFunction: {
+OS << "CleanupFunction ("
+   << 

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157385/new/

https://reviews.llvm.org/D157385

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157385: [clang][CFG] Cleanup functions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder created this revision.
tbaeder added reviewers: aaronpuchert, NoQ, clang.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This might be a little light on the testing side.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157385

Files:
  clang/include/clang/Analysis/CFG.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/Analysis/ThreadSafety.cpp
  clang/test/Analysis/scopes-cfg-output.cpp

Index: clang/test/Analysis/scopes-cfg-output.cpp
===
--- clang/test/Analysis/scopes-cfg-output.cpp
+++ clang/test/Analysis/scopes-cfg-output.cpp
@@ -1419,3 +1419,15 @@
 }
   }
 }
+
+// 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)));
+}
Index: clang/lib/Analysis/ThreadSafety.cpp
===
--- clang/lib/Analysis/ThreadSafety.cpp
+++ clang/lib/Analysis/ThreadSafety.cpp
@@ -2426,6 +2426,16 @@
 AD.getTriggerStmt()->getEndLoc());
   break;
 }
+
+case CFGElement::CleanupFunction: {
+  const CFGCleanupFunction  = BI.castAs();
+
+  LocksetBuilder.handleCall(nullptr, CF.getFunctionDecl(),
+SxBuilder.createVariable(CF.getVarDecl()),
+CF.getVarDecl()->getLocation());
+  break;
+}
+
 case CFGElement::TemporaryDtor: {
   auto TD = BI.castAs();
 
Index: clang/lib/Analysis/CFG.cpp
===
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -881,6 +881,10 @@
 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());
   }
@@ -1321,7 +1325,8 @@
 return {};
   }
 
-  bool hasTrivialDestructor(VarDecl *VD);
+  bool hasTrivialDestructor(VarDecl *VD) const;
+  bool needsAutomaticDestruction(const VarDecl *VD) const;
 };
 
 } // namespace
@@ -1840,32 +1845,39 @@
   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)) {
+QualType Ty = VD->getType();
+
+bool IsCXXRecordType = (Ty->getAsCXXRecordDecl() != nullptr);
 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())
+  if (IsCXXRecordType &&
+  Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
 Block = createNoReturnBlock();
 }
 
 autoCreateBlock();
 
+bool HasCleanupAttr = VD->hasAttr();
+
 // 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);
   }
 }
@@ -2070,7 +2082,12 @@
   return Scope;
 }
 
-bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) {
+bool CFGBuilder::needsAutomaticDestruction(const VarDecl *VD) const {
+  return !hasTrivialDestructor(const_cast(VD)) ||
+ VD->hasAttr();
+}
+
+bool CFGBuilder::hasTrivialDestructor(VarDecl *VD) const {
   // Check for const references bound to temporary. Set type to pointee.
   QualType QT = VD->getType();
   if (QT->isReferenceType()) {
@@ -2124,7 +2141,7 @@
 return Scope;
 
   if (!BuildOpts.AddLifetime && !BuildOpts.AddScopes &&
-  hasTrivialDestructor(VD)) {
+  !needsAutomaticDestruction(VD)) {
 assert(BuildOpts.AddImplicitDtors);
 return Scope;
   }
@@ -5262,8 +5279,9 @@
 case CFGElement::CXXRecordTypedCall:
 case CFGElement::ScopeBegin:
 case