[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

1163

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 ↗

(On Diff #53363)

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 ↗

(On Diff #53363)

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 ↗

(On Diff #53363)

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 ↗

(On Diff #53363)

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.

lib/Lex/HeaderSearch.cpp

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();
     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 (ExternalSource) {
     (void)getExistingFileInfo(File);
   }
   return ModMap.findModuleForHeader(File);
 }
 
 bool HeaderSearch::findUsableModuleForHeader(
     const FileEntry *File, const DirectoryEntry *Root, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) {
+  using namespace llvm::sys;
   if (File && SuggestedModule) {
+    // FIXME: This is a workaround on a limitation: the file manager leaks
+    // original paths for virtual file requests. This leads to modules being
+    // inconsistently built with virtual and searched through real paths, or
+    // vice-versa, which ends up in module relocation errors. Fix that by
+    // trying to find the module map through the virtual one first. Since
+    // real paths that go through the VFS are canonicalized to do not contain
+    // path traversal components, take that into account.
+    SmallVector<std::string, 2> FilePaths;
+
+    StringRef FileName = File->getName();
+    SmallString<256> RootName = StringRef(Root->getName());
+    path::remove_dots(RootName, true/*remove_dot_dot*/);
+    auto Pos = FileName.find(RootName);
+    if (path::is_absolute(FileName) && Pos != StringRef::npos && Pos > 0) {
+      StringRef Suffix = FileName.drop_front(Pos + RootName.str().size());
+      SmallString<256> NewFileName = StringRef(Root->getName());
+      path::append(NewFileName, Suffix);
+      FilePaths.push_back(NewFileName.str());
+    }
+    FilePaths.push_back(FileName.str());
+
     // If there is a module that corresponds to this header, suggest it.
-    hasModuleMap(File->getName(), Root, IsSystemHeaderDir);
+    for (auto &F : FilePaths) {
+      hasModuleMap(F, Root, IsSystemHeaderDir);
       *SuggestedModule = findModuleForHeader(File);
+      if (SuggestedModule)
+        break;
+    }
   }
   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.

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