[clang] [analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. (PR #79398)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Artem Dergachev (haoNoQ)

<details>
<summary>Changes</summary>

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/79398.diff


5 Files Affected:

- (modified) clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h (+6-1) 
- (modified) clang/lib/StaticAnalyzer/Core/BugReporter.cpp (+3-1) 
- (modified) clang/lib/StaticAnalyzer/Core/BugSuppression.cpp (+9-1) 
- (modified) clang/test/Analysis/Checkers/WebKit/call-args.cpp (+6) 
- (modified) clang/test/Analysis/copypaste/suspicious-clones.cpp (+24) 


``````````diff
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 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 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 {

``````````

</details>


https://github.com/llvm/llvm-project/pull/79398