bnbarham updated this revision to Diff 420678.
bnbarham added a comment.
Added a potential plan to remove the FileManager hacks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123104/new/
https://reviews.llvm.org/D123104
Files:
clang/include/clang/Lex/HeaderSearch.h
clang/lib/Basic/FileManager.cpp
clang/lib/Lex/HeaderSearch.cpp
clang/test/Modules/Inputs/all-product-headers.yaml
clang/test/Modules/modulemap-collision.m
Index: clang/test/Modules/modulemap-collision.m
===================================================================
--- /dev/null
+++ 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 <A/A.h>
Index: clang/test/Modules/Inputs/all-product-headers.yaml
===================================================================
--- /dev/null
+++ 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"
+ }
+ ]
+ }
+ ]
+}
Index: clang/lib/Lex/HeaderSearch.cpp
===================================================================
--- clang/lib/Lex/HeaderSearch.cpp
+++ 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<StringRef> 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;
Index: clang/lib/Basic/FileManager.cpp
===================================================================
--- clang/lib/Basic/FileManager.cpp
+++ 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)})
Index: clang/include/clang/Lex/HeaderSearch.h
===================================================================
--- clang/include/clang/Lex/HeaderSearch.h
+++ 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<StringRef> OverrideFilename = None);
/// Find and suggest a usable module for the given file, which is part of
/// the specified framework.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
