From 68d890da0854b4257c2fc2ab461cd6bfacf05c82 Mon Sep 17 00:00:00 2001 From: Ben Langmuir Date: Fri, 23 May 2014 18:15:47 +0000 Subject: [PATCH] Stopgap fix for finding module for a file mapped in the VFS If we lookup a path using its 'real' path first, we need to ensure that when we run header search we still use the VFS-mapped path or we will not be able to find the corresponding module for the header. The real problem is that we tie the name of a file to its underlying FileEntry, which is uniqued by inode, so we only ever get the first name it is looked up by. This doesn't work with modules, which rely on a specific file system structure. I'm hoping to have time to write up a proposal for fixing this more permanently soon, but as a stopgap this patch updates the name of the file's directory if it comes from a VFS mapping. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@209534 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/FileSystemStatCache.h | 4 ++ include/clang/Basic/VirtualFileSystem.h | 3 + lib/Basic/FileManager.cpp | 10 +++ lib/Basic/FileSystemStatCache.cpp | 1 + lib/Basic/VirtualFileSystem.cpp | 6 +- test/VFS/Inputs/actual_module.map | 4 ++ test/VFS/Inputs/import_some_frame.h | 2 + test/VFS/Inputs/public_header.h | 1 + test/VFS/Inputs/public_header2.h | 1 + test/VFS/Inputs/some_frame_module.map | 5 ++ test/VFS/Inputs/vfsoverlay.yaml | 16 ++++- test/VFS/real-path-found-first.m | 74 +++++++++++++++++++++++ 12 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 test/VFS/Inputs/import_some_frame.h create mode 100644 test/VFS/Inputs/public_header2.h create mode 100644 test/VFS/Inputs/some_frame_module.map create mode 100644 test/VFS/real-path-found-first.m diff --git a/include/clang/Basic/FileSystemStatCache.h b/include/clang/Basic/FileSystemStatCache.h index 2e9528db91..af2535661e 100644 --- a/include/clang/Basic/FileSystemStatCache.h +++ b/include/clang/Basic/FileSystemStatCache.h @@ -38,6 +38,10 @@ struct FileData { bool IsDirectory; bool IsNamedPipe; bool InPCH; + bool IsVFSMapped; // FIXME: remove this when files support multiple names + FileData() + : Size(0), ModTime(0), IsDirectory(false), IsNamedPipe(false), + InPCH(false), IsVFSMapped(false) {} }; /// \brief Abstract interface for introducing a FileManager cache for 'stat' diff --git a/include/clang/Basic/VirtualFileSystem.h b/include/clang/Basic/VirtualFileSystem.h index 978e0408da..0a9949612c 100644 --- a/include/clang/Basic/VirtualFileSystem.h +++ b/include/clang/Basic/VirtualFileSystem.h @@ -39,6 +39,9 @@ class Status { llvm::sys::fs::file_type Type; llvm::sys::fs::perms Perms; +public: + bool IsVFSMapped; // FIXME: remove when files support multiple names + public: Status() : Type(llvm::sys::fs::file_type::status_error) {} Status(const llvm::sys::fs::file_status &Status); diff --git a/lib/Basic/FileManager.cpp b/lib/Basic/FileManager.cpp index 940fcaf417..14731f6b09 100644 --- a/lib/Basic/FileManager.cpp +++ b/lib/Basic/FileManager.cpp @@ -274,6 +274,16 @@ const FileEntry *FileManager::getFile(StringRef Filename, bool openFile, NamedFileEnt.setValue(&UFE); if (UFE.isValid()) { // Already have an entry with this inode, return it. + + // FIXME: this hack ensures that if we look up a file by a virtual path in + // the VFS that the getDir() will have the virtual path, even if we found + // the file by a 'real' path first. This is required in order to find a + // module's structure when its headers/module map are mapped in the VFS. + // We should remove this as soon as we can properly support a file having + // multiple names. + if (DirInfo != UFE.Dir && Data.IsVFSMapped) + UFE.Dir = DirInfo; + // If the stat process opened the file, close it to avoid a FD leak. if (F) delete F; diff --git a/lib/Basic/FileSystemStatCache.cpp b/lib/Basic/FileSystemStatCache.cpp index 44ba48aa2f..0f16e94a05 100644 --- a/lib/Basic/FileSystemStatCache.cpp +++ b/lib/Basic/FileSystemStatCache.cpp @@ -39,6 +39,7 @@ static void copyStatusToFileData(const vfs::Status &Status, Data.IsDirectory = Status.isDirectory(); Data.IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file; Data.InPCH = false; + Data.IsVFSMapped = Status.IsVFSMapped; } /// FileSystemStatCache::get - Get the 'stat' information for the specified diff --git a/lib/Basic/VirtualFileSystem.cpp b/lib/Basic/VirtualFileSystem.cpp index c89b071a77..a469c9abc3 100644 --- a/lib/Basic/VirtualFileSystem.cpp +++ b/lib/Basic/VirtualFileSystem.cpp @@ -31,13 +31,13 @@ using llvm::sys::fs::UniqueID; Status::Status(const file_status &Status) : UID(Status.getUniqueID()), MTime(Status.getLastModificationTime()), User(Status.getUser()), Group(Status.getGroup()), Size(Status.getSize()), - Type(Status.type()), Perms(Status.permissions()) {} + Type(Status.type()), Perms(Status.permissions()), IsVFSMapped(false) {} Status::Status(StringRef Name, StringRef ExternalName, UniqueID UID, sys::TimeValue MTime, uint32_t User, uint32_t Group, uint64_t Size, file_type Type, perms Perms) : Name(Name), UID(UID), MTime(MTime), User(User), Group(Group), Size(Size), - Type(Type), Perms(Perms) {} + Type(Type), Perms(Perms), IsVFSMapped(false) {} bool Status::equivalent(const Status &Other) const { return getUniqueID() == Other.getUniqueID(); @@ -801,6 +801,8 @@ ErrorOr VFSFromYAML::status(const Twine &Path) { assert(!S || S->getName() == F->getExternalContentsPath()); if (S && !F->useExternalName(UseExternalNames)) S->setName(PathStr); + if (S) + S->IsVFSMapped = true; return S; } else { // directory DirectoryEntry *DE = cast(*Result); diff --git a/test/VFS/Inputs/actual_module.map b/test/VFS/Inputs/actual_module.map index 282dac37c7..d2f5b64a38 100644 --- a/test/VFS/Inputs/actual_module.map +++ b/test/VFS/Inputs/actual_module.map @@ -2,3 +2,7 @@ module not_real { header "not_real.h" export * } +module import_some_frame { + header "import_some_frame.h" + export * +} diff --git a/test/VFS/Inputs/import_some_frame.h b/test/VFS/Inputs/import_some_frame.h new file mode 100644 index 0000000000..c1f68c83fc --- /dev/null +++ b/test/VFS/Inputs/import_some_frame.h @@ -0,0 +1,2 @@ +#import +#import diff --git a/test/VFS/Inputs/public_header.h b/test/VFS/Inputs/public_header.h index 471107762b..09d9969d31 100644 --- a/test/VFS/Inputs/public_header.h +++ b/test/VFS/Inputs/public_header.h @@ -1 +1,2 @@ +#import void from_framework(void); diff --git a/test/VFS/Inputs/public_header2.h b/test/VFS/Inputs/public_header2.h new file mode 100644 index 0000000000..d883613ac1 --- /dev/null +++ b/test/VFS/Inputs/public_header2.h @@ -0,0 +1 @@ +// public_header2.h diff --git a/test/VFS/Inputs/some_frame_module.map b/test/VFS/Inputs/some_frame_module.map new file mode 100644 index 0000000000..3ce59b254d --- /dev/null +++ b/test/VFS/Inputs/some_frame_module.map @@ -0,0 +1,5 @@ +framework module SomeFramework { + umbrella header "public_header.h" + export * + module * { export * } +} diff --git a/test/VFS/Inputs/vfsoverlay.yaml b/test/VFS/Inputs/vfsoverlay.yaml index 5c1380870d..0aa8cd619a 100644 --- a/test/VFS/Inputs/vfsoverlay.yaml +++ b/test/VFS/Inputs/vfsoverlay.yaml @@ -6,14 +6,26 @@ { 'name': 'not_real.h', 'type': 'file', 'external-contents': 'INPUT_DIR/actual_header.h' }, + { 'name': 'import_some_frame.h', 'type': 'file', + 'external-contents': 'INPUT_DIR/import_some_frame.h' + }, { 'name': 'module.map', 'type': 'file', 'external-contents': 'INPUT_DIR/actual_module.map' }, { 'name': 'include_real.h', 'type': 'file', 'external-contents': 'INPUT_DIR/include_real.h' }, - { 'name': 'SomeFramework.framework/Headers/public_header.h', 'type': 'file', - 'external-contents': 'INPUT_DIR/public_header.h' + { 'name': 'SomeFramework.framework', 'type': 'directory', + 'contents': [ + { 'name': 'Headers', 'type': 'directory', + 'contents': [ + { 'name': 'public_header.h', 'type': 'file', + 'external-contents': 'INPUT_DIR/public_header.h' }, + { 'name': 'public_header2.h', 'type': 'file', + 'external-contents': 'INPUT_DIR/public_header2.h' } + ] + } + ] }, { 'name': 'Foo.framework/Headers/Foo.h', 'type': 'file', 'external-contents': 'INPUT_DIR/Foo.h' diff --git a/test/VFS/real-path-found-first.m b/test/VFS/real-path-found-first.m new file mode 100644 index 0000000000..f494c6eb15 --- /dev/null +++ b/test/VFS/real-path-found-first.m @@ -0,0 +1,74 @@ +// This test is for cases where we lookup a file by its 'real' path before we +// use its VFS-mapped path. If we accidentally use the real path in header +// search, we will not find a module for the headers. To test that we +// intentionally rebuild modules, since the precompiled module file refers to +// the dependency files by real path. + +// REQUIRES: shell +// RUN: rm -rf %t %t-cache %t.pch +// RUN: mkdir -p %t/SomeFramework.framework/Modules +// RUN: cp %S/Inputs/some_frame_module.map %t/SomeFramework.framework/Modules/module.modulemap +// RUN: sed -e "s:INPUT_DIR:%S/Inputs:g" -e "s:OUT_DIR:%t:g" %S/Inputs/vfsoverlay.yaml > %t.yaml + +// Build +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -fsyntax-only %s -verify -Wauto-import \ +// RUN: -Werror=non-modular-include-in-framework-module + +// Rebuild +// RUN: echo ' ' >> %t/SomeFramework.framework/Modules/module.modulemap +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -fsyntax-only %s -verify -Wauto-import \ +// RUN: -Werror=non-modular-include-in-framework-module + +// Load from PCH +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -emit-pch %s -o %t.pch \ +// RUN: -Werror=non-modular-include-in-framework-module \ +// RUN: -fmodules-ignore-macro=WITH_PREFIX +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -include-pch %t.pch -fsyntax-only %s \ +// RUN: -Werror=non-modular-include-in-framework-module -DWITH_PREFIX \ +// RUN: -fmodules-ignore-macro=WITH_PREFIX + +// While indexing +// RUN: c-index-test -index-file %s -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -fsyntax-only -Wauto-import \ +// RUN: -Werror=non-modular-include-in-framework-module | FileCheck %s +// RUN: echo ' ' >> %t/SomeFramework.framework/Modules/module.modulemap +// RUN: c-index-test -index-file %s -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -fsyntax-only -Wauto-import \ +// RUN: -Werror=non-modular-include-in-framework-module | FileCheck %s +// CHECK: warning: treating +// CHECK-NOT: error + +// With a VFS-mapped module map file +// RUN: mv %t/SomeFramework.framework/Modules/module.modulemap %t/hide_module.map +// RUN: echo "{ 'version': 0, 'roots': [ { " > %t2.yaml +// RUN: echo "'name': '%t/SomeFramework.framework/Modules/module.modulemap'," >> %t2.yaml +// RUN: echo "'type': 'file', 'external-contents': '%t/hide_module.map' } ] }" >> %t2.yaml + +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -fsyntax-only %s -verify \ +// RUN: -Wauto-import -Werror=non-modular-include-in-framework-module +// RUN: echo ' ' >> %t/hide_module.map +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -fsyntax-only %s -verify \ +// RUN: -Wauto-import -Werror=non-modular-include-in-framework-module + +// Within a module build +// RUN: echo '@import import_some_frame;' | \ +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -fsyntax-only - \ +// RUN: -Werror=non-modular-include-in-framework-module -x objective-c -I %t +// RUN: echo ' ' >> %t/hide_module.map +// RUN: echo '@import import_some_frame;' | \ +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t-cache -F %t \ +// RUN: -ivfsoverlay %t.yaml -ivfsoverlay %t2.yaml -fsyntax-only - \ +// RUN: -Werror=non-modular-include-in-framework-module -x objective-c -I %t + +#ifndef WITH_PREFIX +#import // expected-warning{{treating}} +#import // expected-warning{{treating}} +@import SomeFramework.public_header2; +#endif -- 2.40.0