llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Jacques Pienaar (jpienaar) <details> <summary>Changes</summary> Modify include-cleaner to recognize declarations in non-self-contained headers as roots for analysis, and to correctly attribute symbol references to these headers. This ensures that symbols used from such headers (like TableGen-generated .inc files) are correctly identified and their includes are not flagged as unused. --- Full diff: https://github.com/llvm/llvm-project/pull/185495.diff 6 Files Affected: - (modified) clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h (+10) - (modified) clang-tools-extra/include-cleaner/lib/Analysis.cpp (+3-3) - (modified) clang-tools-extra/include-cleaner/lib/Record.cpp (+26-4) - (modified) clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp (+3-1) - (modified) clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp (+25-2) - (modified) clang-tools-extra/include-cleaner/unittests/RecordTest.cpp (+21-3) ``````````diff diff --git a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h index 2dcb5ea2555c5..1e6fd8ea23cd6 100644 --- a/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h +++ b/clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h @@ -125,6 +125,11 @@ struct RecordedAST { std::unique_ptr<ASTConsumer> record(); ASTContext *Ctx = nullptr; + /// If set, some declarations from non-self-contained headers will be included + /// in Roots. + /// These are headers that are included by the main file and are not + /// self-contained. + const PragmaIncludes *PI = nullptr; /// The set of declarations written at file scope inside the main file. /// /// These are the roots of the subtrees that should be traversed to find uses. @@ -132,6 +137,11 @@ struct RecordedAST { std::vector<Decl *> Roots; }; +/// Returns true if FID is the main file or a non-self-contained file included +/// by the main file (transitively through other non-self-contained files). +bool isUsedAsMainFile(FileID FID, const SourceManager &SM, + const PragmaIncludes *PI); + /// Recorded main-file preprocessor events relevant to include-cleaner. /// /// This doesn't include facts that we record globally for the whole TU, even diff --git a/clang-tools-extra/include-cleaner/lib/Analysis.cpp b/clang-tools-extra/include-cleaner/lib/Analysis.cpp index e48a380211af0..8d93e21c06284 100644 --- a/clang-tools-extra/include-cleaner/lib/Analysis.cpp +++ b/clang-tools-extra/include-cleaner/lib/Analysis.cpp @@ -70,8 +70,7 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, RT = RefType::Ambiguous; SpellLoc = SM.getSpellingLoc(Loc); } - auto FID = SM.getFileID(SpellLoc); - if (FID != SM.getMainFileID() && FID != SM.getPreambleFileID()) + if (!isUsedAsMainFile(SM.getFileID(SpellLoc), SM, PI)) return; // FIXME: Most of the work done here is repetitive. It might be useful to // have a cache/batching. @@ -81,7 +80,8 @@ void walkUsed(llvm::ArrayRef<Decl *> ASTRoots, } for (const SymbolReference &MacroRef : MacroRefs) { assert(MacroRef.Target.kind() == Symbol::Macro); - if (!SM.isWrittenInMainFile(SM.getSpellingLoc(MacroRef.RefLocation)) || + if (!isUsedAsMainFile(SM.getFileID(SM.getSpellingLoc(MacroRef.RefLocation)), + SM, PI) || shouldIgnoreMacroReference(PP, MacroRef.Target.macro())) continue; CB(MacroRef, headersForSymbol(MacroRef.Target, PP, PI)); diff --git a/clang-tools-extra/include-cleaner/lib/Record.cpp b/clang-tools-extra/include-cleaner/lib/Record.cpp index 0284d6842e2d2..8b67832971f0d 100644 --- a/clang-tools-extra/include-cleaner/lib/Record.cpp +++ b/clang-tools-extra/include-cleaner/lib/Record.cpp @@ -460,6 +460,20 @@ bool PragmaIncludes::shouldKeep(const FileEntry *FE) const { NonSelfContainedFiles.contains(FE->getUniqueID()); } +bool isUsedAsMainFile(FileID FID, const SourceManager &SM, + const PragmaIncludes *PI) { + if (FID == SM.getMainFileID() || FID == SM.getPreambleFileID()) + return true; + SourceLocation IncludeLoc = SM.getIncludeLoc(FID); + if (IncludeLoc.isInvalid()) + return false; + if (PI) { + if (auto FE = SM.getFileEntryRefForID(FID); FE && !PI->isSelfContained(*FE)) + return isUsedAsMainFile(SM.getFileID(IncludeLoc), SM, PI); + } + return false; +} + namespace { template <typename T> bool isImplicitTemplateSpecialization(const Decl *D) { if (const auto *TD = dyn_cast<T>(D)) @@ -476,10 +490,7 @@ std::unique_ptr<ASTConsumer> RecordedAST::record() { Recorder(RecordedAST *Out) : Out(Out) {} void Initialize(ASTContext &Ctx) override { Out->Ctx = &Ctx; } bool HandleTopLevelDecl(DeclGroupRef DG) override { - const auto &SM = Out->Ctx->getSourceManager(); for (Decl *D : DG) { - if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation()))) - continue; if (isImplicitTemplateSpecialization<FunctionDecl>(D) || isImplicitTemplateSpecialization<CXXRecordDecl>(D) || isImplicitTemplateSpecialization<VarDecl>(D)) @@ -487,7 +498,18 @@ std::unique_ptr<ASTConsumer> RecordedAST::record() { // FIXME: Filter out certain Obj-C as well. Out->Roots.push_back(D); } - return ASTConsumer::HandleTopLevelDecl(DG); + return true; + } + + void HandleTranslationUnit(ASTContext &Ctx) override { + auto NotMain = [&](Decl *D) { + const auto &SM = Out->Ctx->getSourceManager(); + return !isUsedAsMainFile( + SM.getFileID(SM.getExpansionLoc(D->getLocation())), SM, Out->PI); + }; + Out->Roots.erase( + std::remove_if(Out->Roots.begin(), Out->Roots.end(), NotMain), + Out->Roots.end()); } }; diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp index fefbfc3a9614d..16f7a840a51fd 100644 --- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -132,7 +132,9 @@ class Action : public clang::ASTFrontendAction { public: Action(llvm::function_ref<bool(llvm::StringRef)> HeaderFilter, llvm::StringMap<std::string> &EditedFiles) - : HeaderFilter(HeaderFilter), EditedFiles(EditedFiles) {} + : HeaderFilter(HeaderFilter), EditedFiles(EditedFiles) { + AST.PI = &PI; + } private: RecordedAST AST; diff --git a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp index ba5a3fbbcaeb2..a4236fea978a2 100644 --- a/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp @@ -77,7 +77,8 @@ class WalkUsedTest : public testing::Test { const auto &SM = AST.sourceManager(); llvm::SmallVector<Decl *> TopLevelDecls; for (Decl *D : AST.context().getTranslationUnitDecl()->decls()) { - if (!SM.isWrittenInMainFile(SM.getExpansionLoc(D->getLocation()))) + if (!isUsedAsMainFile(SM.getFileID(SM.getExpansionLoc(D->getLocation())), + SM, &PI)) continue; TopLevelDecls.emplace_back(D); } @@ -85,7 +86,7 @@ class WalkUsedTest : public testing::Test { walkUsed(TopLevelDecls, MacroRefs, &PI, AST.preprocessor(), [&](const SymbolReference &Ref, llvm::ArrayRef<Header> Providers) { auto [FID, Offset] = SM.getDecomposedLoc(Ref.RefLocation); - if (FID != SM.getMainFileID()) + if (!isUsedAsMainFile(FID, SM, &PI)) ADD_FAILURE() << "Reference outside of the main file!"; OffsetToProviders.emplace(Offset, Providers.vec()); }); @@ -138,6 +139,28 @@ TEST_F(WalkUsedTest, Basic) { Pair(Code.point("move"), UnorderedElementsAre(UtilitySTL)))); } +TEST_F(WalkUsedTest, NonSelfContained) { + llvm::Annotations Code(R"cpp( + #include "header.inc" + void $bar^bar() { + $foo^foo(); + } + )cpp"); + Inputs.Code = Code.code(); + Inputs.ExtraFiles["header.inc"] = R"cpp( + void foo(); + )cpp"; + TestAST AST(Inputs); + auto &SM = AST.sourceManager(); + auto HeaderFile = Header(*AST.fileManager().getOptionalFileRef("header.inc")); + auto MainFile = Header(*SM.getFileEntryRefForID(SM.getMainFileID())); + EXPECT_THAT( + offsetToProviders(AST), + UnorderedElementsAre( + Pair(Code.point("bar"), UnorderedElementsAre(MainFile)), + Pair(Code.point("foo"), UnorderedElementsAre(MainFile, HeaderFile)))); +} + TEST_F(WalkUsedTest, MultipleProviders) { llvm::Annotations Code(R"cpp( #include "header1.h" diff --git a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp index cbf7bae23b365..b62214a86e910 100644 --- a/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp +++ b/clang-tools-extra/include-cleaner/unittests/RecordTest.cpp @@ -72,18 +72,25 @@ class RecordASTTest : public ::testing::Test { RecordASTTest() { struct RecordAction : public ASTFrontendAction { RecordedAST &Out; - RecordAction(RecordedAST &Out) : Out(Out) {} + PragmaIncludes &PI; + RecordAction(RecordedAST &Out, PragmaIncludes &PI) : Out(Out), PI(PI) {} + bool BeginSourceFileAction(CompilerInstance &CI) override { + PI.record(CI); + Out.PI = &PI; + return true; + } std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI, StringRef) override { return Out.record(); } }; Inputs.MakeAction = [this] { - return std::make_unique<RecordAction>(Recorded); + return std::make_unique<RecordAction>(Recorded, PI); }; } TestAST build() { return TestAST(Inputs); } + PragmaIncludes PI; }; // Top-level decl from the main file is a root, nested ones aren't. @@ -103,7 +110,7 @@ TEST_F(RecordASTTest, Namespace) { // Decl in included file is not a root. TEST_F(RecordASTTest, Inclusion) { - Inputs.ExtraFiles["header.h"] = "void headerFunc();"; + Inputs.ExtraFiles["header.h"] = "#pragma once\nvoid headerFunc();"; Inputs.Code = R"cpp( #include "header.h" void mainFunc(); @@ -112,6 +119,16 @@ TEST_F(RecordASTTest, Inclusion) { EXPECT_THAT(Recorded.Roots, testing::ElementsAre(named("mainFunc"))); } +// Decl in non-self-contained included file is a root. +TEST_F(RecordASTTest, NonSelfContainedInclusion) { + Inputs.ExtraFiles["header.inc"] = "void headerFunc();"; + Inputs.Code = R"cpp( + #include "header.inc" + )cpp"; + auto AST = build(); + EXPECT_THAT(Recorded.Roots, testing::ElementsAre(named("headerFunc"))); +} + // Decl from macro expanded into the main file is a root. TEST_F(RecordASTTest, Macros) { Inputs.ExtraFiles["header.h"] = "#define X void x();"; @@ -126,6 +143,7 @@ TEST_F(RecordASTTest, Macros) { // Decl from template instantiation is filtered out from roots. TEST_F(RecordASTTest, ImplicitTemplates) { Inputs.ExtraFiles["dispatch.h"] = R"cpp( + #pragma once struct A { static constexpr int value = 1; }; `````````` </details> https://github.com/llvm/llvm-project/pull/185495 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
