[clang] Diagnose misuse of the cleanup attribute (PR #80040)

Bhuminjay Soni via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 6 23:58:32 PST 2024


https://github.com/11happy updated https://github.com/llvm/llvm-project/pull/80040

>From 93adb872d0e18ff3a1356ab47527d81b61c920cd Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Tue, 30 Jan 2024 23:19:04 +0530
Subject: [PATCH 1/8] Diagnose misuse of the cleanup attribute

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 .../include/clang/Basic/DiagnosticSemaKinds.td  |  4 ++++
 clang/include/clang/Sema/Sema.h                 |  2 ++
 clang/lib/Sema/SemaDeclAttr.cpp                 |  7 +++++++
 clang/lib/Sema/SemaExpr.cpp                     | 17 +++++++++++++++++
 4 files changed, 30 insertions(+)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 24d32cb87c89e..99ef803b1e0ec 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8265,6 +8265,10 @@ def warn_condition_is_assignment : Warning<"using the result of an "
 def warn_free_nonheap_object
   : Warning<"attempt to call %0 on non-heap %select{object %2|object: block expression|object: lambda-to-function-pointer conversion}1">,
     InGroup<FreeNonHeapObject>;
+def warn_free_called_on_unallocated_object : Warning<
+  "'%0' called on unallocated object '%1'">,
+  InGroup<FreeNonHeapObject>;
+  
 
 // Completely identical except off by default.
 def warn_condition_is_idiomatic_assignment : Warning<"using the result "
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index b5946e3f3178f..535c479aeb7c5 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -12932,6 +12932,8 @@ class Sema final {
 
   bool IsStringLiteralToNonConstPointerConversion(Expr *From, QualType ToType);
 
+  bool IsPointerToPointer(QualType LHSType, QualType RHSType);
+
   bool CheckExceptionSpecCompatibility(Expr *From, QualType ToType);
 
   ExprResult PerformImplicitConversion(Expr *From, QualType ToType,
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 069571fcf7864..e149f745cc2f9 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3782,6 +3782,13 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   }
 
   D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD));
+
+  if (S.IsPointerToPointer(ParamTy, Ty)) {
+    VarDecl *VD = cast<VarDecl>(D);
+    S.Diag(Loc, diag::warn_free_called_on_unallocated_object)
+        << NI.getName() << VD;
+    return;
+  }
 }
 
 static void handleEnumExtensibilityAttr(Sema &S, Decl *D,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 2f1ddfb215116..255e0be3cc842 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -10098,6 +10098,23 @@ static bool isVector(QualType QT, QualType ElementType) {
   return false;
 }
 
+bool Sema::IsPointerToPointer(QualType LHSType, QualType RHSType) {
+  if (const PointerType *LHSPointer = dyn_cast<PointerType>(LHSType)) {
+    // Check if LHS is a single pointer, not a pointer to a pointer.
+    if (!isa<PointerType>(LHSPointer->getPointeeType())) {
+      if (isa<PointerType>(RHSType)) {
+        if (const PointerType *RHSPtr = dyn_cast<PointerType>(RHSType)) {
+          // If RHSType is a pointer to a pointer type, return True
+          if (isa<PointerType>(RHSPtr->getPointeeType())) {
+            return true;
+          }
+        }
+      }
+    }
+  }
+  return false;
+}
+
 /// CheckAssignmentConstraints (C99 6.5.16) - This routine currently
 /// has code to accommodate several GCC extensions when type checking
 /// pointers. Here are some objectionable examples that GCC considers warnings:

>From 730f4d7f088645f4b49649c73328ca25e681339a Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Wed, 31 Jan 2024 05:45:00 +0530
Subject: [PATCH 2/8] remove whitespace

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 clang/include/clang/Basic/DiagnosticSemaKinds.td | 1 -
 1 file changed, 1 deletion(-)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 99ef803b1e0ec..ae9ad757788e8 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8268,7 +8268,6 @@ def warn_free_nonheap_object
 def warn_free_called_on_unallocated_object : Warning<
   "'%0' called on unallocated object '%1'">,
   InGroup<FreeNonHeapObject>;
-  
 
 // Completely identical except off by default.
 def warn_condition_is_idiomatic_assignment : Warning<"using the result "

>From 1a3cc18276be89969d5a262ff12499d20fd925ee Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Wed, 31 Jan 2024 20:58:19 +0530
Subject: [PATCH 3/8] change diagnostic

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 clang/include/clang/Basic/DiagnosticSemaKinds.td | 4 ++--
 clang/lib/Sema/SemaDeclAttr.cpp                  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index ae9ad757788e8..e8165394701c9 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8265,8 +8265,8 @@ def warn_condition_is_assignment : Warning<"using the result of an "
 def warn_free_nonheap_object
   : Warning<"attempt to call %0 on non-heap %select{object %2|object: block expression|object: lambda-to-function-pointer conversion}1">,
     InGroup<FreeNonHeapObject>;
-def warn_free_called_on_unallocated_object : Warning<
-  "'%0' called on unallocated object '%1'">,
+def warn_called_on_unallocated_object : Warning<
+  "calling function '%0' on an unallocated object '%1'">,
   InGroup<FreeNonHeapObject>;
 
 // Completely identical except off by default.
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index e149f745cc2f9..02d20a25eadf8 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3785,7 +3785,7 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 
   if (S.IsPointerToPointer(ParamTy, Ty)) {
     VarDecl *VD = cast<VarDecl>(D);
-    S.Diag(Loc, diag::warn_free_called_on_unallocated_object)
+    S.Diag(Loc, diag::warn_called_on_unallocated_object)
         << NI.getName() << VD;
     return;
   }

>From bd676b70e041523b3adc99208265666ffc52bf09 Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Thu, 1 Feb 2024 16:05:13 +0530
Subject: [PATCH 4/8] format

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 clang/lib/Sema/SemaDeclAttr.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 02d20a25eadf8..bd7074b280aa0 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3785,8 +3785,7 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
 
   if (S.IsPointerToPointer(ParamTy, Ty)) {
     VarDecl *VD = cast<VarDecl>(D);
-    S.Diag(Loc, diag::warn_called_on_unallocated_object)
-        << NI.getName() << VD;
+    S.Diag(Loc, diag::warn_called_on_unallocated_object) << NI.getName() << VD;
     return;
   }
 }

