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

[Artem Dergachev via cfe-commits]

https://github.com/haoNoQ closed https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via cfe-commits]

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 
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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // 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 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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@-1{{Call argument is uncounted and unsafe}}
   }
+
+  // Test that the checker works with [[clang::suppress]].
+  void foo_suppressed() {
+[[clang::suppress]]
+

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

[Balazs Benics via cfe-commits]

https://github.com/steakhal approved this pull request.


https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via cfe-commits]

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 
Date: Wed, 24 Jan 2024 14:52:58 -0800
Subject: [PATCH 1/4] [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 4fd81b62751974..419752edbb3457 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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // end namespace ento
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp 
b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
index f3e0a5f9f314ad..ac9a988a7efe29 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -2467,7 +2467,9 @@ ProgramStateManager 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 b5991e47a53887..fded071567f958 100644
--- a/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugSuppression.cpp
@@ -138,9 +138,17 @@ bool BugSuppression::isSuppressed(const BugReport ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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 716219836e6b44..e5c49881070420 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] [analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. (PR #79398)

[Artem Dergachev via cfe-commits]

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 
Date: Wed, 24 Jan 2024 14:52:58 -0800
Subject: [PATCH 1/3] [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 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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // 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 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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] [analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. (PR #79398)

[Artem Dergachev via cfe-commits]

@@ -27,6 +28,8 @@ class PathDiagnosticLocation;
 
 class BugSuppression {
 public:
+  BugSuppression(ASTContext ) : ACtx(ACtx) {}

haoNoQ wrote:

Hmm I somehow never noticed that we have any actual const-correctness for 
`ASTContext`, thanks!

https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

https://github.com/jkorous-apple approved this pull request.

Thanks for fixing this!
LGTM from the perspective of WebKit checkers.

https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balazs Benics via cfe-commits]

https://github.com/steakhal edited 
https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balazs Benics via cfe-commits]

@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 

steakhal wrote:

```suggestion
  llvm::DenseMap CachedSuppressionLocations;

  const ASTContext 
```

https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balazs Benics via cfe-commits]

@@ -27,6 +28,8 @@ class PathDiagnosticLocation;
 
 class BugSuppression {
 public:
+  BugSuppression(ASTContext ) : ACtx(ACtx) {}

steakhal wrote:

```suggestion
  explicit BugSuppression(const ASTContext ) : ACtx(ACtx) {}
```

https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balazs Benics via cfe-commits]

https://github.com/steakhal commented:

I only have minor nits. No objections.

https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via cfe-commits]

https://github.com/Xazax-hun approved this pull request.


https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via cfe-commits]

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 
Date: Wed, 24 Jan 2024 14:52:58 -0800
Subject: [PATCH 1/2] [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 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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // 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 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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] [analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. (PR #79398)

[Artem Dergachev via cfe-commits]

haoNoQ wrote:

The affected checkers were:
- Checkers in the `webkit` package (hence cc @jkorous-apple). These checkers 
manually scan the entire TU in order (rejecting the comforts of 
`checkASTCodeBody`) in order to properly cover uninstantiated templates. 
There's no reason they can't provide a decl-with-issue but it's often annoying 
to detect.
- Copy-paste error checkers. Again, the checker can provide decl-with-issue, 
but plumbing gets quite annoying. The test case is super funny though!

https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff a58dcc5e08665f2d58a28c9d4510cf94de6ed3bf 
21923f92f0ea4169d5fea646221554d4c44e36f1 -- 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h 
clang/lib/StaticAnalyzer/Core/BugReporter.cpp 
clang/lib/StaticAnalyzer/Core/BugSuppression.cpp 
clang/test/Analysis/Checkers/WebKit/call-args.cpp 
clang/test/Analysis/copypaste/suspicious-clones.cpp
``





View the diff from clang-format here.


``diff
diff --git 
a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h 
b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h
index 419752edbb..4f5d2d1082 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) {}
+  BugSuppression(ASTContext ) : ACtx(ACtx) {}
 
   using DiagnosticIdentifierList = llvm::ArrayRef;
 

``




https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via cfe-commits]

https://github.com/haoNoQ unassigned 
https://github.com/llvm/llvm-project/pull/79398
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

llvmbot wrote:




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

Author: Artem Dergachev (haoNoQ)


Changes

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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // 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 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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 

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

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Artem Dergachev (haoNoQ)


Changes

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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // 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 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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 

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

[Artem Dergachev via cfe-commits]

https://github.com/haoNoQ created 
https://github.com/llvm/llvm-project/pull/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.

>From 21923f92f0ea4169d5fea646221554d4c44e36f1 Mon Sep 17 00:00:00 2001
From: Artem Dergachev 
Date: Wed, 24 Jan 2024 14:52:58 -0800
Subject: [PATCH] [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 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) {}
+
   using DiagnosticIdentifierList = llvm::ArrayRef;
 
   /// Return true if the given bug report was explicitly suppressed by the 
user.
@@ -44,7 +47,9 @@ class BugSuppression {
   using CachedRanges =
   llvm::SmallVector;
 
-  llvm::DenseMap CachedSuppressionLocations;
+  llvm::DenseMap CachedSuppressionLocations{};
+
+  ASTContext 
 };
 
 } // 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 
::getStateManager() const {
   return Eng.getStateManager();
 }
 
-BugReporter::BugReporter(BugReporterData ) : D(d) {}
+BugReporter::BugReporter(BugReporterData )
+: 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 ) {
 bool BugSuppression::isSuppressed(const PathDiagnosticLocation ,
   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