Page MenuHomePhabricator

[clang-tidy] Add --skip-headers, part 1 (with TopLevelDecl bit)
Needs ReviewPublic

Authored by chh on Mar 14 2021, 11:14 PM.

Details

Summary
  • Add a TopLevelDecl bit to DeclBase class, so that TraverseDecl can quickly check isTopLevelDecl(). Note that this bit is set only before AST traversal, and not preserved in serialization.
  • Add the --skip-headers flag. When it is used, clang-tidy checks should not check header files, unless a header file path matches the regular expression specified by the --header-filter flag or is a system header and --system-headers flag is used.
    • Add a shared TidyDeclFilter to Tidy's MatchFinderOptions.
    • In MatchASTVisitor::TraverseDecl, skip a DeclNode if the MatchFinderOptions has a DeclFilter that decides to skip the DeclNode location.
    • TidyDeclFilter::skipDecl checks/skips top level Decls.
    • Note that PPCallback-based tidy checks are not affected by --skip-headers yet. They could be changed in part 2 implementation.
  • Use simple cache of the last skipped/accepted FileID in TidyDeclFilter to minimize the overhead of skipLocation.
  • Add the --show-all-warnings flag. It is hidden, not shown by --help. When it is used, ClangTidyDiagnosticConsumer::checkFilters will not suppress any diagnostic message.
    • This is useful to find tidy checks that have not yet handled the --skip-headers flag.
  • Add skip-headers.cpp test to show the effects of new flags.
  • Extend some clang-tidy checker tests to make sure that --skip-headers skips checks/warnings in the header files:
    • google-namespaces.cpp
    • modernize-pass-by-value-header.cpp
  • Add runs with --skip-headers in some clang-tidy checker tests to make sure that the option does not hide warnings in the main file:
    • bugprone-forward-declaration-namespace.cpp
    • bugprone-reserved-identifier.cpp
    • bugprone-suspicious-include.cpp
    • cppcoreguidelines-interfaces-global-init.cpp
    • llvm-include-order.cpp
    • llvm-prefer-register-over-unsigned.cpp
    • llvmlibc-implementation-in-namespace.cpp
    • llvmlibc-restrict-system-libc-headers.cpp
    • misc-no-recursion.cpp
    • modernize-deprecated-headers-cxx03.cpp
    • modernize-deprecated-headers-cxx11.cpp
    • portability-restrict-system-includes-allow.cpp
    • portability-restrict-system-includes-disallow.cpp
    • readability-simplify-bool-expr-chained-conditional-assignment.cpp
    • readability-simplify-bool-expr-chained-conditional-return.cpp
    • readability-simplify-bool-expr-members.cpp
    • readability-simplify-bool-expr.cpp

Diff Detail

Event Timeline

chh created this revision.Mar 14 2021, 11:14 PM
chh requested review of this revision.Mar 14 2021, 11:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2021, 11:14 PM
chh retitled this revision from Add --skip-headers, part 1 to [clang-tidy] Add --skip-headers, part 1.Mar 14 2021, 11:16 PM
chh edited the summary of this revision. (Show Details)
chh edited the summary of this revision. (Show Details)Mar 14 2021, 11:28 PM
njames93 added a subscriber: njames93.

Can I ask what is the purpose of --skip-headers?
What does it offer that --header-filter doesn't already offer?

sammccall requested changes to this revision.Mar 15 2021, 4:20 AM

Can I ask what is the purpose of --skip-headers?
What does it offer that --header-filter doesn't already offer?

TL;DR: it avoids traversing a bunch of nodes where diagnostics will be filtered out anyway, thus running significantly faster.

@chh You're written quite a lot about the design of this, can you make this public please?

Per previous discussion, I'm unhappy with a design that will requires modifications scattered through checks and adding an extra filter to ASTMatchers without a serious attempt to make the existing ASTContext::setTraversalScope do what you want.

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
98

This is a debugging option, but has a name likely to suggest all sorts of maybe-useful behaviors to users.
I'd suggest renaming it and making it Hidden.

clang/include/clang/AST/DeclBase.h
611

Is there a strong reason to make this a new serialized property in the AST?
This information is already available to clang-tidy through the ASTConsumer interface.

This revision now requires changes to proceed.Mar 15 2021, 4:20 AM
chh updated this revision to Diff 330824.Mar 15 2021, 3:22 PM
chh edited the summary of this revision. (Show Details)
chh added a comment.Mar 15 2021, 3:40 PM

Can I ask what is the purpose of --skip-headers?
What does it offer that --header-filter doesn't already offer?

TL;DR: it avoids traversing a bunch of nodes where diagnostics will be filtered out anyway, thus running significantly faster.

@chh You're written quite a lot about the design of this, can you make this public please?

Sure, I will upload or share the design doc as soon as I find a way.

Per previous discussion, I'm unhappy with a design that will requires modifications scattered through checks and adding an extra filter to ASTMatchers without a serious attempt to make the existing ASTContext::setTraversalScope do what you want.

I have tried setTraversalScope for more than 2 weeks without success. Could you show me how to solve the problems?
(1) As it is now only used by clangd, it skips root translationUnitDecl and fails multiple clang-tidy checks. One way to fix that is to change the failing clang-tidy checks now and in the future. That seems much harder to maintain. This patch does not change any clang-tidy checks.
(2) One alternative I tried to preserve the translationUnitDecl and not changing any clang-tidy checks, ends up just like this patch. The AST is preserved and only unwanted top level Decls are skipped, either pre-AST construction or on the fly during traversal.

