[clang] [clang] Prioritze decl comments from macro expansion site (PR #65481)
danakj wrote: oh hey, just stumbled on this, thanks for improving it! https://github.com/llvm/llvm-project/pull/65481 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][Sema] Track trivial-relocatability as a type trait (PR #84621)
danakj wrote: The subspace library also provides mechanisms [for querying](https://suslib.cc/sus-mem-TriviallyRelocatable.html) and marking types trivially relocatable, and relies on the compiler's `__is_trivially_relocatable` as a signal as well. It then optimizes containers and operations internally based on its answer, and allows client code to do the same. Getting `__is_trivially_relocatable` correct here would be very appreciated. https://github.com/llvm/llvm-project/pull/84621 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Make -frewrite-includes put an endif at the end of the included text (PR #67613)
danakj wrote: I was just lamenting the lack of this feature this week. Thank you, I can’t wait to use it. https://github.com/llvm/llvm-project/pull/67613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Comments] Attach comments to decl even if preproc directives are in between (PR #88367)
danakj wrote: `#include` and `#define` at a minimum should break the connection. Comments above a `#define` can be attributed to the macro defined there, which is something that Subdoc does (example result: https://suslib.cc/macro.sus_check.html) https://github.com/llvm/llvm-project/pull/88367 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
danakj wrote: bump for review? https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
Thanks! I will have a look at that on Monday then.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
Hm, I changed `MatchDescendantVisitor::shouldVisitImplicitCode()` to return
true, but that is not sufficient to resolve things for me. Did you try running
the tests in this PR with that change? If I
remove`CallableVisitor::VisitFieldDecl()`, I get 5 errors for missing
diagnostics still for all the field initializer test cases in
`clang/test/SemaCXX/warn-unsafe-buffer-usage-function-attr.cpp`
I will try out `TraverseCXXDefaultInitExpr()` and see what I can see.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
Well, regardless of the value of
`MatchDescendantVisitor::shouldVisitImplicitCode()`, there is no call to a
`MatchDescendantVisitorTraverseCXXDefaultInitExpr()` occurring in this test:
```cc
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
// RUN:-fsafe-buffer-usage-suggestions -verify %s
class UnsafeMembers {
public:
[[clang::unsafe_buffer_usage]]
UnsafeMembers(int) {}
};
struct HoldsUnsafeMembers {
UnsafeMembers FromField{3}; // expected-warning{{function introduces
unsafe buffer manipulation}}
};
```
I will keep trying because you saw something that I am missing, but noting my
progress here in case it's useful.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj edited https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
I see. This test has `TraverseCXXDefaultInitExpr` occur:
```cc
// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage \
// RUN:-fsafe-buffer-usage-suggestions -verify %s
class UnsafeMembers {
public:
[[clang::unsafe_buffer_usage]]
UnsafeMembers(int) {}
};
struct HoldsUnsafeMembers {
HoldsUnsafeMembers() {}
UnsafeMembers FromField{3}; // expected-warning{{function introduces
unsafe buffer manipulation}}
};
```
But if the explicit ctor is omitted, then there is no such
`TraverseCXXDefaultInitExpr` and the FieldDecl must be visited, unless there's
something else to anchor it on.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/3] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation an
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
This is the AST in the case without the explicit constructor:
```
|-CXXRecordDecl col:8 implicit struct HoldsUnsafeMembers
`-FieldDecl col:19 FromField 'UnsafeMembers'
`-CXXConstructExpr 'UnsafeMembers' 'void (int)' list
`-IntegerLiteral 'int' 3
```
https://godbolt.org/z/5freKecWb
AFAICT we are required to visit FieldDecl to handle the 0-ctor class use case.
Thinking through design, maybe we could visit RecordDecl instead, in
CallableVisitor, and then provide a TraverseRecordDecl in the
MatchDescendantVisitor that would traverse each field initializer? I could try
it, I am not sure it would work, but it's not obviously more clear/better to me
either.
I added a comment on the aggregate test case mentioning the lack of
CXXDefaultInitExpr that it is testing for.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
> In your new test the entire constructor is missing. The class is an
> aggregate, it has no constructor: https://godbolt.org/z/aa869TKsf - so I hope
> you'll see the warning whenever you aggregate-initialize an instance of such
> class. Which may or may not mean that there's still no direct need to warn at
> the definition of the field.
https://godbolt.org/z/qjqaW3Eze here is an example of the 3 ways to call the
aggregate ctor.
```
|-DeclStmt
| `-VarDecl col:24 m1 'HoldsUnsafeMembers' callinit
| `-CXXConstructExpr 'HoldsUnsafeMembers' 'void ()
noexcept(false)'
|-DeclStmt
| `-VarDecl col:10 m2 'HoldsUnsafeMembers' cinit
| `-CXXTemporaryObjectExpr 'HoldsUnsafeMembers' 'void ()
noexcept(false)' zeroing
`-DeclStmt
`-VarDecl col:10 m3 'HoldsUnsafeMembers' cinit
`-CXXFunctionalCastExpr 'HoldsUnsafeMembers'
functional cast to HoldsUnsafeMembers
`-InitListExpr 'HoldsUnsafeMembers'
`-CXXDefaultInitExpr 'UnsafeMembers' has rewritten init
`-CXXConstructExpr 'UnsafeMembers' 'void
(int)' list
`-IntegerLiteral 'int' 3
```
We do get a CXXDefaultInitExpr if we construct with `{}` but not otherwise.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
> There's no constructor. There's no binary code for constructor. The unsafe
> function is not invoked from any binary code generated from your example.
Ah yes, this helped me figure out what's going on with the 3 different
initialization paths.
If you construct the aggregate like `A a;` or `auto a = A();` then we do see a
call to a constructor. So where is it? Now it appears in the AST for the class:
```
| |-CXXRecordDecl col:8 implicit struct HoldsUnsafeMembers
| |-FieldDecl col:19 FromField 'UnsafeMembers'
| | `-CXXConstructExpr 'UnsafeMembers' 'void (int)' list
| | `-IntegerLiteral 'int' 3
| |-CXXConstructorDecl col:8 implicit used constexpr
HoldsUnsafeMembers 'void () noexcept(false)' inline default
| | |-CXXCtorInitializer Field 0xbc32670 'FromField' 'UnsafeMembers'
| | | `-CXXDefaultInitExpr 'UnsafeMembers' has rewritten init
| | | `-CXXConstructExpr 'UnsafeMembers' 'void (int)' list
| | | `-IntegerLiteral 'int' 3
```
If you construct it as `auto a = A{};` then yes there is no ctor as you say,
but it does appear at the call site. So okay I agree there's nothing to do for
an aggregate that is never used. And maybe I can get TraverseCXXDefaultInitExpr
to work with that information.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/4] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation an
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/4] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation an
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
+// Visit in-class initializers for fields.
+if (!FD->hasInClassInitializer())
+ return;
+
+Stmts.push_back(FD->getInClassInitializer());
+ }
+
+ if (isa(D) || isa(D)) {
+Stmts.push_back(D->getBody());
+ }
+
+ assert(!Stmts.empty());
+
+ for (Stmt *S : Stmts) {
+auto [FixableGadgets, WarningGadgets, Tracker] =
+findGadgets(S, D->getASTContext(), Handler, EmitSuggestions);
+applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
danakj wrote:
If I understand correctly, we'd still collect a list of gadgets+trackers in the
same way, but we'd pass them all at once to applyGadgets() so it could do some
kind of global analysis over the full set if it wanted to. Though it will just
need to iterate over them for now?
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
+// Visit in-class initializers for fields.
+if (!FD->hasInClassInitializer())
+ return;
+
+Stmts.push_back(FD->getInClassInitializer());
+ }
+
+ if (isa(D) || isa(D)) {
+Stmts.push_back(D->getBody());
+ }
+
+ assert(!Stmts.empty());
+
+ for (Stmt *S : Stmts) {
+auto [FixableGadgets, WarningGadgets, Tracker] =
+findGadgets(S, D->getASTContext(), Handler, EmitSuggestions);
+applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets),
danakj wrote:
Or, I guess we'd have lists of gadget, but the difference here is we'd have
only a single tracker? So applyGadgets actually just walks the global list of
gadgets (it already does this for a list of gadgets) and uses the one
tracker... I will try this out.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
Unfortunately, CXXCtorInitializers do not catch everything. This test fails:
```cc
struct S {
S()
: FromCtor(3), // expected-warning{{function introduces unsafe buffer
manipulation}}
FromCtor2{3} // expected-warning{{function introduces unsafe buffer
manipulation}}
{}
UnsafeMembers FromCtor;
UnsafeMembers FromCtor2;
UnsafeMembers FromField{3}; // expected-warning{{function introduces
unsafe buffer manipulation}}
};
```
Where the int ctor of UnsafeMembers is `[[clang::unsafe_buffer_usage]]`.
This also fails:
```
struct AggregateUnsafeMembers {
UnsafeMembers f1;
UnsafeMembers f2{3}; // expected-warning{{function introduces unsafe
buffer manipulation}}
};
```
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj edited https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj edited https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj edited https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/2] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation an
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
danakj wrote: I have rebased (which dropped the previous PR's commits) and applied the requested changes in a second commit. I also fixed the test failures in that second commit. https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/2] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation an
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 273f93fa4aecc0017b5c7172bc296ced1bc6d5f5 Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/2] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index 866222380974b..dbed6092cd5a0 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1387,8 +1387,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -1483,7 +1483,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3030,39 +3030,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation an
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj created https://github.com/llvm/llvm-project/pull/91777 The -Wunsafe-buffer-usage warning should fire on any call to a function annotated with [[clang::unsafe_buffer_usage]], however it omitted calls to constructors, since the expression is a CXXConstructExpr which does not subclass CallExpr. Thus the matcher on callExpr() does not find these expressions. Add a new WarningGadget that matches cxxConstructExpr that are calling a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching against the std::span(ptr, size) constructor because that is handled by SpanTwoParamConstructorGadget and we never want two gadgets to match the same thing (and this is guarded by asserts). The gadgets themselves do not report the warnings, instead each gadget's Stmt is passed to the UnsafeBufferUsageHandler (implemented by UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a CXXConstructExpr statement must be a match for std::span(ptr, size), but that is no longer the case. We want the Reporter to generate different warnings (in the -Wunsafe-buffer-usage-in-container subgroup) for the span contructor. And we will want it to report more warnings for other std-container-specific gadgets in the future. To handle this we allow the gadget to control if the warning is general (it calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it calls handleUnsafeOperationInContainer()). Then the WarningGadget grows a virtual method to dispatch to the appropriate path in the UnsafeBufferUsageHandler. By doing so, we no longer need getBaseStmt in the Gadget interface. The only use of it for FixableGadgets was to get the SourceLocation, so we make an explicit virtual method for that on Gadget. Then the handleUnsafeOperation() dispatcher can be a virtual method that is only in WarningGadget. The SpanTwoParamConstructorGadget gadget dispatches to handleUnsafeOperationInContainer() while the other WarningGadgets all dispatch to the original handleUnsafeBufferUsage(). Tests are added for annotated constructors, conversion operattors, call operators, fold expressions, and regular methods. >From d7694d70393fd49792470147c7ca136fd33f194e Mon Sep 17 00:00:00 2001 From: danakj Date: Fri, 10 May 2024 13:31:17 -0400 Subject: [PATCH] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors The -Wunsafe-buffer-usage warning should fire on any call to a function annotated with [[clang::unsafe_buffer_usage]], however it omitted calls to constructors, since the expression is a CXXConstructExpr which does not subclass CallExpr. Thus the matcher on callExpr() does not find these expressions. Add a new WarningGadget that matches cxxConstructExpr that are calling a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching against the std::span(ptr, size) constructor because that is handled by SpanTwoParamConstructorGadget and we never want two gadgets to match the same thing (and this is guarded by asserts). The gadgets themselves do not report the warnings, instead each gadget's Stmt is passed to the UnsafeBufferUsageHandler (implemented by UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a CXXConstructExpr statement must be a match for std::span(ptr, size), but that is no longer the case. We want the Reporter to generate different warnings (in the -Wunsafe-buffer-usage-in-container subgroup) for the span contructor. And we will want it to report more warnings for other std-container-specific gadgets in the future. To handle this we allow the gadget to control if the warning is general (it calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it calls handleUnsafeOperationInContainer()). Then the WarningGadget grows a virtual method to dispatch to the appropriate path in the UnsafeBufferUsageHandler. By doing so, we no longer need getBaseStmt in the Gadget interface. The only use of it for FixableGadgets was to get the SourceLocation, so we make an explicit virtual method for that on Gadget. Then the handleUnsafeOperation() dispatcher can be a virtual method that is only in WarningGadget. The SpanTwoParamConstructorGadget gadget dispatches to handleUnsafeOperationInContainer() while the other WarningGadgets all dispatch to the original handleUnsafeBufferUsage(). Tests are added for annotated constructors, conversion operattors, call operators, fold expressions, and regular methods. --- .../Analysis/Analyses/UnsafeBufferUsage.h | 5 + .../Analyses/UnsafeBufferUsageGadgets.def | 1 + clang/lib/Analysis/UnsafeBufferUsage.cpp | 166 -- clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++- ...warn-unsafe-buffer-usage-function-attr.cpp | 38 5 files changed, 175 insertions(
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
danakj wrote: cc: @haoNoQ https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91777
>From 1c835f4cc77823360762b8e53e2bbfe027c977cf Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 169 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 178 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..e67bdcc15b72c 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation getS
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91777
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation getS
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91777
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/2] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj created
https://github.com/llvm/llvm-project/pull/91991
This is built on top of https://github.com/llvm/llvm-project/pull/91777, and
includes the commits there for now.
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
v
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj edited https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj edited https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
danakj wrote: > Hi! Thank you for digging into this! Sorry for the delay. > > > The new UnsafeBufferUsageCtorAttrGadget gadget explicitly avoids matching > > against the std::span(ptr, size) constructor because that is handled by > > SpanTwoParamConstructorGadget and we never want two gadgets to match the > > same thing (and this is guarded by asserts). > > Hmm at a glance I'm not sure this should really be illegal. It's a big > problem when fixable gadgets overlap, because this means that they'll try to > fix the same code in two incompatible ways. I don't immediately see why this > would be a problem for warning gadgets that don't try to fix anything. This > just means that the code is non-compliant for two different reasons, which is > theoretically fine. I also tried to reproduce your assert and I didn't hit > it; which one are you running into? https://github.com/llvm/llvm-project/blob/2ff43ce87e66d9324370e35ea6743ef57400c76e/clang/lib/Analysis/UnsafeBufferUsage.cpp#L1373-L1374 These assert that exactly one gadget matched. I think it's kinda worthwhile keeping the warnings independent too, so I don't see this as a bad thing. If we were to mark the span ctor as `[[clang::unsafe_buffer_usage]]` for instance, it the span-specific warning gives a better output still, and generating two warnings for the same piece of code is noisy. > > To handle this we allow the gadget to control if the warning is general (it > > calls handleUnsafeBufferUsage()) or is a std-container-specific warning (it > > calls handleUnsafeOperationInContainer()). > > Ooo I love this. > > My initial thinking was, just make the base Gadget class "public" (move it > into `UnsafeBufferUsage.h` so that the handler could see it) and make the > handler switch over the gadget's `getKind()` to produce specialized messages > for each gadget. This would help us unscrew the rest of the `Reporter` code > so that it also didn't have to guess by statement kind. > > Your design can grow into that too - make a separate handler method for each > gadget kind (or group of kinds), and it preserves even more encapsulation. > Additionally it throws away `getBaseStmt()` in favor of a more "integrated" > solution. I'm a big fan, let's keep this. :+1: https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
danakj wrote: These lit test failures are indeed from this PR's last commit, I will dig into why and correct what's wrong. https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91777
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/3] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
danakj wrote: > > https://github.com/llvm/llvm-project/blob/2ff43ce87e66d9324370e35ea6743ef57400c76e/clang/lib/Analysis/UnsafeBufferUsage.cpp#L1373-L1374 > > > > These assert that exactly one gadget matched. I think it's kinda worthwhile > > keeping the warnings independent too, so I don't see this as a bad thing. > > If we were to mark the span ctor as `[[clang::unsafe_buffer_usage]]` for > > instance, it the span-specific warning gives a better output still, and > > generating two warnings for the same piece of code is noisy. > > Hmm this isn't really what these assertions are about. They don't protect > against duplicate gadgets, they protect against duplicate `.bind()` tags > across gadgets. Because gadgets are connected by the `anyOf()` matcher (as > opposed to `eachOf()`), a single match result will always have exactly one > gadget, the first one in the list, even if they match the same statement. > > So I think you're right that there's a problem: if you have two gadgets match > the same statement, only one will be reported. So your solution is probably > necessary. We should also maybe do something about that, so that warning > gadgets weren't conflicting this way. But I don't think _this_ assertion > guards against that. Ah yeah, okay, that makes sense. I did bump into the asserts while working on this, but not in the way that I thought. When I remove the `unless(HasTwoParamSpanCtorDecl)` I don't hit the asserts. > If this still doesn't sound right to you, can you include a test where the > assertion fails for you? No matter how ridiculous the test is. The machine is > complex enough to appreciate ridiculous tests. I think we want to get to the > bottom of this. It sounds right to me yeah, especially after testing it. https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
@@ -2295,6 +2292,23 @@ class UnsafeBufferUsageReporter : public
UnsafeBufferUsageHandler {
}
}
+ void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) override {
+SourceLocation Loc;
+SourceRange Range;
+unsigned MsgParam = 0;
+if (const auto *CtorExpr = dyn_cast(Operation)) {
danakj wrote:
Got it. The old code was dyn but that was because it handled other cases too.
Once this function handles other gadgets we'll adjust it.
https://github.com/llvm/llvm-project/pull/91777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
@@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, } #ifndef NDEBUG Handler.addDebugNoteForVar( - VD, F->getBaseStmt()->getBeginLoc(), + VD, F->getSourceLoc(), danakj wrote: Yeah, this is the only callsite that needs the SourceLocation for a `FixableGadget`. https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
@@ -921,10 +937,55 @@ class UnsafeBufferUsageAttrGadget : public WarningGadget {
}
static Matcher matcher() {
-return
stmt(callExpr(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage
-.bind(OpTag));
+auto HasUnsafeFnDecl =
+callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)));
+return stmt(callExpr(HasUnsafeFnDecl).bind(OpTag));
+ }
+
+ void handleUnsafeOperation(UnsafeBufferUsageHandler &Handler,
+ bool IsRelatedToDecl,
+ ASTContext &Ctx) const override {
+Handler.handleUnsafeOperation(Op, IsRelatedToDecl, Ctx);
+ }
+ SourceLocation getSourceLoc() const override { return Op->getBeginLoc(); }
+
+ DeclUseList getClaimedVarUseSites() const override { return {}; }
+};
+
+/// A call of a constructor that performs unchecked buffer operations
+/// over one of its pointer parameters, or constructs a class object that will
+/// perform buffer operations that depend on the correctness of the parameters.
+class UnsafeBufferUsageCtorAttrGadget : public WarningGadget {
+ constexpr static const char *const OpTag = "cxx_construct_expr";
+ const CXXConstructExpr *Op;
+
+public:
+ UnsafeBufferUsageCtorAttrGadget(const MatchFinder::MatchResult &Result)
+ : WarningGadget(Kind::UnsafeBufferUsageCtorAttr),
+Op(Result.Nodes.getNodeAs(OpTag)) {}
+
+ static bool classof(const Gadget *G) {
+return G->getKind() == Kind::UnsafeBufferUsageCtorAttr;
+ }
+
+ static Matcher matcher() {
+auto HasUnsafeCtorDecl =
+hasDeclaration(cxxConstructorDecl(hasAttr(attr::UnsafeBufferUsage)));
+// std::span(ptr, size) ctor is handled by SpanTwoParamConstructorGadget.
+auto HasTwoParamSpanCtorDecl = hasDeclaration(
danakj wrote:
Oh, yeah good idea. Done.
https://github.com/llvm/llvm-project/pull/91777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP][Safe Buffers] Serialize unsafe_buffer_usage pragmas (PR #92031)
danakj wrote: I am curious so I will ask: why did this take an approach of serializing the output of the pragmas (the maps) instead of serializing the pragmas themselves as AST nodes, so that the maps would be constructed in the same way as from parsing the text directly? that's how other pragmas were serialized that I have seen. https://github.com/llvm/llvm-project/pull/92031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
@@ -2856,7 +2916,7 @@ getFixIts(FixableGadgetSets &FixablesForAllVars, const FixitStrategy &S, } #ifndef NDEBUG Handler.addDebugNoteForVar( - VD, F->getBaseStmt()->getBeginLoc(), + VD, F->getSourceLoc(), danakj wrote: I will leave this for a followup, yeah. Thanks https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
@@ -1315,9 +1374,9 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
virtual std::optional
getFixits(const FixitStrategy &s) const final;
-
- // TODO remove this method from FixableGadget interface
danakj wrote:
This one used to return null, but it does return something useful now. So
instead of putting this TODO back (and equally putting it on every
FixableGadget subclass), I can apply it up in the Gadget class.
https://github.com/llvm/llvm-project/pull/91777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91777
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/4] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91777
>From 8b318dadac6d0ec53b5d26461edfe19a391845ec Mon Sep 17 00:00:00 2001
From: danakj
Date: Fri, 10 May 2024 13:31:17 -0400
Subject: [PATCH 1/4] Respect the [[clang::unsafe_buffer_usage]] attribute for
constructors
The -Wunsafe-buffer-usage warning should fire on any call to a function
annotated with [[clang::unsafe_buffer_usage]], however it omitted calls
to constructors, since the expression is a CXXConstructExpr which does
not subclass CallExpr. Thus the matcher on callExpr() does not find
these expressions.
Add a new WarningGadget that matches cxxConstructExpr that are calling
a CXXConstructDecl annotated by [[clang::unsafe_buffer_usage]] and fires
the warning. The new UnsafeBufferUsageCtorAttrGadget gadget explicitly
avoids matching against the std::span(ptr, size) constructor because
that is handled by SpanTwoParamConstructorGadget and we never want two
gadgets to match the same thing (and this is guarded by asserts).
The gadgets themselves do not report the warnings, instead each gadget's
Stmt is passed to the UnsafeBufferUsageHandler (implemented by
UnsafeBufferUsageReporter). The Reporter is previously hardcoded that a
CXXConstructExpr statement must be a match for std::span(ptr, size),
but that is no longer the case. We want the Reporter to generate
different warnings (in the -Wunsafe-buffer-usage-in-container subgroup)
for the span contructor. And we will want it to report more warnings for
other std-container-specific gadgets in the future. To handle this we
allow the gadget to control if the warning is general
(it calls handleUnsafeBufferUsage()) or is a std-container-specific
warning (it calls handleUnsafeOperationInContainer()).
Then the WarningGadget grows a virtual method to dispatch to the
appropriate path in the UnsafeBufferUsageHandler. By doing so, we no
longer need getBaseStmt in the Gadget interface. The only use of it for
FixableGadgets was to get the SourceLocation, so we make an explicit
virtual method for that on Gadget. Then the handleUnsafeOperation()
dispatcher can be a virtual method that is only in WarningGadget.
The SpanTwoParamConstructorGadget gadget dispatches to
handleUnsafeOperationInContainer() while the other WarningGadgets all
dispatch to the original handleUnsafeBufferUsage().
Tests are added for annotated constructors, conversion operattors, call
operators, fold expressions, and regular methods.
---
.../Analysis/Analyses/UnsafeBufferUsage.h | 5 +
.../Analyses/UnsafeBufferUsageGadgets.def | 1 +
clang/lib/Analysis/UnsafeBufferUsage.cpp | 167 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 22 ++-
...warn-unsafe-buffer-usage-function-attr.cpp | 38
5 files changed, 176 insertions(+), 57 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
index 5d16dcc824c50..228b4ae1e3e11 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
@@ -106,6 +106,11 @@ class UnsafeBufferUsageHandler {
virtual void handleUnsafeOperation(const Stmt *Operation,
bool IsRelatedToDecl, ASTContext &Ctx) =
0;
+ /// Invoked when an unsafe operation with a std container is found.
+ virtual void handleUnsafeOperationInContainer(const Stmt *Operation,
+bool IsRelatedToDecl,
+ASTContext &Ctx) = 0;
+
/// Invoked when a fix is suggested against a variable. This function groups
/// all variables that must be fixed together (i.e their types must be
changed
/// to the same target type to prevent type mismatches) into a single fixit.
diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
index 3273c642eed51..242ad763ba62b 100644
--- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
+++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsageGadgets.def
@@ -36,6 +36,7 @@ WARNING_GADGET(Decrement)
WARNING_GADGET(ArraySubscript)
WARNING_GADGET(PointerArithmetic)
WARNING_GADGET(UnsafeBufferUsageAttr)
+WARNING_GADGET(UnsafeBufferUsageCtorAttr)
WARNING_GADGET(DataInvocation)
WARNING_CONTAINER_GADGET(SpanTwoParamConstructor) // Uses of `std::span(arg0,
arg1)`
FIXABLE_GADGET(ULCArraySubscript) // `DRE[any]` in an Unspecified
Lvalue Context
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c42e70d5b95ac..b24588e722aaa 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -492,7 +492,7 @@ class Gadget {
#endif
virtual bool isWarningGadget() const = 0;
- virtual const Stmt *getBaseStmt() const = 0;
+ virtual SourceLocation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
@@ -1315,9 +1374,9 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
virtual std::optional
getFixits(const FixitStrategy &s) const final;
-
- // TODO remove this method from FixableGadget interface
danakj wrote:
Also it's slightly different (per your comment above), in that we can remove it
from the WarningGadget API, leaving it only on FixableGadget for debug prints.
I have updated the TODO to say so.
https://github.com/llvm/llvm-project/pull/91777
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Safe Buffers] Serialize unsafe_buffer_usage pragmas (PR #92031)
danakj wrote: Thanks for the explanation :) https://github.com/llvm/llvm-project/pull/92031 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for constructors (PR #91777)
https://github.com/danakj closed https://github.com/llvm/llvm-project/pull/91777 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] [NFC] Clarify assume diagnostic (PR #93077)
danakj wrote: Thanks, this is very good https://github.com/llvm/llvm-project/pull/93077 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
danakj wrote: Sorry I just haven't gotten back to making the fixes yet. If someone else wants to apply them and land it I won't be upset, otherwise I will get to this eventually.. https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 9c0a0ca3f0bcf4910592dca2b7b9d7d9b83f6cee Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/5] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index fad2f52e89ef14..a501bead74a9e6 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1973,8 +1973,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -2068,7 +2068,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3614,39 +3614,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 9c0a0ca3f0bcf4910592dca2b7b9d7d9b83f6cee Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/4] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index fad2f52e89ef14..a501bead74a9e6 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1973,8 +1973,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -2068,7 +2068,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3614,39 +3614,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3328,3 +3300,63 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ // We do not want to visit a Lambda expression defined inside a method
+ // independently. Instead, it should be visited along with the outer method.
+ // FIXME: do we want to do the same thing for `BlockDecl`s?
+ if (const auto *fd = dyn_cast(D)) {
+if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
+ return;
+ }
+
+ // Do not emit fixit suggestions for functions declared in an
+ // extern "C" block.
+ if (const auto *FD = dyn_cast(D)) {
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
+ for (const CXXCtorInitializer *CI : ID->inits()) {
+Stmts.push_back(CI->getInit());
+ }
+}
+ }
+
+ if (const auto *FD = dyn_cast(D)) {
danakj wrote:
Thanks, I have (finally) applied that delta, and got the same test results. I
also didn't know the `2` trick for duplicate warnings, that's helpful!
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
https://github.com/danakj updated
https://github.com/llvm/llvm-project/pull/91991
>From 9c0a0ca3f0bcf4910592dca2b7b9d7d9b83f6cee Mon Sep 17 00:00:00 2001
From: danakj
Date: Mon, 13 May 2024 12:11:41 -0400
Subject: [PATCH 1/5] Generate -Wunsafe-buffer-usage warnings in ctor and field
initializers
Field initializers must be found by visiting FieldDecl and then
running the warning checks/matchers against the in-class initializer,
which is itself a Stmt. This catches warnings in places such as:
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr};
};
CXXCtorInitializers are not statements either, but they point to an
initializer expression which is. When visiting a FunctionDecl, also walk
through any constructor initializers and run the warning checks/matchers
against their initializer expressions. This catches warnings for
initializing fields and calling other constructors, such as:
struct C {
C(P* Ptr) : AnUnsafeCtor(Ptr) {}
}
We add tests for explicit construction, for field initialization, base
class constructor calls, delegated constructor calls, and aggregate
initialization.
Note that aggregate initialization through `()` is treated differently
today by the AST matchers than `{}`. The former is not considered as
calling an implicit constructor, while the latter is.
MatchDescendantVisitor::shouldVisitImplicitCode() returns false with a
TODO, which means we do not catch warnings of the form:
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType{Ptr};
But we do already catch them when written as (in C++20):
struct AggregateInitType { AnUnsafeCtor U; }
AggregateInitType(Ptr);
Returning true from MatchDescendantVisitor::shouldVisitImplicitCode(),
however, breaks expectations for field in-class initializers by moving
the SourceLocation, possibly to inside the implicit ctor instead of on
the line where the field initialization happens.
struct C {
P* Ptr;
AnUnsafeCtor U{Ptr}; // expected-warning{{this is never seen then}}
};
Tests are also added for std::span(ptr, size) constructor being called
from a field initializer and a constructor initializer.
---
clang/lib/Analysis/UnsafeBufferUsage.cpp | 108 --
clang/lib/Sema/AnalysisBasedWarnings.cpp | 8 ++
...warn-unsafe-buffer-usage-function-attr.cpp | 67 +++
...ffer-usage-in-container-span-construct.cpp | 10 ++
4 files changed, 155 insertions(+), 38 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp
b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index fad2f52e89ef14..a501bead74a9e6 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1973,8 +1973,8 @@ class DerefSimplePtrArithFixableGadget : public
FixableGadget {
/// Scan the function and return a list of gadgets found with provided kits.
static std::tuple
-findGadgets(const Decl *D, const UnsafeBufferUsageHandler &Handler,
-bool EmitSuggestions) {
+findGadgets(const Stmt *S, ASTContext &Ctx,
+const UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
struct GadgetFinderCallback : MatchFinder::MatchCallback {
FixableGadgetList FixableGadgets;
@@ -2068,7 +2068,7 @@ findGadgets(const Decl *D, const UnsafeBufferUsageHandler
&Handler,
// clang-format on
}
- M.match(*D->getBody(), D->getASTContext());
+ M.match(*S, Ctx);
return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets),
std::move(CB.Tracker)};
}
@@ -3614,39 +3614,9 @@ class VariableGroupsManagerImpl : public
VariableGroupsManager {
}
};
-void clang::checkUnsafeBufferUsage(const Decl *D,
- UnsafeBufferUsageHandler &Handler,
- bool EmitSuggestions) {
-#ifndef NDEBUG
- Handler.clearDebugNotes();
-#endif
-
- assert(D && D->getBody());
- // We do not want to visit a Lambda expression defined inside a method
- // independently. Instead, it should be visited along with the outer method.
- // FIXME: do we want to do the same thing for `BlockDecl`s?
- if (const auto *fd = dyn_cast(D)) {
-if (fd->getParent()->isLambda() && fd->getParent()->isLocalClass())
- return;
- }
-
- // Do not emit fixit suggestions for functions declared in an
- // extern "C" block.
- if (const auto *FD = dyn_cast(D)) {
-for (FunctionDecl *FReDecl : FD->redecls()) {
- if (FReDecl->isExternC()) {
-EmitSuggestions = false;
-break;
- }
-}
- }
-
- WarningGadgetSets UnsafeOps;
- FixableGadgetSets FixablesForAllVars;
-
- auto [FixableGadgets, WarningGadgets, Tracker] =
- findGadgets(D, Handler, EmitSuggestions);
-
+void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets,
+ WarningGadgetList WarningGadgets, DeclUseTracker Tracker,
+ UnsafeBufferUsageHandler &Handler, bool EmitSuggestions) {
if (!EmitSuggestions) {
// Our job is very easy without suggestions. Just warn about
// every problematic operation
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
danakj wrote: The suggested fixes were applied, thanks for them. It was a bit of work to page all this back in after a while. https://github.com/llvm/llvm-project/pull/91991 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Don't redundantly specify the default template argument to `BumpPtrAllocatorImpl` (PR #114857)
https://github.com/danakj approved this pull request. https://github.com/llvm/llvm-project/pull/114857 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Respect the [[clang::unsafe_buffer_usage]] attribute for field and constructor initializers (PR #91991)
@@ -3912,3 +3896,56 @@ void clang::checkUnsafeBufferUsage(const Decl *D,
}
}
}
+
+void clang::checkUnsafeBufferUsage(const Decl *D,
+ UnsafeBufferUsageHandler &Handler,
+ bool EmitSuggestions) {
+#ifndef NDEBUG
+ Handler.clearDebugNotes();
+#endif
+
+ assert(D);
+
+ SmallVector Stmts;
+
+ if (const auto *FD = dyn_cast(D)) {
+// We do not want to visit a Lambda expression defined inside a method
+// independently. Instead, it should be visited along with the outer
method.
+// FIXME: do we want to do the same thing for `BlockDecl`s?
+if (const auto *MD = dyn_cast(D)) {
+ if (MD->getParent()->isLambda() && MD->getParent()->isLocalClass())
+return;
+}
+
+for (FunctionDecl *FReDecl : FD->redecls()) {
+ if (FReDecl->isExternC()) {
+// Do not emit fixit suggestions for functions declared in an
+// extern "C" block.
+EmitSuggestions = false;
+break;
+ }
+}
+
+Stmts.push_back(FD->getBody());
+
+if (const auto *ID = dyn_cast(D)) {
danakj wrote:
I don't think we have that option, because they are not statements:
https://github.com/llvm/llvm-project/blob/5545f76dc94e76ef6800823bdd1e107ad2264717/clang/include/clang/Basic/StmtNodes.td#L133-L134
That's why this code has to pull the `getInit()` out of it to find a statement.
https://github.com/llvm/llvm-project/pull/91991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Fix bug in unsafe casts to incomplete types (PR #116433)
@@ -2270,19 +2270,28 @@ class UnsafeBufferUsageReporter : public
UnsafeBufferUsageHandler {
MsgParam = 5;
} else if (const auto *ECE = dyn_cast(Operation)) {
QualType destType = ECE->getType();
+bool destTypeComplete = true;
+
if (!isa(destType))
return;
+destType = destType.getTypePtr()->getPointeeType();
+if (const auto *D = destType->getAsTagDecl())
+ destTypeComplete = D->isCompleteDefinition();
-const uint64_t dSize =
-Ctx.getTypeSize(destType.getTypePtr()->getPointeeType());
+// If destination type is incomplete, it is unsafe to cast to anyway,
no
+// need to check its' type:
danakj wrote:
```suggestion
// need to check its type:
```
https://github.com/llvm/llvm-project/pull/116433
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [-Wunsafe-buffer-usage] Fix bug in unsafe casts to incomplete types (PR #116433)
https://github.com/danakj approved this pull request. https://github.com/llvm/llvm-project/pull/116433 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