>From f7279ba9dd52e0685106d0b53552a9ea223a0582 Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Fri, 2 Feb 2024 17:30:38 +0530
Subject: [PATCH 5/8] call CheckFunctionCall to detect misuse

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 clang/docs/ReleaseNotes.rst                   |  3 ++
 .../clang/Basic/DiagnosticSemaKinds.td        |  3 --
 clang/include/clang/Sema/Sema.h               |  4 +--
 clang/lib/Sema/SemaDeclAttr.cpp               | 28 +++++++++++++++----
 clang/lib/Sema/SemaExpr.cpp                   | 17 -----------
 5 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 254e0a9cb7297..116b2b9829fdb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -106,6 +106,9 @@ Improvements to Clang's diagnostics
 - Clang now applies syntax highlighting to the code snippets it
   prints.
 
+- Clang now provides improved warnings for the cleanup attribute to detect misuse scenarios,
+  such as attempting to call `free` on unallocated objects.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index e8165394701c9..24d32cb87c89e 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8265,9 +8265,6 @@ def warn_condition_is_assignment : Warning<"using the result of an "
 def warn_free_nonheap_object
   : Warning<"attempt to call %0 on non-heap %select{object %2|object: block expression|object: lambda-to-function-pointer conversion}1">,
     InGroup<FreeNonHeapObject>;
-def warn_called_on_unallocated_object : Warning<
-  "calling function '%0' on an unallocated object '%1'">,
-  InGroup<FreeNonHeapObject>;
 
 // Completely identical except off by default.
 def warn_condition_is_idiomatic_assignment : Warning<"using the result "
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 535c479aeb7c5..f84804abd0252 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13873,8 +13873,6 @@ class Sema final {
                         bool AllowOnePastEnd = true, bool IndexNegated = false);
   void CheckArrayAccess(const Expr *E);
 
-  bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,
-                         const FunctionProtoType *Proto);
   bool CheckObjCMethodCall(ObjCMethodDecl *Method, SourceLocation loc,
                            ArrayRef<const Expr *> Args);
   bool CheckPointerCall(NamedDecl *NDecl, CallExpr *TheCall,
@@ -13973,6 +13971,8 @@ class Sema final {
   ExprResult SemaConvertVectorExpr(Expr *E, TypeSourceInfo *TInfo,
                                    SourceLocation BuiltinLoc,
                                    SourceLocation RParenLoc);
+  bool CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall,
+                         const FunctionProtoType *Proto);
 
 private:
   bool SemaBuiltinPrefetch(CallExpr *TheCall);
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index bd7074b280aa0..fda5d67205d17 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3782,12 +3782,28 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   }
 
   D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD));
