[clang] Support header shadowing diagnostics in Clang header search #162491
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -444,8 +444,8 @@ StringRef DirectoryLookup::getName() const { | |
| OptionalFileEntryRef HeaderSearch::getFileAndSuggestModule( | ||
| StringRef FileName, SourceLocation IncludeLoc, const DirectoryEntry *Dir, | ||
| bool IsSystemHeaderDir, Module *RequestingModule, | ||
| ModuleMap::KnownHeader *SuggestedModule, bool NeedSuggest, | ||
| bool OpenFile /*=true*/, bool CacheFailures /*=true*/) { | ||
| // If we have a module map that might map this header, load it and | ||
| // check whether we'll have a suggestion for a module. | ||
| auto File = getFileMgr().getFileRef(FileName, OpenFile, CacheFailures); | ||
|
|
@@ -462,6 +462,9 @@ OptionalFileEntryRef HeaderSearch::getFileAndSuggestModule( | |
| return std::nullopt; | ||
| } | ||
|
|
||
| if (!NeedSuggest) | ||
| return *File; | ||
|
|
||
| // If there is a module that corresponds to this header, suggest it. | ||
| if (!findUsableModuleForHeader( | ||
| *File, Dir ? Dir : File->getFileEntry().getDir(), RequestingModule, | ||
|
|
@@ -478,7 +481,7 @@ OptionalFileEntryRef DirectoryLookup::LookupFile( | |
| SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath, | ||
| Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, | ||
| bool &InUserSpecifiedSystemFramework, bool &IsFrameworkFound, | ||
| bool &IsInHeaderMap, SmallVectorImpl<char> &MappedName, bool NeedSuggest, | ||
| bool OpenFile) const { | ||
| InUserSpecifiedSystemFramework = false; | ||
| IsInHeaderMap = false; | ||
|
|
@@ -489,19 +492,21 @@ OptionalFileEntryRef DirectoryLookup::LookupFile( | |
| // Concatenate the requested file onto the directory. | ||
| TmpDir = getDirRef()->getName(); | ||
| llvm::sys::path::append(TmpDir, Filename); | ||
| if (NeedSuggest) { | ||
| if (SearchPath) { | ||
| StringRef SearchPathRef(getDirRef()->getName()); | ||
| SearchPath->clear(); | ||
| SearchPath->append(SearchPathRef.begin(), SearchPathRef.end()); | ||
| } | ||
| if (RelativePath) { | ||
| RelativePath->clear(); | ||
| RelativePath->append(Filename.begin(), Filename.end()); | ||
| } | ||
| } | ||
|
|
||
| return HS.getFileAndSuggestModule( | ||
| TmpDir, IncludeLoc, getDir(), isSystemHeaderDirectory(), | ||
| RequestingModule, SuggestedModule, NeedSuggest, OpenFile); | ||
| } | ||
|
|
||
| if (isFramework()) | ||
|
|
@@ -881,6 +886,35 @@ diagnoseFrameworkInclude(DiagnosticsEngine &Diags, SourceLocation IncludeLoc, | |
| << IncludeFilename; | ||
| } | ||
|
|
||
| /// Return true if a shadow has been detected and the caller should | ||
| /// stop and return the first-found file and module, false otherwise. | ||
| static bool checkAndStoreCandidate( | ||
| ModuleMap::KnownHeader *SuggestedModule, OptionalFileEntryRef CandidateFile, | ||
| StringRef CandidateDir, DiagnosticsEngine &Diags, StringRef Filename, | ||
| SourceLocation IncludeLoc, ModuleMap::KnownHeader &FirstModule, | ||
| OptionalFileEntryRef &FirstHeader, SmallString<1024> &FirstDir) { | ||
| if (!FirstHeader) { | ||
| // Found the first candidate | ||
| FirstHeader = CandidateFile; | ||
| FirstDir = CandidateDir; | ||
| if (SuggestedModule) | ||
| FirstModule = *SuggestedModule; | ||
| return false; | ||
| } | ||
|
|
||
| if (FirstDir != CandidateDir) { | ||
| // Found a second candidate from a different directory | ||
| Diags.Report(IncludeLoc, diag::warn_header_shadowed) | ||
| << Filename << FirstDir << CandidateDir; | ||
| if (SuggestedModule) | ||
| *SuggestedModule = FirstModule; | ||
| return true; | ||
| } | ||
|
|
||
| // Found a candidate from the same directory as the first one | ||
| return false; | ||
|
Comment on lines
+914
to
+915
There was a problem hiding this comment. Clang elides duplicate search paths during search path ordering. I think finding the same header file in the same location can only happen in some cases where the same include path is present in both the quoted search path and angle search path (and that might only be in existing cases where Clang doesn't match GCC behavior). It could also happen with the corrected MSVC search path ordering from my draft PR, but since MSVC doesn't support There was a problem hiding this comment. Thanks for review. Based on the code implementation and my testing, it appears that although the search paths themselves are deduplicated, the current baseline implementation executes two search strategies simultaneously: 1. MSVC rule (not corrected) for "quoted search": This strategy searches based on the include stack, checking each directory level in the stack. For example, in ./x.cpp -> a/a.h -> a/b.h, when searching for b.h, it first looks in the paths "a" and ".". These paths are represented in the code by "Includers". code Therefore, when paths from strategy 1 and strategy 2 happen to have an identical path, this case can occur. The current implementation of this diagnostic does not return immediately upon the first finding. Its purpose is to check if a header file with the same name exists anywhere within the combined set of paths from strategies 1 and 2. This check avoids false positives that could arise if, for instance, a/b.h is found in strategy 1 (Includers) and the same b.h is found in an "-I a" from strategy 2. |
||
| } | ||
|
|
||
| /// LookupFile - Given a "foo" or \<foo> reference, look up the indicated file, | ||
| /// return null on failure. isAngled indicates whether the file reference is | ||
| /// for system \#include's or not (i.e. using <> instead of ""). Includers, if | ||
|
|
@@ -923,13 +957,18 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| // Otherwise, just return the file. | ||
| return getFileAndSuggestModule(Filename, IncludeLoc, nullptr, | ||
| /*IsSystemHeaderDir*/ false, | ||
| RequestingModule, SuggestedModule, true, | ||
| OpenFile, CacheFailures); | ||
| } | ||
|
|
||
| // This is the header that MSVC's header search would have found. | ||
| bool First = true; | ||
| bool NeedSuggest = true; | ||
| ModuleMap::KnownHeader MSSuggestedModule; | ||
| OptionalFileEntryRef MSFE; | ||
| ModuleMap::KnownHeader FirstModule; | ||
| OptionalFileEntryRef FirstHeader; | ||
| SmallString<1024> FirstDir; | ||
|
|
||
| // Check to see if the file is in the #includer's directory. This cannot be | ||
| // based on CurDir, because each includer could be a #include of a | ||
|
|
@@ -938,7 +977,6 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| // headers. | ||
| if (!Includers.empty() && !isAngled) { | ||
| SmallString<1024> TmpDir; | ||
| for (const auto &IncluderAndDir : Includers) { | ||
| OptionalFileEntryRef Includer = IncluderAndDir.first; | ||
|
|
||
|
|
@@ -962,10 +1000,15 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| }(); | ||
| if (OptionalFileEntryRef FE = getFileAndSuggestModule( | ||
| TmpDir, IncludeLoc, IncluderAndDir.second, IncluderIsSystemHeader, | ||
| RequestingModule, SuggestedModule, NeedSuggest)) { | ||
| NeedSuggest = false; | ||
|
|
||
| if (!Includer) { | ||
| assert(First && "only first includer can have no file"); | ||
| checkAndStoreCandidate( | ||
| SuggestedModule, FE, IncluderAndDir.second.getName(), Diags, | ||
| Filename, IncludeLoc, FirstModule, FirstHeader, FirstDir); | ||
| break; | ||
| } | ||
|
|
||
| // Leave CurDir unset. | ||
|
|
@@ -994,22 +1037,28 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| diagnoseFrameworkInclude(Diags, IncludeLoc, | ||
| IncluderAndDir.second.getName(), Filename, | ||
| *FE); | ||
| checkAndStoreCandidate( | ||
| SuggestedModule, FE, IncluderAndDir.second.getName(), Diags, | ||
| Filename, IncludeLoc, FirstModule, FirstHeader, FirstDir); | ||
| break; | ||
| } | ||
|
|
||
| // Otherwise, we found the path via MSVC header search rules. If | ||
| // -Wmsvc-include is enabled, we have to keep searching to see if we | ||
| // would've found this header in -I or -isystem directories. | ||
| if (checkAndStoreCandidate( | ||
| SuggestedModule, FE, IncluderAndDir.second.getName(), Diags, | ||
| Filename, IncludeLoc, FirstModule, FirstHeader, FirstDir)) { | ||
| // Found mutiple candidates via MSVC rules | ||
| if (Diags.isIgnored(diag::ext_pp_include_search_ms, IncludeLoc)) | ||
| return FirstHeader; | ||
| break; | ||
| } | ||
| MSFE = FE; | ||
| if (SuggestedModule) { | ||
| MSSuggestedModule = *SuggestedModule; | ||
| *SuggestedModule = ModuleMap::KnownHeader(); | ||
| } | ||
| } | ||
| First = false; | ||
| } | ||
|
|
@@ -1069,6 +1118,7 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| } | ||
|
|
||
| SmallString<64> MappedName; | ||
| First = true; | ||
|
|
||
| // Check each directory in sequence to see if it contains this file. | ||
| for (; It != search_dir_end(); ++It) { | ||
|
|
@@ -1078,7 +1128,7 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| OptionalFileEntryRef File = It->LookupFile( | ||
| Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule, | ||
| SuggestedModule, InUserSpecifiedSystemFramework, IsFrameworkFoundInDir, | ||
| IsInHeaderMap, MappedName, NeedSuggest, OpenFile); | ||
| if (!MappedName.empty()) { | ||
| assert(IsInHeaderMap && "MappedName should come from a header map"); | ||
| CacheLookup.MappedName = | ||
|
|
@@ -1097,53 +1147,62 @@ OptionalFileEntryRef HeaderSearch::LookupFile( | |
| if (!File) | ||
| continue; | ||
|
|
||
| if (First) { | ||
| First = false; | ||
| NeedSuggest = false; | ||
| CurDir = It; | ||
| IncludeNames[*File] = Filename; | ||
|
|
||
| // This file is a system header or C++ unfriendly if the dir is. | ||
| HeaderFileInfo &HFI = getFileInfo(*File); | ||
| HFI.DirInfo = CurDir->getDirCharacteristic(); | ||
|
|
||
| // If the directory characteristic is User but this framework was | ||
| // user-specified to be treated as a system framework, promote the | ||
| // characteristic. | ||
| if (HFI.DirInfo == SrcMgr::C_User && InUserSpecifiedSystemFramework) | ||
| HFI.DirInfo = SrcMgr::C_System; | ||
|
|
||
| // If the filename matches a known system header prefix, override | ||
| // whether the file is a system header. | ||
| for (unsigned j = SystemHeaderPrefixes.size(); j; --j) { | ||
| if (Filename.starts_with(SystemHeaderPrefixes[j - 1].first)) { | ||
| HFI.DirInfo = SystemHeaderPrefixes[j - 1].second ? SrcMgr::C_System | ||
| : SrcMgr::C_User; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (checkMSVCHeaderSearch(Diags, MSFE, &File->getFileEntry(), | ||
| IncludeLoc) && | ||
| SuggestedModule) | ||
| *SuggestedModule = MSSuggestedModule; | ||
Jinjie-Huang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| bool FoundByHeaderMap = !IsMapped ? false : *IsMapped; | ||
| if (!Includers.empty()) | ||
| diagnoseFrameworkInclude(Diags, IncludeLoc, | ||
| Includers.front().second.getName(), Filename, | ||
| *File, isAngled, FoundByHeaderMap); | ||
|
|
||
| // Remember this location for the next lookup we do. | ||
| cacheLookupSuccess(CacheLookup, It, IncludeLoc); | ||
| } | ||
|
|
||
| if (checkAndStoreCandidate(SuggestedModule, File, It->getName(), Diags, | ||
| Filename, IncludeLoc, FirstModule, FirstHeader, | ||
| FirstDir)) | ||
| return FirstHeader; | ||
| } | ||
|
|
||
| if (First && checkMSVCHeaderSearch(Diags, MSFE, nullptr, IncludeLoc)) { | ||
Jinjie-Huang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (SuggestedModule) | ||
| *SuggestedModule = MSSuggestedModule; | ||
| return MSFE; | ||
| } | ||
|
|
||
| if (FirstHeader) | ||
| return FirstHeader; | ||
|
|
||
| // Otherwise, didn't find it. Remember we didn't find this. | ||
| CacheLookup.HitIt = search_dir_end(); | ||
| return std::nullopt; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // RUN: rm -rf %t | ||
| // RUN: split-file %s %t | ||
|
|
||
| // RUN: %clang_cc1 -Wheader-shadowing -Eonly %t/main.c -I %t/include 2>&1 | FileCheck %s --check-prefix=SHADOWING | ||
| // SHADOWING: {{.*}} warning: multiple candidates for header 'header.h' found; | ||
|
|
||
| //--- main.c | ||
| #include "header.h" | ||
|
|
||
| //--- include/header.h | ||
| #pragma once | ||
|
|
||
| //--- header.h | ||
| #pragma once |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.