[analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. #79398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Artem Dergachev (haoNoQ) ChangesThere 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 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 won't happen in the default setup because luckily all default checkers do provide decl-with-issue. Full diff: https://github.com/llvm/llvm-project/pull/79398.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index 4fd81b627519744..419752edbb34578 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 f3e0a5f9f314ade..ac9a988a7efe298 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 b5991e47a538875..fded071567f9582 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 716219836e6b445..e5c498810704203 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@-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 61eb45a37a07e6f..8a7b46618893298 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 {
|
|
@llvm/pr-subscribers-clang Author: Artem Dergachev (haoNoQ) ChangesThere 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 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 won't happen in the default setup because luckily all default checkers do provide decl-with-issue. Full diff: https://github.com/llvm/llvm-project/pull/79398.diff 5 Files Affected:
diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index 4fd81b627519744..419752edbb34578 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 f3e0a5f9f314ade..ac9a988a7efe298 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 b5991e47a538875..fded071567f9582 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 716219836e6b445..e5c498810704203 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@-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 61eb45a37a07e6f..8a7b46618893298 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 {
|
haoNoQ
requested review from
Xazax-hun,
steakhal,
Szelethus,
jkorous-apple,
balazske and
NagyDonat
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The affected checkers were:
|
Xazax-hun
approved these changes
Jan 25, 2024
steakhal
reviewed
Jan 25, 2024
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
Outdated
Show resolved
Hide resolved
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
Outdated
Show resolved
Hide resolved
jkorous-apple
approved these changes
Jan 25, 2024
…sion.h Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
…sion.h Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
steakhal
approved these changes
Jan 26, 2024
carlosgalvezp
pushed a commit
to carlosgalvezp/llvm-project
that referenced
this pull request
Feb 1, 2024
…issue. (llvm#79398) 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 won't happen in the default setup because luckily all default checkers do provide decl-with-issue. --------- Co-authored-by: Balazs Benics <benicsbalazs@gmail.com>
haoNoQ
added a commit
to haoNoQ/llvm-project
that referenced
this pull request
Feb 13, 2024
…issue. (llvm#79398) 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 won't happen in the default setup because luckily all default checkers do provide decl-with-issue. --------- Co-authored-by: Balazs Benics <benicsbalazs@gmail.com> (cherry picked from commit 56e241a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 won't happen in the default setup because luckily all default checkers do provide decl-with-issue.