[clang] Use `{File,Directory}EntryRef` in modular header search (part 2/2)

[jansvoboda11]

Summary

This patch removes some deprecated uses of {File,Directory}Entry::getName(). No functional change intended.

Depends on D151854.

[jansvoboda11]

Rebase, reformat

[jansvoboda11]

I noticed the Modules/crash-vfs-umbrella-frameworks.m test fails. I'll need to look into this some more.

[jansvoboda11]

Fix failing test

[jansvoboda11]

Regarding the failing Modules/crash-vfs-umbrella-frameworks.m test, this is my understanding after debugging it.

Whenever we find that A.framework/Frameworks/B.framework is a symlink, we treat B as a separate top-level framework, not as a subframework of A. We detect this in ::getTopFrameworkDir() (in HeaderSearch.cpp) by calling FileManager::getCanonicalPath(), which resolves the symlink. We use the returned directory when calling the module map parser. Before this change, the DirectoryEntry::getName() ended up being the path Clang first accessed: A.framework/Frameworks/B.framework. After this change, it's the as-requested canonical path instead: B.framework. Since this canonicalization takes place and doesn't have any side-effect (I believe & hope), I think it should be fine to allow dropping the A.framework/Frameworks/B.framework directory from the reproducer VFS.
Interestingly, before this change, the reproducer generated PCMs only for A and I, which makes me believe B ended up being treated as a subframework somehow (maybe the VFS interferes with the symlink canonicalization?). With this patch, the reproducer generates PCMs for all A, B and I, which matches the original (crashing) invocation on line 13. For the record, the test was introduced in D20194.

@benlangmuir, does that sound like a reasonable thing to do?

[benlangmuir]

I think it should be fine to allow dropping the A.framework/Frameworks/B.framework directory from the reproducer VFS

I think technically this is wrong, since if you're missing the symlink, then A might not build -- e.g. it could be doing a relative include that needs the symlink. But am I understanding correctly that the reproducer was already broken in this case? If so I'm fine with this.

The right thing to do would be to capture both the framework and the symlink. I'm not sure how practical that is with the current architecture. @JDevlieghere any thoughts? Longer term, our CAS work could ultimately end up solving this in a better way by fully capturing all the inputs.

[jansvoboda11]

In D151855#4403879, @benlangmuir wrote:

I think it should be fine to allow dropping the A.framework/Frameworks/B.framework directory from the reproducer VFS

I think technically this is wrong, since if you're missing the symlink, then A might not build -- e.g. it could be doing a relative include that needs the symlink. But am I understanding correctly that the reproducer was already broken in this case? If so I'm fine with this.

You're right, this would break relative includes, which I believe the current test case would handle correctly. (It only fails to canonicalize the paths to B.framework and make it into a top-level framework.) I'll try to verify this, just to be sure.

The right thing to do would be to capture both the framework and the symlink. I'm not sure how practical that is with the current architecture.

Yeah, I think currently we only record the (as-requested) file names while parsing a module map. I'm not sure how we could record both the canonical and subframework-like paths with the current more precise FileEntryRefs. (Long-term, we should probably unify this with the "affecting files and module maps" logic in clang-scan-deps.)

[jansvoboda11]

In D151855#4403934, @jansvoboda11 wrote:

In D151855#4403879, @benlangmuir wrote:

I think it should be fine to allow dropping the A.framework/Frameworks/B.framework directory from the reproducer VFS

I think technically this is wrong, since if you're missing the symlink, then A might not build -- e.g. it could be doing a relative include that needs the symlink. But am I understanding correctly that the reproducer was already broken in this case? If so I'm fine with this.

You're right, this would break relative includes, which I believe the current test case would handle correctly. (It only fails to canonicalize the paths to B.framework and make it into a top-level framework.) I'll try to verify this, just to be sure.

I tried changing the #include <B/B.h> to relative #include "../Frameworks/B.framework/Headers/B.h" in A.h. The original Clang invocation compiles both of these fine with and without this patch, promoting B to a top-level framework.

The relative include fails to compile with the reproducer VFS, both with and without this patch. This boils down to Clang calling FileSystem::getRealPath("<snip>/Frameworks/A.framework/Frameworks/B.framework") when deciding whether to promote B to top-level framework. Here the RedirectingFileSystem ends up just canonicalizing the path (making it absolute, removing ., etc.) and forwarding to the underlying (real) FS. This doesn't have the symlink, so it returns non-zero error-code, and FileManager::getCanonicalName() ends up returning the virtual path under A.framework, preventing promotion of B to a top-level framework. This ends up registering B as a sub-framework of A in one place, but then Clang sees <snip>/Frameworks/B.framework and attempts to create B again with the same DirectoryEntry umbrella directory, resulting in the error below:

<build>/tools/clang/test/Modules/Output/crash-vfs-umbrella-frameworks.m.tmp/i/Frameworks/A.framework/Frameworks/B.framework/Modules/module.modulemap:2:19: error: umbrella for module 'A.B' already covers this directory
    2 |   umbrella header "B.h"
      |                   ^
<build>/tools/clang/test/Modules/Output/crash-vfs-umbrella-frameworks.m.tmp/i/Frameworks/A.framework/Frameworks/B.framework/Modules/module.modulemap:4:10: error: inferred submodules require a module with an umbrella
    4 |   module * { export * }
      |          ^
While building module 'I' imported from <source>/clang/test/Modules/crash-vfs-umbrella-frameworks.m:42:
In file included from <module-includes>:1:
<build>/tools/clang/test/Modules/Output/crash-vfs-umbrella-frameworks.m.tmp/i/Frameworks/I.framework/Headers/I.h:2:9: fatal error: could not build module 'A'
    2 | #import <A/A.h>
      |  ~~~~~~~^
<source>/clang/test/Modules/crash-vfs-umbrella-frameworks.m:42:9: fatal error: could not build module 'I'
   42 | @import I;
      |  ~~~~~~~^
4 errors generated.

(Note that the A.framework/Frameworks/B.framework/Headers/B.h and A.framework/Frameworks/B.framework/Modules/module.modulemap files do make it into the reproducer VFS with this patch, since they both get touched by the original Clang invocation. For some reason, we end up with duplicate entries for these in the overlay file without this patch.)

That said, this patch doesn't regress the relative include reproducer, since it's already broken. I'm not exactly sure what the fix for that particular issue is, since the interactions are pretty subtle. I don't think it's worth blocking this patch on it, though. What do you think?

[benlangmuir]

I don't think it's worth blocking this patch on it, though. What do you think?

Agreed, I don't think this is making it worse.

clang/include/clang/Lex/HeaderSearch.h

public:
   /// \returns The module with the given name.
   Module *lookupModule(StringRef ModuleName,
                        SourceLocation ImportLoc = SourceLocation(),
                        bool AllowSearch = true,
                        bool AllowExtraModuleMapSearch = false);
 
   /// Try to find a module map file in the given directory, returning
   /// \c nullopt if none is found.
-  OptionalFileEntryRef lookupModuleMapFile(const DirectoryEntry *Dir,
+  OptionalFileEntryRef lookupModuleMapFile(DirectoryEntryRef Dir,
                                            bool IsFramework);
 
   /// Determine whether there is a module map that may map the header
   /// with the given file name to a (sub)module.
   /// Always returns false if modules are disabled.
   ///
   /// \param Filename The name of the file.
   ///

clang/include/clang/Lex/ModuleMap.h

public:
   /// \param File The included file.
   void diagnoseHeaderInclusion(Module *RequestingModule,
                                bool RequestingModuleIsModuleInterface,
                                SourceLocation FilenameLoc, StringRef Filename,
                                FileEntryRef File);
 
   /// Determine whether the given header is part of a module
   /// marked 'unavailable'.
-  bool isHeaderInUnavailableModule(const FileEntry *Header) const;
+  bool isHeaderInUnavailableModule(FileEntryRef Header) const;
 
   /// Determine whether the given header is unavailable as part
   /// of the specified module.
-  bool isHeaderUnavailableInModule(const FileEntry *Header,
+  bool isHeaderUnavailableInModule(FileEntryRef Header,
                                    const Module *RequestingModule) const;
 
   /// Retrieve a module with the given name.
   ///
   /// \param Name The name of the module to look up.
   ///
   /// \returns The named module, if known; otherwise, returns null.
   Module *findModule(StringRef Name) const;

clang/lib/Lex/HeaderSearch.cpp

if (const FileEntry *PMMFile = getPrivateModuleMap(File, FileMgr)) {
     }
   }
 
   // This directory has a module map.
   return LMM_NewlyLoaded;
 }
 
 OptionalFileEntryRef
-HeaderSearch::lookupModuleMapFile(const DirectoryEntry *Dir, bool IsFramework) {
+HeaderSearch::lookupModuleMapFile(DirectoryEntryRef Dir, bool IsFramework) {
   if (!HSOpts->ImplicitModuleMaps)
     return std::nullopt;
   // For frameworks, the preferred spelling is Modules/module.modulemap, but
   // module.map at the framework root is also accepted.
-  SmallString<128> ModuleMapFileName(Dir->getName());
+  SmallString<128> ModuleMapFileName(Dir.getName());
   if (IsFramework)
     llvm::sys::path::append(ModuleMapFileName, "Modules");
   llvm::sys::path::append(ModuleMapFileName, "module.modulemap");
   if (auto F = FileMgr.getOptionalFileRef(ModuleMapFileName))
     return *F;
 
   // Continue to allow module.map
-  ModuleMapFileName = Dir->getName();
+  ModuleMapFileName = Dir.getName();
   llvm::sys::path::append(ModuleMapFileName, "module.map");
   if (auto F = FileMgr.getOptionalFileRef(ModuleMapFileName))
     return *F;
 
   // For frameworks, allow to have a private module map with a preferred
   // spelling when a public module map is absent.
   if (IsFramework) {
-    ModuleMapFileName = Dir->getName();
+    ModuleMapFileName = Dir.getName();
     llvm::sys::path::append(ModuleMapFileName, "Modules",
                             "module.private.modulemap");
     if (auto F = FileMgr.getOptionalFileRef(ModuleMapFileName))
       return *F;
   }
   return std::nullopt;
 }
 

clang/lib/Lex/ModuleMap.cpp

ModuleMap::findResolvedModulesForHeader(const FileEntry *File) const {
   // FIXME: Is this necessary?
   resolveHeaderDirectives(File);
   auto It = Headers.find(File);
   if (It == Headers.end())
     return std::nullopt;
   return It->second;
 }
 
-bool ModuleMap::isHeaderInUnavailableModule(const FileEntry *Header) const {
+bool ModuleMap::isHeaderInUnavailableModule(FileEntryRef Header) const {
   return isHeaderUnavailableInModule(Header, nullptr);
 }
 
-bool
-ModuleMap::isHeaderUnavailableInModule(const FileEntry *Header,
-                                       const Module *RequestingModule) const {
+bool ModuleMap::isHeaderUnavailableInModule(
+    FileEntryRef Header, const Module *RequestingModule) const {
   resolveHeaderDirectives(Header);
   HeadersMap::const_iterator Known = Headers.find(Header);
   if (Known != Headers.end()) {
     for (SmallVectorImpl<KnownHeader>::const_iterator
              I = Known->second.begin(),
              E = Known->second.end();
          I != E; ++I) {
 
for (SmallVectorImpl<KnownHeader>::const_iterator
         if (!RequestingModule && I->getRole() == ModuleMap::TextualHeader)
           continue;
         return false;
       }
     }
     return true;
   }
 
-  const DirectoryEntry *Dir = Header->getDir();
-  SmallVector<const DirectoryEntry *, 2> SkippedDirs;
+  OptionalDirectoryEntryRef Dir = Header.getDir();
+  SmallVector<DirectoryEntryRef, 2> SkippedDirs;
   StringRef DirName = Dir->getName();
 
   auto IsUnavailable = [&](const Module *M) {
     return !M->isAvailable() && (!RequestingModule ||
                                  M->isSubModuleOf(RequestingModule));
   };
 
   // Keep walking up the directory hierarchy, looking for a directory with
   // an umbrella header.
   do {
-    llvm::DenseMap<const DirectoryEntry *, Module *>::const_iterator KnownDir
-      = UmbrellaDirs.find(Dir);
+    auto KnownDir = UmbrellaDirs.find(*Dir);
     if (KnownDir != UmbrellaDirs.end()) {
       Module *Found = KnownDir->second;
       if (IsUnavailable(Found))
         return true;
 
       // Search up the module stack until we find a module with an umbrella
       // directory.
       Module *UmbrellaModule = Found;
       while (!UmbrellaModule->getEffectiveUmbrellaDir() &&
              UmbrellaModule->Parent)
         UmbrellaModule = UmbrellaModule->Parent;
 
       if (UmbrellaModule->InferSubmodules) {
-        for (const DirectoryEntry *SkippedDir : llvm::reverse(SkippedDirs)) {
+        for (DirectoryEntryRef SkippedDir : llvm::reverse(SkippedDirs)) {
           // Find or create the module that corresponds to this directory name.
           SmallString<32> NameBuf;
           StringRef Name = sanitizeFilenameAsIdentifier(
-              llvm::sys::path::stem(SkippedDir->getName()), NameBuf);
+              llvm::sys::path::stem(SkippedDir.getName()), NameBuf);
           Found = lookupModuleQualified(Name, Found);
           if (!Found)
             return false;
           if (IsUnavailable(Found))
             return true;
         }
 
         // Infer a submodule with the same name as this header file.
         SmallString<32> NameBuf;
         StringRef Name = sanitizeFilenameAsIdentifier(
-                           llvm::sys::path::stem(Header->getName()),
+                           llvm::sys::path::stem(Header.getName()),
                            NameBuf);
         Found = lookupModuleQualified(Name, Found);
         if (!Found)
           return false;
       }
 
       return IsUnavailable(Found);
     }
 
-    SkippedDirs.push_back(Dir);
+    SkippedDirs.push_back(*Dir);
 
     // Retrieve our parent path.
     DirName = llvm::sys::path::parent_path(DirName);
     if (DirName.empty())
       break;
 
     // Resolve the parent path to a directory entry.
-    if (auto DirEntry = SourceMgr.getFileManager().getDirectory(DirName))
-      Dir = *DirEntry;
-    else
-      Dir = nullptr;
+    Dir = SourceMgr.getFileManager().getOptionalDirectoryRef(DirName);
   } while (Dir);
 
   return false;
 }
 
 Module *ModuleMap::findModule(StringRef Name) const {
   llvm::StringMap<Module *>::const_iterator Known = Modules.find(Name);
   if (Known != Modules.end())

clang/test/Modules/crash-vfs-umbrella-frameworks.m

 // RUN: find %t/crash-vfs-*.cache/vfs | \
 // RUN:   grep "B.framework/Headers/B.h" | count 1
 
 // CHECK: Preprocessed source(s) and associated run script(s) are located at:
 // CHECK-NEXT: note: diagnostic msg: {{.*}}.m
 // CHECK-NEXT: note: diagnostic msg: {{.*}}.cache
 
 // CHECKYAML:      'type': 'directory',
-// CHECKYAML:      'name': "/[[PATH:.*]]/i/Frameworks/A.framework/Frameworks/B.framework/Headers",
-// CHECKYAML-NEXT:      'contents': [
-// CHECKYAML-NEXT:        {
-// CHECKYAML-NEXT:          'type': 'file',
-// CHECKYAML-NEXT:          'name': "B.h",
-// CHECKYAML-NEXT:          'external-contents': "/[[PATH]]/i/Frameworks/B.framework/Headers/B.h"
-
-// CHECKYAML:      'type': 'directory',
-// CHECKYAML:      'name': "/[[PATH]]/i/Frameworks/B.framework/Headers",
+// CHECKYAML:      'name': "/[[PATH:.*]]/i/Frameworks/B.framework/Headers",
 // CHECKYAML-NEXT:      'contents': [
 // CHECKYAML-NEXT:        {
 // CHECKYAML-NEXT:          'type': 'file',
 // CHECKYAML-NEXT:          'name': "B.h",
 // CHECKYAML-NEXT:          'external-contents': "/[[PATH]]/i/Frameworks/B.framework/Headers/B.h"
 
 @import I;