-
-  if (S.IsPointerToPointer(ParamTy, Ty)) {
-    VarDecl *VD = cast<VarDecl>(D);
-    S.Diag(Loc, diag::warn_called_on_unallocated_object) << NI.getName() << VD;
-    return;
-  }
+  VarDecl *VD = cast<VarDecl>(D);
+  // Create a reference to the variable declaration. This is a fake/dummy
+  // reference.
+  DeclRefExpr *VariableReference = DeclRefExpr::Create(
+      S.Context, NestedNameSpecifierLoc{}, SourceLocation{}, VD, false,
+      DeclarationNameInfo{VD->getDeclName(), VD->getLocation()}, VD->getType(),
+      VK_LValue);
+
+  // Create a unary operator expression that represents taking the address of
+  // the variable. This is a fake/dummy expression.
+  Expr *AddressOfVariable = UnaryOperator::Create(
+      S.Context, VariableReference, UnaryOperatorKind::UO_AddrOf,
+      S.Context.getPointerType(VD->getType()), VK_PRValue, OK_Ordinary,
+      SourceLocation{}, false, FPOptionsOverride{});
+
+  // Create a function call expression. This is a fake/dummy call expression.
+  CallExpr *FunctionCallExpression = CallExpr::Create(
+      S.Context, E, ArrayRef{AddressOfVariable}, S.Context.VoidTy, VK_PRValue,
+      SourceLocation{}, FPOptionsOverride{});
+
+  S.CheckFunctionCall(FD, FunctionCallExpression,
+                      FD->getType()->getAs<FunctionProtoType>());
 }
 
 static void handleEnumExtensibilityAttr(Sema &S, Decl *D,
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 255e0be3cc842..2f1ddfb215116 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -10098,23 +10098,6 @@ static bool isVector(QualType QT, QualType ElementType) {
   return false;
 }
 
-bool Sema::IsPointerToPointer(QualType LHSType, QualType RHSType) {
-  if (const PointerType *LHSPointer = dyn_cast<PointerType>(LHSType)) {
-    // Check if LHS is a single pointer, not a pointer to a pointer.
-    if (!isa<PointerType>(LHSPointer->getPointeeType())) {
-      if (isa<PointerType>(RHSType)) {
-        if (const PointerType *RHSPtr = dyn_cast<PointerType>(RHSType)) {
-          // If RHSType is a pointer to a pointer type, return True
-          if (isa<PointerType>(RHSPtr->getPointeeType())) {
-            return true;
-          }
-        }
-      }
-    }
-  }
-  return false;
-}
-
 /// CheckAssignmentConstraints (C99 6.5.16) - This routine currently
 /// has code to accommodate several GCC extensions when type checking
 /// pointers. Here are some objectionable examples that GCC considers warnings:

>From ef3353f75eaa93dfa10bb12c745ec7496b344a79 Mon Sep 17 00:00:00 2001
From: Bhuminjay Soni <Soni5Happy at gmail.com>
Date: Tue, 6 Feb 2024 19:05:12 +0530
Subject: [PATCH 6/8] Update clang/docs/ReleaseNotes.rst

Co-authored-by: Aaron Ballman <aaron at aaronballman.com>
---
 clang/docs/ReleaseNotes.rst | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 116b2b9829fdb..a57600c64012d 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -106,8 +106,9 @@ Improvements to Clang's diagnostics
 - Clang now applies syntax highlighting to the code snippets it
   prints.
 
-- Clang now provides improved warnings for the cleanup attribute to detect misuse scenarios,
-  such as attempting to call `free` on unallocated objects.
+- Clang now provides improved warnings for the ``cleanup`` attribute to detect misuse scenarios,
+  such as attempting to call ``free`` on an unallocated object. Fixes
+  `#79443 <https://github.com/llvm/llvm-project/issues/79443>`_.
 
 Improvements to Clang's time-trace
 ----------------------------------

>From 5ae7d97e06c4c94c5dfe81a2fda77ef0a461a85f Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Wed, 7 Feb 2024 13:27:32 +0530
Subject: [PATCH 7/8] add test

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 clang/lib/Sema/SemaDeclAttr.cpp | 17 ++++++++++-------
 clang/test/Sema/attr-cleanup.c  | 10 ++++++++--
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index fda5d67205d17..0b1922e9b534b 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3780,13 +3780,11 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
       << NI.getName() << ParamTy << Ty;
     return;
   }
