Author: Artem Dergachev Date: 2024-01-31T13:55:31-08:00 New Revision: 56e241a07ff7f839d39775dbf05de5153a8e7d9f
URL: https://github.com/llvm/llvm-project/commit/56e241a07ff7f839d39775dbf05de5153a8e7d9f DIFF: https://github.com/llvm/llvm-project/commit/56e241a07ff7f839d39775dbf05de5153a8e7d9f.diff LOG: [analyzer] Unbreak [[clang::suppress]] on checkers without decl-with-issue. (#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 <benicsbal...@gmail.com> Added: Modified: 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 Removed: ################################################################################ diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h index 4fd81b6275197..35ae06575eaa0 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: + explicit BugSuppression(const ASTContext &ACtx) : ACtx(ACtx) {} + using DiagnosticIdentifierList = llvm::ArrayRef<llvm::StringRef>; /// Return true if the given bug report was explicitly suppressed by the user. @@ -45,6 +48,8 @@ class BugSuppression { llvm::SmallVector<SourceRange, EXPECTED_NUMBER_OF_SUPPRESSIONS>; llvm::DenseMap<const Decl *, CachedRanges> CachedSuppressionLocations; + + const ASTContext &ACtx; }; } // end namespace ento diff --git a/clang/lib/StaticAnalyzer/Core/BugReporter.cpp b/clang/lib/StaticAnalyzer/Core/BugReporter.cpp index f3e0a5f9f314a..3617fdd778e3c 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@-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 { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits