[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