[clang] [analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. (PR #79398)
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 30 17:59:50 PST 2024
https://github.com/haoNoQ updated https://github.com/llvm/llvm-project/pull/79398
>From 21923f92f0ea4169d5fea646221554d4c44e36f1 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <adergachev at apple.com>
Date: Wed, 24 Jan 2024 14:52:58 -0800
Subject: [PATCH 1/5] [analyzer] Unbreak [[clang::suppress]] on checkers
without decl-with-issue.
There are currently a few checkers that don't fill in the bug report's
"decl with issue" field (typically a function in which the bug is found).
The new attribute [[clang::suppress]] uses decl-with-issue to reduce
the size of the suppression source range map so that it didn't need
to do that for the entire translation unit.
I'm already seeing a few problems with this approach so I'll probably
redesign it in some point as it looks like a premature optimization.
Not only checkers shouldn't be required to pass decl-with-issue
(consider clang-tidy checkers that never had such notion),
but also it's not necessarily uniquely determined (consider
leak suppressions at allocation site).
For now I'm adding a simple stop-gap solution that falls back to
building the suppression map for the entire TU whenever decl-with-issue
isn't specified. Which typically won't happen by
default because luckily all default checkers do provide decl-with-issue.
---
.../Core/BugReporter/BugSuppression.h | 7 +++++-
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 4 +++-
.../StaticAnalyzer/Core/BugSuppression.cpp | 10 +++++++-
.../Analysis/Checkers/WebKit/call-args.cpp | 6 +++++
.../Analysis/copypaste/suspicious-clones.cpp | 24 +++++++++++++++++++
5 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index 4fd81b6275197..419752edbb345 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
@@ -19,6 +19,7 @@
#include "llvm/ADT/SmallVector.h"
namespace clang {
+class ASTContext;
class Decl;
namespace ento {
@@ -27,6 +28,8 @@ class PathDiagnosticLocation;
class BugSuppression {
public:
+ BugSuppression(ASTContext &ACtx): ACtx(ACtx) {}
+
using DiagnosticIdentifierList = llvm::ArrayRef<llvm::StringRef>;
/// Return true if the given bug report was explicitly suppressed by the user.
@@ -44,7 +47,9 @@ class BugSuppression {
using CachedRanges =
llvm::SmallVector<SourceRange, EXPECTED_NUMBER_OF_SUPPRESSIONS>;
- llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations;
+ llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations{};
+
+ ASTContext &ACtx;
};
} // end namespace ento
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index f3e0a5f9f314a..ac9a988a7efe2 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2467,7 +2467,9 @@ ProgramStateManager &PathSensitiveBugReporter::getStateManager() const {
return Eng.getStateManager();
}
-BugReporter::BugReporter(BugReporterData &d) : D(d) {}
+BugReporter::BugReporter(BugReporterData &d)
+ : D(d), UserSuppressions(d.getASTContext()) {}
+
BugReporter::~BugReporter() {
// Make sure reports are flushed.
assert(StrBugTypes.empty() &&
diff --git a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
index b5991e47a5388..fded071567f95 100644
--- a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
@@ -138,9 +138,17 @@ bool BugSuppression::isSuppressed(const BugReport &R) {
bool BugSuppression::isSuppressed(const PathDiagnosticLocation &Location,
const Decl *DeclWithIssue,
DiagnosticIdentifierList Hashtags) {
- if (!Location.isValid() || DeclWithIssue == nullptr)
+ if (!Location.isValid())
return false;
+ if (!DeclWithIssue) {
+ // FIXME: This defeats the purpose of passing DeclWithIssue to begin with.
+ // If this branch is ever hit, we're re-doing all the work we've already
+ // done as well as perform a lot of work we'll never need.
+ // Gladly, none of our on-by-default checkers currently need it.
+ DeclWithIssue = ACtx.getTranslationUnitDecl();
+ }
+
// While some warnings are attached to AST nodes (mostly path-sensitive
// checks), others are simply associated with a plain source location
// or range. Figuring out the node based on locations can be tricky,
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 716219836e6b4..e5c4988107042 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -10,6 +10,12 @@ namespace simple {
consume_refcntbl(provide());
// expected-warning at -1{{Call argument is uncounted and unsafe}}
}
+
+ // Test that the checker works with [[clang::suppress]].
+ void foo_suppressed() {
+ [[clang::suppress]]
+ consume_refcntbl(provide()); // no-warning
+ }
}
namespace multi_arg {
diff --git a/clang/test/Analysis/copypaste/suspicious-clones.cpp b/clang/test/Analysis/copypaste/suspicious-clones.cpp
index 61eb45a37a07e..8a7b466188932 100644
--- a/clang/test/Analysis/copypaste/suspicious-clones.cpp
+++ b/clang/test/Analysis/copypaste/suspicious-clones.cpp
@@ -21,6 +21,30 @@ int maxClone(int x, int y, int z) {
return z; // expected-warning{{Potential copy-paste error; did you really mean to use 'z' here?}}
}
+// Test that the checker works with [[clang::suppress]].
+int max_suppressed(int a, int b) {
+ log();
+ if (a > b)
+ return a;
+
+ // This [[clang::suppress]] doesn't suppress anything but we need it here
+ // because otherwise the other function won't count as a perfect clone.
+ // FIXME: The checker should probably skip the attribute entirely
+ // when detecting clones. Otherwise warnings will still get suppressed,
+ // but for a completely wrong reason.
+ [[clang::suppress]]
+ return b; // no-note
+}
+
+int maxClone_suppressed(int x, int y, int z) {
+ log();
+ if (x > y)
+ return x;
+ [[clang::suppress]]
+ return z; // no-warning
+}
+
+
// Tests finding a suspicious clone that references global variables.
struct mutex {
>From af2fa0d20eabc7b48a4b2aea8ae6797ae3cc6b29 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <adergachev at apple.com>
Date: Wed, 24 Jan 2024 18:46:48 -0800
Subject: [PATCH 2/5] Fix whitespace.
---
.../clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index 419752edbb345..4f5d2d1082ee2 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
@@ -28,7 +28,7 @@ class PathDiagnosticLocation;
class BugSuppression {
public:
- BugSuppression(ASTContext &ACtx): ACtx(ACtx) {}
+ BugSuppression(ASTContext &ACtx) : ACtx(ACtx) {}
using DiagnosticIdentifierList = llvm::ArrayRef<llvm::StringRef>;
>From 24cb013427c07d5b91395e339f8e12b8fa4570fc Mon Sep 17 00:00:00 2001
From: Artem Dergachev <noqnoqneo at gmail.com>
Date: Thu, 25 Jan 2024 13:29:19 -0800
Subject: [PATCH 3/5] Update
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
Co-authored-by: Balazs Benics <benicsbalazs at gmail.com>
---
.../clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index 4f5d2d1082ee2..dd460503c84ec 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
@@ -28,7 +28,7 @@ class PathDiagnosticLocation;
class BugSuppression {
public:
- BugSuppression(ASTContext &ACtx) : ACtx(ACtx) {}
+ explicit BugSuppression(const ASTContext &ACtx) : ACtx(ACtx) {}
using DiagnosticIdentifierList = llvm::ArrayRef<llvm::StringRef>;
>From 37fa8982a98fb1ca280437998bf0cba7c33d9b25 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <noqnoqneo at gmail.com>
Date: Thu, 25 Jan 2024 13:29:31 -0800
Subject: [PATCH 4/5] Update
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
Co-authored-by: Balazs Benics <benicsbalazs at gmail.com>
---
.../clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index dd460503c84ec..35ae06575eaa0 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
@@ -47,9 +47,9 @@ class BugSuppression {
using CachedRanges =
llvm::SmallVector<SourceRange, EXPECTED_NUMBER_OF_SUPPRESSIONS>;
- llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations{};
+ llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations;
- ASTContext &ACtx;
+ const ASTContext &ACtx;
};
} // end namespace ento
>From 399a349d8f3607daa3f80a89f64f9456f9994f06 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <adergachev at apple.com>
Date: Tue, 30 Jan 2024 17:59:31 -0800
Subject: [PATCH 5/5] A tiny style fix to encourage buildbots to restart after
unrelated error.
---
clang/lib/StaticAnalyzer/Core/BugReporter.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index ac9a988a7efe2..3617fdd778e3c 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2467,8 +2467,8 @@ ProgramStateManager &PathSensitiveBugReporter::getStateManager() const {
return Eng.getStateManager();
}
-BugReporter::BugReporter(BugReporterData &d)
- : D(d), UserSuppressions(d.getASTContext()) {}
+BugReporter::BugReporter(BugReporterData &D)
+ : D(D), UserSuppressions(D.getASTContext()) {}
BugReporter::~BugReporter() {
// Make sure reports are flushed.
More information about the cfe-commits
mailing list