[Modules] Use looked-up filename when looking for module maps

[bnbarham]

Summary

Prevent possible modulemap collisions by making sure to always use the
looked-up filename, regardless of any possible overlays.

Upstreamed from apple#llvm-project
72cf785051fb1b3ef22eee4dd33366e41a275981. As well as preventing the
collisions as above, it also highlighted an issue with the recent change
to narrow the FileManager hack of setting the DirectoryEntry in
FileEntry to the most recently looked up directory
(3fda0edc51fd68192a30e302d45db081bb02d7f9). Specifically, it would cause
the incorrect path to be used in DoFrameworkLookup in a crash
reproducer:

• Crash reproducers have use-external-names set to false
• File->getFileEntry().getDir()->getName() would then be the *cached* path, not the just looked up one
• crash-vfs-umbrella-frameworks.m fails with this change since the correct path is now looked up and causes B to be loaded twice

[dexonsmith]

This LGTM, with a couple of comments (on the comments!) inline.

Upstreamed from apple#llvm-project 72cf785051fb1b3ef22eee4dd33366e41a275981.

Note, for some extra background:

• This was out-of-tree because it seemed "hacky". The thinking at the time (2017!) was that we should try to get a "clean" fix for upstream, which is what FileEntryRef is on the path toward...
• ... but FileEntryRef is blocked because of hard-to-reason-about failures related to modules (see, for example, my reverted patch at https://reviews.llvm.org/D92975).
• In the meantime, the in-tree behaviour is wrong, and it doesn't make sense to just leave it broken upstream indefinitely... and keeping it downstream is making it harder to wrap our heads around the full problem.
• In discussing the intersection of this with the fix @bnbarham is working on (reverted in https://reviews.llvm.org/D123103 after this test failed in downstream integration), we've come up with a more incremental way of pushing FileEntryRef forward... hopefully incremental enough that we'll be unblocked and can finish it off. That's what I want documented in the FileManager FIXME as part of this patch.

clang/lib/Lex/HeaderSearch.cpp

1570–1579

Nit: I'd very slightly prefer the two paragraphs be part of the same comment (put a // here for the blank line), but if you'd prefer it like this it's fine.

1571–1572

Can you also expand the "redirection" FIXME in FileManager.cpp (or add an appropriate one)?

I think the steps are something like:

• Add API to determine if the name has been rewritten by the VFS (that's the patch you're working on).
• Expose the requested filename for adoption. I think the way to implement this is to allow "redirection FileEntryRef"s to be returned, rather than returning the pointed-at-FileEntryRef, and customizing getName() to see through the indirection.
• Update callers such as HeaderSearch::findUsableModuleForHeader() (comment should mention this function specifically as an example I think!) to explicitly get the requested filename rather than using getName().
• Add a FileManager::getExternalPath API for explicitly getting the remapped external filename when there is one available, and 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().
• Add an API to VFS to get the external filename lazily, stop doing it up front, and update FileManager::getExternalPath to do the right thing. (This will stop creating redirection entries for those cases.)

The need to expose the requested-name explicitly as a first step is a new insight we had when discussing this offline. Overall I think the plan for unwinding these hacks is under-documented; since we've spent a good deal of time on this, I'd like to record what we know so anyone new looking at FileManager::getFileRef() can guess how to help out (and better understand any odd behaviour they're seeing). Feel free to trim down and/or expand and/or completely rewrite, as long as the comment in FileManager encapsulates your understanding of how we can incrementally unwind all these hacks.

[dexonsmith]

(Forgot to click "accept".)

[benlangmuir]

clang/include/clang/Lex/HeaderSearch.h

763

This parameter could use a comment, even if it just points you to read the FIXME in the implementation.

[dexonsmith]

clang/include/clang/Lex/HeaderSearch.h

763

Good point; maybe it could have a better name/type, too, something like:

Optional<StringRef> OverrideModuleMapFilename = None

[bnbarham]

clang/include/clang/Lex/HeaderSearch.h

763

I believe this is actually the header name, but Optional + OverrideFilename seems worthwhile to me.

[bnbarham]

Added a potential plan to remove the FileManager hacks.

[bnbarham]

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

[dexonsmith]

In D123104#3439261, @bnbarham wrote:

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

Capturing clang -check error since the build log won't be available forever:

error: 'warning' diagnostics seen but not expected: 

  File /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/modulemap-collision.m.tmp/build/module.private.modulemap Line 1: private submodule 'A.Private' in private module map, expected top-level module

error: 'note' diagnostics seen but not expected: 

  File /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/modulemap-collision.m.tmp/build/module.private.modulemap Line 1: rename 'A.Private' to ensure it can be found by name

I wonder if https://github.com/apple/llvm-project/commit/67c70038bcc0d771e2e39c875ad7d69e329c7fc4 could be related. A long shot (I can't see how that could be it!), but the other not-upstreamed patches look even less relevant.

clang/test/Modules/modulemap-collision.m

12

Just noticed -fmodules-cache-path=tmp, which seems like it should be =%t/cache instead... but I doubt that's the cause of the failure.

[dexonsmith]

In D123104#3443448, @dexonsmith wrote:

I wonder if https://github.com/apple/llvm-project/commit/67c70038bcc0d771e2e39c875ad7d69e329c7fc4 could be related. A long shot (I can't see how that could be it!), but the other not-upstreamed patches look even less relevant.

In case it is related, note the thread ending at https://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170807/200915.html, which explains why it was reverted from upstream in 4164dd9167f2f049e2a5e1bd3970a6bb45889462 and describes an alternative fix. @Bigcheese @jansvoboda11 @vsapsai, perhaps something to be aware of.

clang/include/clang/Lex/HeaderSearch.h

Module *loadFrameworkModule(StringRef Name,
                               bool IsSystem);
 
   /// Load all of the module maps within the immediate subdirectories
   /// of the given search directory.
   void loadSubdirectoryModuleMaps(DirectoryLookup &SearchDir);
 
   /// 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.
   ///
   /// \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 findUsableModuleForFrameworkHeader(
       const FileEntry *File, StringRef FrameworkName, Module *RequestingModule,

clang/lib/Basic/FileManager.cpp

if (Status.getName() == Filename) {
     // to return.
     //
     // This redirection logic intentionally leaks the external name of a
     // redirected file that uses 'use-external-name' in \a
     // vfs::RedirectionFileSystem. This allows clang to report the external
     // 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)})
              .first;
     assert(Redirection.second->V.is<FileEntry *>() &&
            "filename redirected to a non-canonical filename?");
     assert(Redirection.second->V.get<FileEntry *>() == UFE &&
            "filename from getStatValue() refers to wrong file");

clang/lib/Lex/HeaderSearch.cpp

if (EC != llvm::errc::no_such_file_or_directory &&
           << FileName << EC.message();
     }
     return None;
   }
 
   // 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;
 }
 
 /// LookupFile - Lookup the specified file in this search path, returning it
 /// if it exists or returning null if not.
 Optional<FileEntryRef> DirectoryLookup::LookupFile(
*SuggestedModule = (Module.getRole() & ModuleMap::TextualHeader)
                            ? ModuleMap::KnownHeader()
                            : Module;
 
   return true;
 }
 
 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;
 }
 
 bool HeaderSearch::findUsableModuleForFrameworkHeader(
     const FileEntry *File, StringRef FrameworkName, Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule, bool IsSystemFramework) {

clang/test/Modules/Inputs/all-product-headers.yaml

+{
+  '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"
+        }
+      ]
+    }
+  ]
+}

clang/test/Modules/modulemap-collision.m

+// 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>