-
-  D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD));
   VarDecl *VD = cast<VarDecl>(D);
   // Create a reference to the variable declaration. This is a fake/dummy
   // reference.
   DeclRefExpr *VariableReference = DeclRefExpr::Create(
-      S.Context, NestedNameSpecifierLoc{}, SourceLocation{}, VD, false,
+      S.Context, NestedNameSpecifierLoc{}, SourceLocation{Loc}, VD, false,
       DeclarationNameInfo{VD->getDeclName(), VD->getLocation()}, VD->getType(),
       VK_LValue);
 
@@ -3795,15 +3793,20 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   Expr *AddressOfVariable = UnaryOperator::Create(
       S.Context, VariableReference, UnaryOperatorKind::UO_AddrOf,
       S.Context.getPointerType(VD->getType()), VK_PRValue, OK_Ordinary,
-      SourceLocation{}, false, FPOptionsOverride{});
+      SourceLocation{Loc}, false, FPOptionsOverride{});
 
   // Create a function call expression. This is a fake/dummy call expression.
   CallExpr *FunctionCallExpression = CallExpr::Create(
       S.Context, E, ArrayRef{AddressOfVariable}, S.Context.VoidTy, VK_PRValue,
-      SourceLocation{}, FPOptionsOverride{});
+      SourceLocation{Loc}, FPOptionsOverride{});
+
+  if(S.CheckFunctionCall(FD, FunctionCallExpression,
+                      FD->getType()->getAs<FunctionProtoType>())){
+                        return;
+                      }
+
+  D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD));
 
-  S.CheckFunctionCall(FD, FunctionCallExpression,
-                      FD->getType()->getAs<FunctionProtoType>());
 }
 
 static void handleEnumExtensibilityAttr(Sema &S, Decl *D,
diff --git a/clang/test/Sema/attr-cleanup.c b/clang/test/Sema/attr-cleanup.c
index 2c38687622c2b..d86c56df45dac 100644
--- a/clang/test/Sema/attr-cleanup.c
+++ b/clang/test/Sema/attr-cleanup.c
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 %s -verify -fsyntax-only
+// RUN: %clang_cc1 -Wfree-nonheap-object -fsyntax-only -verify %s
 
 void c1(int *a);
-
+typedef unsigned long size_t;
 extern int g1 __attribute((cleanup(c1))); // expected-warning {{'cleanup' attribute only applies to local variables}}
 int g2 __attribute((cleanup(c1))); // expected-warning {{'cleanup' attribute only applies to local variables}}
 static int g3 __attribute((cleanup(c1))); // expected-warning {{'cleanup' attribute only applies to local variables}}
@@ -48,3 +48,9 @@ void t6(void) {
 }
 
 void t7(__attribute__((cleanup(c4))) int a) {} // expected-warning {{'cleanup' attribute only applies to local variables}}
+
+extern void free(void *);
+extern void *malloc(size_t size);
+void t8(void) {
+  void *p __attribute__((cleanup(free))) = malloc(10); // expected-warning{{attempt to call free on non-heap object 'p'}}
+}

>From 4e987665a885d28493a31ebe8faaed197bb6a3f6 Mon Sep 17 00:00:00 2001
From: 11happy <soni5happy at gmail.com>
Date: Wed, 7 Feb 2024 13:27:59 +0530
Subject: [PATCH 8/8] format code

Signed-off-by: 11happy <soni5happy at gmail.com>
---
 clang/lib/Sema/SemaDeclAttr.cpp | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 0b1922e9b534b..405e5990a0e35 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3800,13 +3800,12 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
       S.Context, E, ArrayRef{AddressOfVariable}, S.Context.VoidTy, VK_PRValue,
       SourceLocation{Loc}, FPOptionsOverride{});
 
-  if(S.CheckFunctionCall(FD, FunctionCallExpression,
-                      FD->getType()->getAs<FunctionProtoType>())){
-                        return;
-                      }
+  if (S.CheckFunctionCall(FD, FunctionCallExpression,
+                          FD->getType()->getAs<FunctionProtoType>())) {
+    return;
+  }
 
   D->addAttr(::new (S.Context) CleanupAttr(S.Context, AL, FD));
-
 }
 
 static void handleEnumExtensibilityAttr(Sema &S, Decl *D,



More information about the cfe-commits mailing list