https://github.com/carlosgalvezp updated https://github.com/llvm/llvm-project/pull/151035
>From 89a2228e6599b5d1dbd0b734403b317076a93669 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20G=C3=A1lvez?= <carlos.gal...@zenseact.com> Date: Thu, 24 Jul 2025 21:10:43 +0000 Subject: [PATCH] [clang-tidy] Avoid matching nodes in system headers This commit is a re-do of e4a8969e56572371201863594b3a549de2e23f32, which got reverted, with the same goal: dramatically speed-up clang-tidy by avoiding doing work in system headers (which is wasteful as warnings are later discarded). This proposal was already discussed here with favorable feedback: https://github.com/llvm/llvm-project/pull/132725 The novelty of this patch is: - It's less aggressive: it does not fiddle with AST traversal. This solves the issue with the previous patch, which impacted the ability to inspect parents of a given node. - Instead, what we optimize for is exitting early in each Traverse* function of MatchASTVisitor if the node is in a system header, thus avoiding calling the match() function with its corresponding callback (when there is a match). - It does not cause any failing tests. - It does not move MatchFinderOptions outside - instead we add a user-defined default constructor which solves the same problem. - It introduces a function "shouldSkipNode" which can be extended for adding more conditions. For example there's a PR open about skipping modules in clang-tidy where this could come handy: https://github.com/llvm/llvm-project/pull/145630 As a benchmark, I ran clang-tidy with all checks activated, on a single .cpp file which #includes all the standard C++ headers, then measure the time as well as found warnings. On trunk: Suppressed 213314 warnings (213314 in non-user code). real 0m14.311s user 0m14.126s sys 0m0.185s With this patch: Suppressed 149399 warnings (149399 in non-user code). real 0m3.583s user 0m3.454s sys 0m0.128s With the original patch that got reverted: Suppressed 8050 warnings (8050 in non-user code). Runtime: around 1 second. A lot of warnings remain and the runtime is sligthly higher, but we still got a dramatic reduction with no change in functionality. Further investigation has shown that all of the remaining warnings are due to PPCallbacks - implementing a similar system-header exclusion mechanism there can lead to almost no warnings left in system headers. This does not bring the runtime down as much, though. However this may not be as straightforward or wanted, it may even need to be done on a per-check basis (there's about 10 checks or so that would need to explicitly ignore system headers). I will leave that for another patch, it's low priority and does not improve the runtime much (it just prints better statistics). Fixes #52959 --- clang-tools-extra/clang-tidy/ClangTidy.cpp | 4 ++ clang-tools-extra/docs/ReleaseNotes.rst | 4 ++ .../clang-tidy/infrastructure/file-filter.cpp | 5 -- .../infrastructure/system-headers.cpp | 4 +- clang/docs/ReleaseNotes.rst | 3 + .../clang/ASTMatchers/ASTMatchFinder.h | 5 ++ clang/lib/ASTMatchers/ASTMatchFinder.cpp | 62 +++++++++++++++++-- 7 files changed, 75 insertions(+), 12 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp index 4ae2864d310d0..b612d4f18accb 100644 --- a/clang-tools-extra/clang-tidy/ClangTidy.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -424,6 +424,10 @@ ClangTidyASTConsumerFactory::createASTConsumer( FinderOptions.CheckProfiling.emplace(Profiling->Records); } + // Avoid processing system headers, unless the user explicitly requests it + if (!Context.getOptions().SystemHeaders.value_or(false)) + FinderOptions.IgnoreSystemHeaders = true; + std::unique_ptr<ast_matchers::MatchFinder> Finder( new ast_matchers::MatchFinder(std::move(FinderOptions))); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 85b31bc0b42a6..0095ec47bad25 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,6 +101,10 @@ Improvements to clang-query Improvements to clang-tidy -------------------------- +- :program:`clang-tidy` no longer attemps to analyze code from system headers + by default, greatly improving performance. This behavior is disabled if the + `SystemHeaders` option is enabled. + - The :program:`run-clang-tidy.py` and :program:`clang-tidy-diff.py` scripts now run checks in parallel by default using all available hardware threads. Both scripts display the number of threads being used in their output. diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp index 448ef9ddf166c..d9ec1049963b0 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp @@ -66,19 +66,14 @@ class A { A(int); }; // CHECK4-NOT: warning: // CHECK4-QUIET-NOT: warning: -// CHECK: Suppressed 3 warnings (3 in non-user code) // CHECK: Use -header-filter=.* to display errors from all non-system headers. // CHECK-QUIET-NOT: Suppressed -// CHECK2: Suppressed 1 warnings (1 in non-user code) -// CHECK2: Use -header-filter=.* {{.*}} // CHECK2-QUIET-NOT: Suppressed -// CHECK3: Suppressed 2 warnings (2 in non-user code) // CHECK3: Use -header-filter=.* {{.*}} // CHECK3-QUIET-NOT: Suppressed // CHECK4-NOT: Suppressed {{.*}} warnings // CHECK4-NOT: Use -header-filter=.* {{.*}} // CHECK4-QUIET-NOT: Suppressed -// CHECK6: Suppressed 2 warnings (2 in non-user code) // CHECK6: Use -header-filter=.* {{.*}} int x = 123; diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp index 9fa990b6aac8c..a25480e9aa39c 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp @@ -11,9 +11,9 @@ // RUN: clang-tidy -help | FileCheck -check-prefix=CHECK-OPT-PRESENT %s // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=true %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s -// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s +// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers=false %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS --allow-empty %s // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: true' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-SYSTEM-HEADERS %s -// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s +// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS --allow-empty %s #include <system_header.h> // CHECK-SYSTEM-HEADERS: system_header.h:1:13: warning: single-argument constructors must be marked explicit diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 0bd4857077879..87d2c47629111 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -230,6 +230,9 @@ AST Matchers - Ensure ``hasBitWidth`` doesn't crash on bit widths that are dependent on template parameters. +- Add a boolean member ``IgnoreSystemHeaders`` to ``MatchFinderOptions``. This + allows it to ignore nodes in system headers when traversing the AST. + clang-format ------------ diff --git a/clang/include/clang/ASTMatchers/ASTMatchFinder.h b/clang/include/clang/ASTMatchers/ASTMatchFinder.h index 73cbcf1f25025..2d36e8c4fae1c 100644 --- a/clang/include/clang/ASTMatchers/ASTMatchFinder.h +++ b/clang/include/clang/ASTMatchers/ASTMatchFinder.h @@ -135,10 +135,15 @@ class MatchFinder { llvm::StringMap<llvm::TimeRecord> &Records; }; + MatchFinderOptions() {} + /// Enables per-check timers. /// /// It prints a report after match. std::optional<Profiling> CheckProfiling; + + /// Avoids matching declarations in system headers. + bool IgnoreSystemHeaders{false}; }; MatchFinder(MatchFinderOptions Options = MatchFinderOptions()); diff --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp b/clang/lib/ASTMatchers/ASTMatchFinder.cpp index 6d0ba0b7907a1..2095eb9339d98 100644 --- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp +++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp @@ -1336,6 +1336,41 @@ class MatchASTVisitor : public RecursiveASTVisitor<MatchASTVisitor>, return false; } + template <typename T> static SourceLocation getNodeLocation(const T &Node) { + return Node.getBeginLoc(); + } + + static SourceLocation getNodeLocation(const CXXCtorInitializer &Node) { + return Node.getSourceLocation(); + } + + static SourceLocation getNodeLocation(const TemplateArgumentLoc &Node) { + return Node.getLocation(); + } + + static SourceLocation getNodeLocation(const Attr &Node) { + return Node.getLocation(); + } + + bool isInSystemHeader(const SourceLocation Loc) { + const SourceManager &SM = getASTContext().getSourceManager(); + return SM.isInSystemHeader(Loc); + } + + template <typename T> bool shouldSkipNode(const T &Node) { + if (Options.IgnoreSystemHeaders && isInSystemHeader(getNodeLocation(Node))) + return true; + return false; + } + + template <typename T> bool shouldSkipNode(const T *Node) { + return (Node == nullptr) || shouldSkipNode(*Node); + } + + bool shouldSkipNode(const QualType) { return false; } + + bool shouldSkipNode(const NestedNameSpecifier) { return false; } + /// Bucket to record map. /// /// Used to get the appropriate bucket for each matcher. @@ -1465,9 +1500,8 @@ bool MatchASTVisitor::objcClassIsDerivedFrom( } bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) { - if (!DeclNode) { + if (shouldSkipNode(DeclNode)) return true; - } bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || DeclNode->isImplicit(); @@ -1495,9 +1529,9 @@ bool MatchASTVisitor::TraverseDecl(Decl *DeclNode) { } bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) { - if (!StmtNode) { + if (shouldSkipNode(StmtNode)) return true; - } + bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || TraversingASTChildrenNotSpelledInSource; @@ -1507,11 +1541,17 @@ bool MatchASTVisitor::TraverseStmt(Stmt *StmtNode, DataRecursionQueue *Queue) { } bool MatchASTVisitor::TraverseType(QualType TypeNode) { + if (shouldSkipNode(TypeNode)) + return true; + match(TypeNode); return RecursiveASTVisitor<MatchASTVisitor>::TraverseType(TypeNode); } bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode) { + if (shouldSkipNode(TypeLocNode)) + return true; + // The RecursiveASTVisitor only visits types if they're not within TypeLocs. // We still want to find those types via matchers, so we match them here. Note // that the TypeLocs are structurally a shadow-hierarchy to the expressed @@ -1523,6 +1563,9 @@ bool MatchASTVisitor::TraverseTypeLoc(TypeLoc TypeLocNode) { } bool MatchASTVisitor::TraverseNestedNameSpecifier(NestedNameSpecifier *NNS) { + if (shouldSkipNode(NNS)) + return true; + match(*NNS); return RecursiveASTVisitor<MatchASTVisitor>::TraverseNestedNameSpecifier(NNS); } @@ -1532,6 +1575,9 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc( if (!NNS) return true; + if (shouldSkipNode(NNS)) + return true; + match(NNS); // We only match the nested name specifier here (as opposed to traversing it) @@ -1544,7 +1590,7 @@ bool MatchASTVisitor::TraverseNestedNameSpecifierLoc( bool MatchASTVisitor::TraverseConstructorInitializer( CXXCtorInitializer *CtorInit) { - if (!CtorInit) + if (shouldSkipNode(CtorInit)) return true; bool ScopedTraversal = TraversingASTNodeNotSpelledInSource || @@ -1562,11 +1608,17 @@ bool MatchASTVisitor::TraverseConstructorInitializer( } bool MatchASTVisitor::TraverseTemplateArgumentLoc(TemplateArgumentLoc Loc) { + if (shouldSkipNode(Loc)) + return true; + match(Loc); return RecursiveASTVisitor<MatchASTVisitor>::TraverseTemplateArgumentLoc(Loc); } bool MatchASTVisitor::TraverseAttr(Attr *AttrNode) { + if (shouldSkipNode(AttrNode)) + return true; + match(*AttrNode); return RecursiveASTVisitor<MatchASTVisitor>::TraverseAttr(AttrNode); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