When I compared any solution that uses setTraversalScope, I found that this patch has much fewer changes.

chh added inline comments.Mar 15 2021, 3:50 PM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
98

What name do you suggest?
Did you mean "undocumented" flags?
Is there any rule against that?

clang/include/clang/AST/DeclBase.h
611

For clang-tidy and this feature, this new bit does not need to be preserved during serialization. I thought it might be simpler to preserve it during serialization by default. Now seeing many test failed, I have uploaded a new patch to try without serialization changes.

chh added a comment.Mar 15 2021, 4:14 PM

Here are some important sections copied from my design docs.
This patch presents only one of the simplest working implementation I have tried.
I hope it would serve as a good baseline to compare any alternatives.

Introduction

Clang-tidy by default suppresses all warnings in header files. Its --header-filter option accepts a regular expression so that warnings of files matching the regular expression will be shown. Its --system-headers option is used to display warnings of system header files.

Even when --header-filter and --system-headers are not used, clang-tidy still spends time checking header files. Typical C/C++ source files include many header files and we measured a large portion of clang-tidy run time spent in checking header files.

The new clang-tidy --skip-headers option will really skip checks of header files. The --show-all-warnings option will display all warnings, no matter what is specified by --header-filter or --system-headers.

Preliminary Results

An implementation with these clang-tidy options was tested with both simplified tests and real Android builds.

With small files and only clang-tidy checks, no static analysis checks, we see 80% to 90% saved time by skip-headers. With large real Android compilation units, we still see 50% to 75% savings.

When the clang-analyzer-* checks are also enabled, the skip-header option can save 4% to 60% time, depending on the percentage of code needing static analysis.

When building in an Android subdirectory, total saving of build time due to saving from clang-tidy --skip-headers could be a few percents to 1/3. This saving depends on the amount of other non-clang-tidy related compilation and linking of object files.

Implementation Alternatives

For the skip-headers feature, we have different plans for the 3 kinds of clang-tidy checks:

  • For ASTMatchFinder-based checks, we compared 3 implementations.
  • For the PPCallbacks-based checks, we tried one implementation with good results as shown in the previous sections, but it required changes into every such callback function. We don't have better alternatives yet, so the implementation is pushed to our step2 plan.
  • For the static-analyzer based clang-analyzer-* checks, we do not have any implementation yet. Maybe it will be possible in the future.

For the near-future, step1 implementation, we should define skip-headers as a feature to save part but not all tidy checks on header files. More implementations in the future could save more checks on header files, but this flag will not lose diagnostics unless they are already suppressed now.

Note that in all alternatives to skip ASTMatchFinder-based checks, there are common assumptions and algorithms:

  • ASTMatchFinder-based checks are triggered from MatchASTVisitor during the recursive walk over AST nodes. By skipping some header file AST nodes, the tidy checks can still work properly. The only impact should be the possible diagnostics on the AST nodes in header files. If an AST Decl node is in a header file to be skipped, it and its sub-nodes can be skipped by tidy checks. However, these nodes should still be available, for example if referred by other code in the non-skipped files.
  • To honor the --header-filter and --system-headers flag, when comparing a Decl node's file location, we should not skip files that were selected by header-filter or system-headers.
  • Some caching optimization is used to avoid repeated checks of the same file for many Decl nodes.
  • It is okay if some clang-tidy checks have their own AST walk and issue diagnostics on should-be-skipped headers. This could be considered a missed optimization.
  • A diagnostic not on skipped headers should never be lost by skip-headers.

We use a TidyFileFilter that can handle above file-location-checks-and-caches. The following two public methods are used to check if a given AST (Decl) or an AST node's SourceLocation should be skipped.

TidyFileFilter::skipDecl(Decl)
TidyFileFilter::skipLocation(SourceLocation)

Its private methods will cache checked source location (FileID) to minimize overhead.

In the previous sections, our prototype to skip PPCallbacks-based checks also uses TidyFileFilter::skipLocation.

The alternatives we are comparing all use the TidyFileFilter class. The difference is whether to use it during the recursive walk of AST, or to use it to change AST before the walk of AST.

The optional TidyFileFilter is added like this:
In the MatchFinder::Options add one FileFilter optional shared pointer.
It will be set up only when the --skip-headers flag is used:

std::shared_ptr<OptionsFileFilter> SharedFileFilter;
if (*Context.getOptions().SkipHeaders)
  SharedFileFilter = std::shared_ptr<OptionsFileFilter>(
      new TidyFileFilter(Context, *SM));
FinderOptions.Filter = SharedFileFilter;

This is similar to the current setting of the CheckProfiling option.

if (Context.getEnableProfiling()) {
  Profiling = std::make_unique<ClangTidyProfiling>(
      Context.getProfileStorageParams());
  FinderOptions.CheckProfiling.emplace(Profiling->Records);
}
chh updated this revision to Diff 330880.Mar 15 2021, 10:02 PM

fix windows test failures

chh updated this revision to Diff 339067.Apr 20 2021, 6:15 PM
chh edited the summary of this revision. (Show Details)

hide the --show-all-warnings flag from --help

chh updated this revision to Diff 351615.Jun 11 2021, 6:32 PM

sync with latest clang/llvm

chh retitled this revision from [clang-tidy] Add --skip-headers, part 1 to [clang-tidy] Add --skip-headers, part 1 (with TopLevelDecl bit).Jun 11 2021, 7:56 PM
chh added a comment.Jun 11 2021, 8:01 PM

Please review https://reviews.llvm.org/D98710, which uses get/setTraversalScope instead of adding a TopLevelDecl bit.