From ea18a659f5eb7283011d21e6370a7becfc7df39e Mon Sep 17 00:00:00 2001 From: June Rhodes Date: Thu, 1 Aug 2024 20:32:37 +1000 Subject: [PATCH] Fix two IWYU detection bugs (#8) - Fixed an issue where a forward declaration after the main declaration would incorrectly be marked as the dependency, instead of the real declaration. - Fixed an issue where dependencies transitively included in headers that aren't being IWYU tracked, from an IWYU tracked header, would cause the includes in the IWYU tracked header to be marked as removable (even though a transitive dependency comes from them). --- clang/lib/Frontend/ClangRulesets.cpp | 110 +++++++++++++++++++++------ 1 file changed, 86 insertions(+), 24 deletions(-) diff --git a/clang/lib/Frontend/ClangRulesets.cpp b/clang/lib/Frontend/ClangRulesets.cpp index ee2f6542942ba9..a5dc5191ddedfb 100644 --- a/clang/lib/Frontend/ClangRulesets.cpp +++ b/clang/lib/Frontend/ClangRulesets.cpp @@ -666,6 +666,7 @@ class ClangRulesetsState { llvm::DenseMap> IWYUDependencyTree; llvm::DenseMap> IWYUIncludeTree; + llvm::DenseSet IWYUIncludeMustTrack; clang::FileID LastIWYUFileID; clang::OptionalFileEntryRef LastIWYUFileEntry; @@ -676,7 +677,7 @@ class ClangRulesetsState { Timing(std::make_unique()), CreatedEffectiveConfigs(), RuleByNamespacedName(), RulesetByNamespacedName(), IWYUDependencyTree(), IWYUIncludeTree(), - LastIWYUFileID(), LastIWYUFileEntry(){}; + IWYUIncludeMustTrack(), LastIWYUFileID(), LastIWYUFileEntry(){}; ClangRulesetsState(const ClangRulesetsState &) = delete; ClangRulesetsState(ClangRulesetsState &&) = delete; ~ClangRulesetsState() { @@ -690,8 +691,7 @@ class ClangRulesetsState { public: void iwyuTrackInclusionDirective(SourceLocation IncludingHashLoc, OptionalFileEntryRef IncludedFile) { - llvm::DenseMap::iterator - DirState; + { RULESET_TIME_REGION_BEFORE_ANALYSIS( this->CI, Timer, this->Timing, @@ -708,12 +708,18 @@ class ClangRulesetsState { if (!LastIWYUFileEntry) { return; } - DirState = this->Dirs.find(LastIWYUFileEntry->getDir()); + } + // @note: If A has IWYU enabled, and it includes B, then we must track B's + // includes so that transitive dependency checks work (i.e. if A uses a + // function from a header that B includes, we don't want to incorrectly + // report that the header B isn't used because we can't reach the dependency + // via B's headers). The IWYUIncludeMustTrack set keeps track of these + // files. + if (!IWYUIncludeMustTrack.contains(*LastIWYUFileEntry)) { + auto DirState = this->Dirs.find(LastIWYUFileEntry->getDir()); if (DirState == this->Dirs.end()) { return; } - } - { if (!DirState->second.Materialized) { RULESET_TIME_REGION_BEFORE_ANALYSIS( this->CI, MaterializationTimer, this->Timing, @@ -733,6 +739,7 @@ class ClangRulesetsState { RulesetIWYUPreprocessorCaptureIncludeDependencyInsertTimer); auto &List = IWYUIncludeTree.getOrInsertDefault(*LastIWYUFileEntry); List.try_emplace(*IncludedFile, IncludingHashLoc); + IWYUIncludeMustTrack.insert(*IncludedFile); RULESET_TRACE_IWYU_PREPROCESSOR( "File '" << LastIWYUFileEntry->getName() << "' includes file '" << IncludedFile->getName() << "'\n") @@ -795,7 +802,76 @@ class ClangRulesetsState { } } - void iwyuTrackSemaUsage(LookupResult &Result) { + void iwyuTrackSemaUsageDecl(llvm::DenseSet *DepList, + NamedDecl *Dest, + const OptionalFileEntryRef &UsageFile, + bool ScanRedecls) { + // Add this decl. + auto DestEntry = SrcMgr.getFileEntryRefForID( + SrcMgr.getFileID(SrcMgr.getFileLoc(Dest->getLocation()))); + if (DestEntry) { + if (DepList == nullptr) { + DepList = &IWYUDependencyTree.getOrInsertDefault(*UsageFile); + } +#if RULESET_ENABLE_TRACING_IWYU + if (!DepList->contains(*DestEntry)) { + RULESET_TRACE_IWYU("C++ sema dependency: '" + << UsageFile->getName() << "' depends on decl '" + << Dest->getDeclName() << "' ('" + << Dest->getDeclKindName() << "') in '" + << DestEntry->getName() << "'\n") + } +#endif + DepList->insert(*DestEntry); + } else { + RULESET_TRACE_IWYU("C++ sema dependency: skipped (no file location)\n") + } + + // Also add dependencies on the underlying decl for things that have been + // aliased. + NamedDecl *UnderlyingDecl = Dest->getUnderlyingDecl(); + if (UnderlyingDecl != nullptr && UnderlyingDecl != Dest) { + RULESET_TRACE_IWYU("C++ sema dependency had underlying decl.\n") + this->iwyuTrackSemaUsageDecl(DepList, UnderlyingDecl, UsageFile, + ScanRedecls); + } + + // If this is a tag decl, add a dependency on all non-forward redeclarations + // to catch scenarios where a forward declaration occurs after the real + // declaration due to header include ordering. + if (ScanRedecls && isa(Dest)) { + TagDecl *TD = cast(Dest); + for (const auto &Redecl : TD->redecls()) { + if (Redecl == nullptr) { + continue; + } + if (Redecl == Dest) { + RULESET_TRACE_IWYU( + "C++ sema dependency redecl: (same) '" + << Redecl->getDeclName() << "' ('" + << ((DeclContext *)Redecl)->getDeclKindName() << "') " + << Redecl->getLocation().printToString(SrcMgr) << "\n") + continue; + } + if (!Redecl->isCompleteDefinition()) { + RULESET_TRACE_IWYU( + "C++ sema dependency redecl: (incomplete) '" + << Redecl->getDeclName() << "' ('" + << ((DeclContext *)Redecl)->getDeclKindName() << "') " + << Redecl->getLocation().printToString(SrcMgr) << "\n") + continue; + } + RULESET_TRACE_IWYU( + "C++ sema dependency redecl: (recursing) '" + << Redecl->getDeclName() << "' ('" + << ((DeclContext *)Redecl)->getDeclKindName() << "') " + << Redecl->getLocation().printToString(SrcMgr) << "\n") + this->iwyuTrackSemaUsageDecl(DepList, Redecl, UsageFile, false); + } + } + } + + void iwyuTrackSemaUsageInternal(LookupResult &Result) { llvm::DenseMap::iterator DirState; { @@ -838,31 +914,17 @@ class ClangRulesetsState { if (Dest == nullptr) { continue; } - auto DestEntry = SrcMgr.getFileEntryRefForID( - SrcMgr.getFileID(SrcMgr.getFileLoc(Dest->getLocation()))); - if (DestEntry) { - if (DepList == nullptr) { - DepList = &IWYUDependencyTree.getOrInsertDefault(*UsageFile); - } -#if RULESET_ENABLE_TRACING_IWYU - if (!DepList->contains(*DestEntry)) { - RULESET_TRACE_IWYU("C++ sema dependency: '" - << UsageFile->getName() << "' depends on '" - << DestEntry->getName() << "'\n") - } -#endif - DepList->insert(*DestEntry); - } + this->iwyuTrackSemaUsageDecl(DepList, Dest, UsageFile, true); } } } void iwyuTrackSemaUsage(LookupResult &Result, Scope *Scope) { - iwyuTrackSemaUsage(Result); + iwyuTrackSemaUsageInternal(Result); } void iwyuTrackSemaUsage(LookupResult &Result, DeclContext *LookupCtx) { - iwyuTrackSemaUsage(Result); + iwyuTrackSemaUsageInternal(Result); } private: