[modules] Avoid module relocation error when building modules from VFS

[bruno]

Summary

The reproducer script (generated by the crash reproducer) invokes clang
and tries to rebuild modules using the headers in the .cache/vfs
directory. Depending on the order that modules are constructed it is
possible that there's a mismatch between the directory that a module was
constructed and the path that it was found:

fatal error: module '_Builtin_stddef_max_align_t' was built in directory
'/Users/bruno/Dev/install/clang/bin/../lib/clang/<ver>/include'
but now resides in directory
'/<path_to>.cache/vfs/Users/bruno/Dev/install/clang/lib/clang/<ver>/include'

This happens because getFile() can leak the original path depending on
the way it's accessed. This seems to be known issue and one that demands
a lot of work to get rid of, according to:

[cfe-dev] Module maps, __FILE__ and lazy stat'ing of header
http://lists.llvm.org/pipermail/cfe-dev/2014-July/038273.html

I tried a couple of fixes to the issue but this is the less invasive I
could come up with. This fixes the issue by first looking into modules
by using virtual dir paths instead of the real ones. Let me know if
there's a better solution or whether I'm missing something.

With this change on Darwin we are able to simulate a crash for a simple
application using "Foundation/Foundation.h" (which relies on a bunch of
different frameworks and headers) and successfully rebuild all the
modules by relying solely at the VFS overlay.

[bruno]

Ping!

[benlangmuir]

This doesn't appear to be a safe change. We can't assume that RootName wouldn't ever be legitimately found somewhere in a path (even without a VFS) and changing the path prefix could give a completely different location. I would also be concerned about the performance impact of performing two lookups, since finding a module for a header is not always a cheap operation.

I'm not sure off hand how else to fix this.

[rsmith]

Can you find who's poisoning the FileManager's idea of the name of the file with a full path to the VFS overlay and fix that?

lib/Lex/HeaderSearch.cpp

1164

remove_dot_dot=true is not a correct transformation to do to a path and can result in the wrong file being found.

[bruno]

Hi Richard & Ben,

Thanks for the review!

I've attached a new patch and changed the approach: made findUsableModuleForHeader receive a FileName
instead of FileEntry. Answering the questions below:

FileManager::getFile returns the same filepath when a file is first requested and found. However,
for cached searches it returns the real path to the file. In the scenario of the testcase, the leak
happens when HeaderSearch::getFileAndSuggestModule calls getFileMgr().getFile for bar.h, prior to
calling findUsableModuleForHeader. Since bar.h is already cached, the real path is passed to findUsableModuleForHeader,
which triggers down the road the module redefinition error.

From what I understand from http://lists.llvm.org/pipermail/cfe-dev/2014-July/038273.html, changing
the behavior of the cached result has side-effects and demands a non trivial change, am I assuming
the right thing?

[benlangmuir]

From what I understand from http://lists.llvm.org/pipermail/cfe-dev/2014-July/038273.html, changing the behavior of the cached result has side-effects and demands a non trivial change, am I assuming the right thing?

Yes.

Threading through the Filename seems fine to me. Specific comments below.

lib/Basic/FileManager.cpp

221–223

The first time we lookup a file, we will return whatever name comes back from the VFS, which may or may not be the requested path. If this is a real file system, it will be the requested path. If it is a redirecting file system and "use-external-names" is true (which it is by default), then the VFS will return the external contents path, which is generally not the same as the requested path.

lib/Lex/HeaderSearch.cpp

493

What does this change do? FE->getDir()->getName() will be the virtual path if there is VFS, which is usually the one you want to operate on.

[bruno]

Hi Ben,

lib/Basic/FileManager.cpp

221–223

The behaviour I'm seeing for VFS is a bit different: If it is a redirecting file system and "use-external-names" is true, then the FileManager returns the requested file path. Sub-sequential cached queries return the "use-external-names" / real-path instead. This is the rationale why I'm trying to thread the FileName instead of the FileEntry. Am I missing something?

lib/Lex/HeaderSearch.cpp

493

You're right! I assumed FE->getDir()->getName() would give me the real path directory for the file in question, but I double checked here and that doesn't happen.

[benlangmuir]

lib/Basic/FileManager.cpp

221–223

That's not supposed to happen. You can see below that we always use the name from the StatCache, which in turn comes from the the VFS status() result. Have you traced through what's happening here? We should figure out how we got the requested path as the name.

[bruno]

Hi Ben,

lib/Basic/FileManager.cpp

221–223

I double checked here and you're right, it actually always return the real path for the first request *and* the cached one when "use-external-names" == true. I was misguided by the fact that other places are using the virtual FileName instead of the one in FileEntry, this further lead to inconsistent paths. I'm abandoning this patch since it does not make sense for the crash reproducer, using "use-external-names" = false is the right thing there.

include/clang/Lex/HeaderSearch.h

private:
   /// \brief Load all of the module maps within the immediate subdirectories
   /// of the given search directory.
   void loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir);
 
   /// \brief 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.
-  bool findUsableModuleForHeader(const FileEntry *File,
-                                 const DirectoryEntry *Root,
+  bool findUsableModuleForHeader(StringRef FileName, const DirectoryEntry *Root,
                                  Module *RequestingModule,
                                  ModuleMap::KnownHeader *SuggestedModule,
                                  bool IsSystemHeaderDir);
 
   /// \brief Find and suggest a usable module for the given file, which is part of
   /// the specified framework.
   ///
   /// \return \c true if the file can be used, \c false if we are not permitted to

lib/Basic/FileManager.cpp

const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
                                       bool CacheFailure) {
   ++NumFileLookups;
 
   // See if there is already an entry in the map.
   auto &NamedFileEnt =
       *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
 
   // See if there is already an entry in the map.
+  // FIXME: Note that when first requested, the returned file name equals to the
+  // requested path. This is not true when returning a cached result. This is
+  // inconsistent and might lead to clients making the wrong assumptions.
   if (NamedFileEnt.second)
     return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
                                                     : NamedFileEnt.second;
 
   ++NumFileCacheMisses;
 
   // By default, initialize it to invalid.
   NamedFileEnt.second = NON_EXISTENT_FILE;

lib/Lex/HeaderSearch.cpp

const FileEntry *HeaderSearch::getFileAndSuggestModule(
     Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule) {
   // If we have a module map that might map this header, load it and
   // check whether we'll have a suggestion for a module.
   const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
   if (!File)
     return nullptr;
 
   // If there is a module that corresponds to this header, suggest it.
-  if (!findUsableModuleForHeader(File, Dir ? Dir : File->getDir(),
+  if (!findUsableModuleForHeader(FileName, Dir ? Dir : File->getDir(),
                                  RequestingModule, SuggestedModule,
                                  IsSystemHeaderDir))
     return nullptr;
 
   return File;
 }
 
 /// LookupFile - Lookup the specified file in this search path, returning it
if (SearchPath)
                          Private+strlen(Private));
 
     FE = FileMgr.getFile(FrameworkName, /*openFile=*/!SuggestedModule);
   }
 
   // If we found the header and are allowed to suggest a module, do so now.
   if (FE && SuggestedModule) {
     // Find the framework in which this header occurs.
-    StringRef FrameworkPath = FE->getDir()->getName();
+    StringRef FrameworkPath = llvm::sys::path::parent_path(FrameworkName);
     bool FoundFramework = false;
     do {
       // Determine whether this directory exists.
       const DirectoryEntry *Dir = FileMgr.getDirectory(FrameworkPath);
       if (!Dir)
         break;
 
       // If this is a framework directory, then we're a subframework of this
if (FE && SuggestedModule) {
     } while (true);
 
     bool IsSystem = getDirCharacteristic() != SrcMgr::C_User;
     if (FoundFramework) {
       if (!HS.findUsableModuleForFrameworkHeader(
               FE, FrameworkPath, RequestingModule, SuggestedModule, IsSystem))
         return nullptr;
     } else {
-      if (!HS.findUsableModuleForHeader(FE, getDir(), RequestingModule,
-                                        SuggestedModule, IsSystem))
+      if (!HS.findUsableModuleForHeader(FrameworkName, getDir(),
+                                        RequestingModule, SuggestedModule,
+                                        IsSystem))
         return nullptr;
     }
   }
   return FE;
 }
 
 void HeaderSearch::setTarget(const TargetInfo &Target) {
   ModMap.setTarget(Target);
if (ExternalSource) {
     // Make sure the external source has handled header info about this file,
     // which includes whether the file is part of a module.
     (void)getExistingFileInfo(File);
   }
   return ModMap.findModuleForHeader(File);
 }
 
 bool HeaderSearch::findUsableModuleForHeader(
-    const FileEntry *File, const DirectoryEntry *Root, Module *RequestingModule,
+    StringRef FileName, const DirectoryEntry *Root, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) {
+  const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
   if (File && SuggestedModule) {
     // If there is a module that corresponds to this header, suggest it.
-    hasModuleMap(File->getName(), Root, IsSystemHeaderDir);
+    // FIXME: Look module through the filename to avoid FileManager leaking
+    // real paths when using the VFS...
+    hasModuleMap(FileName, Root, IsSystemHeaderDir);
     *SuggestedModule = findModuleForHeader(File);
   }
   return true;
 }
 
 bool HeaderSearch::findUsableModuleForFrameworkHeader(
     const FileEntry *File, StringRef FrameworkName, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule, bool IsSystemFramework) {
   // If we're supposed to suggest a module, look for one now.
   if (SuggestedModule) {
     // Find the top-level framework based on this framework.
     SmallVector<std::string, 4> SubmodulePath;
     const DirectoryEntry *TopFrameworkDir
       = ::getTopFrameworkDir(FileMgr, FrameworkName, SubmodulePath);
     
     // Determine the name of the top-level framework.

test/Modules/Inputs/crash-recovery/Users/bar/a.h

+#ifndef _BAR_H
+#define _BAR_H
+
+#include "bar.h"
+
+#endif

test/Modules/Inputs/crash-recovery/Users/bar/bar.h

+#ifndef __CLANG_MAX_ALIGN_T_DEFINED
+#define __CLANG_MAX_ALIGN_T_DEFINED
+
+typedef long double max_align_t;
+
+#endif

test/Modules/Inputs/crash-recovery/Users/bar/module.modulemap

+module bar [system] [extern_c] {
+  header "bar.h"
+}

test/Modules/Inputs/crash-recovery/Users/foo/foo.h

+#include <a.h>

test/Modules/Inputs/crash-recovery/Users/foo/module.modulemap

+module foo [system] [extern_c] {
+  header "foo.h"
+}

test/Modules/crash-vfs-module-redefinition.m

+// REQUIRES: crash-recovery, system-darwin, shell
+
+// FIXME: This XFAIL is cargo-culted from crash-report.c. Do we need it?
+// XFAIL: mingw32
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/i %t/m %t
+
+// RUN: not env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
+// RUN: %clang -fsyntax-only -nostdinc %s \
+// RUN:     -I %S/Inputs/crash-recovery/Users/bar \
+// RUN:     -I %S/Inputs/crash-recovery/Users -isysroot %/t/i/ \
+// RUN:     -fmodules -fmodules-cache-path=%t/m/ 2>&1 | FileCheck %s
+
+// RUN: FileCheck --check-prefix=CHECKSRC %s -input-file %t/crash-vfs-*.m
+// RUN: FileCheck --check-prefix=CHECKSH %s -input-file %t/crash-vfs-*.sh
+
+#include <foo/foo.h>
+
+// CHECK: Preprocessed source(s) and associated run script(s) are located at:
+// CHECK-NEXT: note: diagnostic msg: {{.*}}.m
+// CHECK-NEXT: note: diagnostic msg: {{.*}}.cache
+
+// CHECKSRC: @import foo;
+
+// CHECKSH: # Crash reproducer
+// CHECKSH-NEXT: # Driver args: "-fsyntax-only"
+// CHECKSH-NEXT: # Original command: {{.*$}}
+// CHECKSH-NEXT: "-cc1"
+// CHECKSH: "-resource-dir"
+// CHECKSH: "-isysroot" "{{[^"]*}}/i/"
+// CHECKSH: "crash-vfs-{{[^ ]*}}.m"
+// CHECKSH: "-ivfsoverlay" "crash-vfs-{{[^ ]*}}.cache/vfs/vfs.yaml"
+// CHECKSH: "-fmodules-cache-path=crash-vfs-{{[^ ]*}}.cache/modules"
+
+// RUN: cd %t
+// RUN: chmod 755 crash-vfs-*.sh
+// RUN: ./crash-vfs-*.sh