diff --git a/clang/include/clang/Lex/HeaderSearch.h b/clang/include/clang/Lex/HeaderSearch.h --- a/clang/include/clang/Lex/HeaderSearch.h +++ b/clang/include/clang/Lex/HeaderSearch.h @@ -748,13 +748,19 @@ /// Find and suggest a usable module for the given file. /// - /// \return \c true if the file can be used, \c false if we are not permitted to - /// find this file due to requirements from \p RequestingModule. + /// \param OverrideFilename see FIXME in the implementation. This shouldn't + /// differ from the name provided by \p File, but due to various hacks in + /// \c FileManager, may end up doing so. We need to make sure the name is as + /// *requested*, so pass it in separately for now. + /// + /// \return \c true if the file can be used, \c false if we are not permitted + /// to find this file due to requirements from \p RequestingModule. bool findUsableModuleForHeader(const FileEntry *File, const DirectoryEntry *Root, Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule, - bool IsSystemHeaderDir); + bool IsSystemHeaderDir, + Optional OverrideFilename = None); /// Find and suggest a usable module for the given file, which is part of /// the specified framework. diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp --- a/clang/lib/Basic/FileManager.cpp +++ b/clang/lib/Basic/FileManager.cpp @@ -287,11 +287,48 @@ // name to users (in diagnostics) and to tools that don't have access to // the VFS (in debug info and dependency '.d' files). // - // FIXME: This is pretty complicated. It's also inconsistent with how - // "real" filesystems behave and confuses parts of clang expect to see the - // name-as-accessed on the \a FileEntryRef. Maybe the returned \a - // FileEntryRef::getName() could return the accessed name unmodified, but - // make the external name available via a separate API. + // FIXME: This is pretty complex and has some very complicated interactions + // with the rest of clang. It's also inconsistent with how "real" + // filesystems behave and confuses parts of clang expect to see the + // name-as-accessed on the \a FileEntryRef. + // + // Further, it isn't *just* external names, but will also give back absolute + // paths when a relative path was requested - the check is comparing the + // name from the status, which is passed an absolute path resolved from the + // current working directory. `clang-apply-replacements` appears to depend + // on this behaviour, though it's adjusting the working directory, which is + // definitely not supported. Once that's fixed this hack should be able to + // be narrowed to only when there's an externally mapped name given back. + // + // A potential plan to remove this is as follows - + // - Add API to determine if the name has been rewritten by the VFS. + // - Fix `clang-apply-replacements` to pass down the absolute path rather + // than changing the CWD. Narrow this hack down to just externally + // mapped paths. + // - Expose the requested filename. One possibility would be to allow + // redirection-FileEntryRefs to be returned, rather than returning + // the pointed-at-FileEntryRef, and customizing `getName()` to look + // through the indirection. + // - Update callers such as `HeaderSearch::findUsableModuleForHeader()` + // to explicitly use the requested filename rather than just using + // `getName()`. + // - Add a `FileManager::getExternalPath` API for explicitly getting the + // remapped external filename when there is one available. Adopt it in + // callers like diagnostics/deps reporting instead of calling + // `getName()` directly. + // - Switch the meaning of `FileEntryRef::getName()` to get the requested + // name, not the external name. Once that sticks, revert callers that + // want the requested name back to calling `getName()`. + // - Update the VFS to always return the requested name. This could also + // return the external name, or just have an API to request it + // lazily. The latter has the benefit of making accesses of the + // external path easily tracked, but may also require extra work than + // just returning up front. + // - (Optionally) Add an API to VFS to get the external filename lazily + // and update `FileManager::getExternalPath()` to use it instead. This + // has the benefit of making such accesses easily tracked, though isn't + // necessarily required (and could cause extra work than just adding to + // eg. `vfs::Status` up front). auto &Redirection = *SeenFileEntries .insert({Status.getName(), FileEntryRef::MapValue(*UFE, DirInfo)}) diff --git a/clang/lib/Lex/HeaderSearch.cpp b/clang/lib/Lex/HeaderSearch.cpp --- a/clang/lib/Lex/HeaderSearch.cpp +++ b/clang/lib/Lex/HeaderSearch.cpp @@ -415,7 +415,7 @@ // If there is a module that corresponds to this header, suggest it. if (!findUsableModuleForHeader( &File->getFileEntry(), Dir ? Dir : File->getFileEntry().getDir(), - RequestingModule, SuggestedModule, IsSystemHeaderDir)) + RequestingModule, SuggestedModule, IsSystemHeaderDir, FileName)) return None; return *File; @@ -1563,10 +1563,20 @@ bool HeaderSearch::findUsableModuleForHeader( const FileEntry *File, const DirectoryEntry *Root, Module *RequestingModule, - ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) { + ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir, + Optional OverrideFilename) { if (File && needModuleLookup(RequestingModule, SuggestedModule)) { // If there is a module that corresponds to this header, suggest it. - hasModuleMap(File->getName(), Root, IsSystemHeaderDir); + // + // FIXME: File->getName() *should* be the same as FileName, but because + // of the VFS and various hacks in FileManager, that's not necessarily the + // case. See the FIXME in `FileManager::getFileRef` for a plan to remove + // this case. The eventual goal is to be able to use + // `FileEntryRef::getName`, though it might take a few steps to get there. + StringRef Filename = File->getName(); + if (OverrideFilename) + Filename = *OverrideFilename; + hasModuleMap(Filename, Root, IsSystemHeaderDir); return suggestModule(*this, File, RequestingModule, SuggestedModule); } return true; diff --git a/clang/test/Modules/Inputs/all-product-headers.yaml b/clang/test/Modules/Inputs/all-product-headers.yaml new file mode 100644 --- /dev/null +++ b/clang/test/Modules/Inputs/all-product-headers.yaml @@ -0,0 +1,33 @@ +{ + 'version': 0, + 'case-sensitive': 'false', + 'roots': [ + { + 'type': 'directory', + 'name': "DUMMY_DIR/build/A.framework/PrivateHeaders" + 'contents': [ + { + 'type': 'file', + 'name': "A.h", + 'external-contents': "DUMMY_DIR/sources/A.h" + } + ] + }, + { + 'type': 'directory', + 'name': "DUMMY_DIR/build/A.framework/Modules" + 'contents': [ + { + 'type': 'file', + 'name': "module.modulemap", + 'external-contents': "DUMMY_DIR/build/module.modulemap" + }, + { + 'type': 'file', + 'name': "module.private.modulemap", + 'external-contents': "DUMMY_DIR/build/module.private.modulemap" + } + ] + } + ] +} diff --git a/clang/test/Modules/modulemap-collision.m b/clang/test/Modules/modulemap-collision.m new file mode 100644 --- /dev/null +++ b/clang/test/Modules/modulemap-collision.m @@ -0,0 +1,15 @@ +// REQUIRES: shell + +// RUN: rm -rf %t +// RUN: mkdir -p %t/sources %t/build +// RUN: echo "// A.h" > %t/sources/A.h +// RUN: echo "framework module A {}" > %t/sources/module.modulemap +// RUN: echo "framework module A.Private { umbrella header \"A.h\" }" > %t/sources/module.private.modulemap +// RUN: cp %t/sources/module.modulemap %t/build/module.modulemap +// RUN: cp %t/sources/module.private.modulemap %t/build/module.private.modulemap + +// RUN: sed -e "s:DUMMY_DIR:%t:g" %S/Inputs/all-product-headers.yaml > %t/build/all-product-headers.yaml +// RUN: %clang_cc1 -fsyntax-only -ivfsoverlay %t/build/all-product-headers.yaml -F%t/build -fmodules -fimplicit-module-maps -fmodules-cache-path=tmp -x objective-c %s -verify + +// expected-no-diagnostics +#import