[clang][deps] Canonicalize module map path

[benlangmuir]

Summary

When dep-scanning, canonicalize the module map path as much as we can. This avoids unnecessarily needing to build multiple versions of a module due to symlinks or case-insensitive file paths.

Despite the name tryGetRealPathName, the previous implementation did not actually return the realpath most of the time, and indeed it would be incorrect to do so since the realpath could be outside the module directory, which would have broken finding headers relative to the module.

Instead, use a canonicalization that is specific to the needs of modulemap files (canonicalize the directory separately from the filename).

[jansvoboda11]

IIUC this not only canonicalizes path to the main input file when compiling a PCM, but also -fmodule-map-file= arguments for dependencies, correct? Since that behavior is desirable, I think it would make sense to check for that in the test, WDYT?

clang/lib/Lex/ModuleMap.cpp

1307

Is that because the parser is looking for headers in ../Headers and ../PrivateHeaders? If so, would it make sense to make the parser smarter and canonicalize even framework paths here?

[benlangmuir]

In D134923#3825673, @jansvoboda11 wrote:

IIUC this not only canonicalizes path to the main input file when compiling a PCM, but also -fmodule-map-file= arguments for dependencies, correct? Since that behavior is desirable, I think it would make sense to check for that in the test, WDYT?

Yep, makes sense to me!

clang/lib/Lex/ModuleMap.cpp

1307

The failure I am aware of is that loadModuleMapFile will not detect the correct module Directory. I'm not sure if there are more failures after that. I didn't want to complicate this logic, because it seems like there are several possibilities:

• Modules/module.modulemap
• Versions/A/Modules/module.modulemap
• Versions/Current/Modules/module.modulemap

Also, if we get one of the versioned paths, but Modules/module.modulemap does not point to that specific file then it would be incorrect to use it. I'm inclined to leave this alone, at least for the current change.

[benlangmuir]

Test -fmodule-map-file paths as well.

[jansvoboda11]

Thanks, this LGTM with the test extension.

clang/test/ClangScanDeps/modules-symlink-dir.c

34

Nit: I assume {{.}} is here to prevent issues with path separators on Windows. I'd prefer putting / here for readability and injecting sed 's:\\\\\?:/:g' into the RUN line like we do in rest of scanner tests.

[benlangmuir]

Fix windows path separators using sed instead of regex "."

[jansvoboda11]

(You could also use the PREFIX variable you're passing to FileCheck to remove some .* regexes.)

[benlangmuir]

Simplify modmap checks in test to use PREFIX

[benlangmuir]

Canonicalize path separator(s) in between the (already canonicalized directory) and the filename. This could cause issues with redundant path separators (foo//bar) or non-native separators (foo/bar vs foo\bar on Windows). Hopefully this was the only issue on Windows.

[benlangmuir]

Rebased after landing https://reviews.llvm.org/D135220, which should hopefully fix the Windows test failure (it fixed the path for me locally on Windows, but I had some python-related issues that prevented me from running the tests properly).

clang/include/clang/Lex/ModuleMap.h

public:
   /// this module map will be globally unique for top-level modules. In the case
   /// of inferred modules, returns the module map that allowed the inference
   /// (e.g. contained 'module *'). Otherwise, returns
   /// getContainingModuleMapFile().
   Optional<FileEntryRef> getModuleMapFileForUniquing(const Module *M) const;
 
   void setInferredModuleAllowedBy(Module *M, const FileEntry *ModMap);
 
+  /// Canonicalize \p Path in a manner suitable for a module map file. In
+  /// particular, this canonicalizes the parent directory separately from the
+  /// filename so that it does not affect header resolution relative to the
+  /// modulemap.
+  ///
+  /// \returns an error code if any filesystem operations failed. In this case
+  /// \p Path is not modified.
+  std::error_code canonicalizeModuleMapPath(SmallVectorImpl<char> &Path);
+
   /// Get any module map files other than getModuleMapFileForUniquing(M)
   /// that define submodules of a top-level module \p M. This is cheaper than
   /// getting the module map file for each submodule individually, since the
   /// expected number of results is very small.
   AdditionalModMapsSet *getAdditionalModuleMapFiles(const Module *M) {
     auto I = AdditionalModMaps.find(M);
     if (I == AdditionalModMaps.end())
       return nullptr;

clang/lib/Lex/HeaderSearch.cpp

std::string HeaderSearch::getCachedModuleFileNameImpl(StringRef ModuleName,
   } else {
     // Construct the name <ModuleName>-<hash of ModuleMapPath>.pcm which should
     // ideally be globally unique to this particular module. Name collisions
     // in the hash are safe (because any translation unit can only import one
     // module with each name), but result in a loss of caching.
     //
     // To avoid false-negatives, we form as canonical a path as we can, and map
     // to lower-case in case we're on a case-insensitive file system.
-    std::string Parent =
-        std::string(llvm::sys::path::parent_path(ModuleMapPath));
-    if (Parent.empty())
-      Parent = ".";
-    auto Dir = FileMgr.getDirectory(Parent);
-    if (!Dir)
+    SmallString<128> CanonicalPath(ModuleMapPath);
+    if (getModuleMap().canonicalizeModuleMapPath(CanonicalPath))
       return {};
-    auto DirName = FileMgr.getCanonicalName(*Dir);
-    auto FileName = llvm::sys::path::filename(ModuleMapPath);
 
-    llvm::hash_code Hash =
-      llvm::hash_combine(DirName.lower(), FileName.lower());
+    llvm::hash_code Hash = llvm::hash_combine(CanonicalPath.str().lower());
 
     SmallString<128> HashStr;
     llvm::APInt(64, size_t(Hash)).toStringUnsigned(HashStr, /*Radix*/36);
     llvm::sys::path::append(Result, ModuleName + "-" + HashStr + ".pcm");
   }
   return Result.str().str();
 }
 

clang/lib/Lex/ModuleMap.cpp

ModuleMap::getModuleMapFileForUniquing(const Module *M) const {
   return getContainingModuleMapFile(M);
 }
 
 void ModuleMap::setInferredModuleAllowedBy(Module *M, const FileEntry *ModMap) {
   assert(M->IsInferred && "module not inferred");
   InferredModuleAllowedBy[M] = ModMap;
 }
 
+std::error_code
+ModuleMap::canonicalizeModuleMapPath(SmallVectorImpl<char> &Path) {
+  StringRef Dir = llvm::sys::path::parent_path({Path.data(), Path.size()});
+
+  // Do not canonicalize within the framework; the module map parser expects
+  // Modules/ not Versions/A/Modules.
+  if (llvm::sys::path::filename(Dir) == "Modules") {
+    StringRef Parent = llvm::sys::path::parent_path(Dir);
+    if (Parent.endswith(".framework"))
+      Dir = Parent;
+  }
+
+  FileManager &FM = SourceMgr.getFileManager();
+  auto DirEntry = FM.getDirectory(Dir.empty() ? "." : Dir);
+  if (!DirEntry)
+    return DirEntry.getError();
+
+  // Canonicalize the directory.
+  StringRef CanonicalDir = FM.getCanonicalName(*DirEntry);
+  if (CanonicalDir != Dir) {
+    bool Done = llvm::sys::path::replace_path_prefix(Path, Dir, CanonicalDir);
+    (void)Done;
+    assert(Done && "Path should always start with Dir");
+  }
+
+  // In theory, the filename component should also be canonicalized if it
+  // on a case-insensitive filesystem. However, the extra canonicalization is
+  // expensive and if clang looked up the filename it will always be lowercase.
+
+  // Remove ., remove redundant separators, and switch to native separators.
+  // This is needed for separators between CanonicalDir and the filename.
+  llvm::sys::path::remove_dots(Path);
+
+  return std::error_code();
+}
+
 void ModuleMap::addAdditionalModuleMapFile(const Module *M,
                                            const FileEntry *ModuleMap) {
   AdditionalModMaps[M].insert(ModuleMap);
 }
 
 LLVM_DUMP_METHOD void ModuleMap::dump() {
   llvm::errs() << "Modules:";
   for (llvm::StringMap<Module *>::iterator M = Modules.begin(),

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

ModuleID ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
   ModI.first->second = std::make_unique<ModuleDeps>();
   ModuleDeps &MD = *ModI.first->second;
 
   MD.ID.ModuleName = M->getFullModuleName();
   MD.ImportedByMainFile = DirectModularDeps.contains(M);
   MD.ImplicitModulePCMPath = std::string(M->getASTFile()->getName());
   MD.IsSystem = M->IsSystem;
 
-  Optional<FileEntryRef> ModuleMap = MDC.ScanInstance.getPreprocessor()
-                                         .getHeaderSearchInfo()
-                                         .getModuleMap()
-                                         .getModuleMapFileForUniquing(M);
+  ModuleMap &ModMapInfo =
+      MDC.ScanInstance.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+
+  Optional<FileEntryRef> ModuleMap = ModMapInfo.getModuleMapFileForUniquing(M);
 
   if (ModuleMap) {
-    StringRef Path = ModuleMap->getFileEntry().tryGetRealPathName();
-    if (Path.empty())
-      Path = ModuleMap->getName();
+    SmallString<128> Path = ModuleMap->getNameAsRequested();
+    ModMapInfo.canonicalizeModuleMapPath(Path);
     MD.ClangModuleMapFile = std::string(Path);
   }
 
   serialization::ModuleFile *MF =
       MDC.ScanInstance.getASTReader()->getModuleManager().lookup(
           M->getASTFile());
   MDC.ScanInstance.getASTReader()->visitInputFiles(
       *MF, true, true, [&](const serialization::InputFile &IF, bool isSystem) {

clang/test/ClangScanDeps/modules-symlink-dir.c

+// Check that we canonicalize the module map path without changing the module
+// directory, which would break header lookup.
+
+// REQUIRES: shell
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json
+// RUN: ln -s module %t/symlink-to-module
+// RUN: ln -s ../actual.modulemap %t/module/module.modulemap
+// RUN: ln -s A %t/module/F.framework/Versions/Current
+// RUN: ln -s Versions/Current/Modules %t/module/F.framework/Modules
+// RUN: ln -s Versions/Current/Headers %t/module/F.framework/Headers
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -j 1 \
+// RUN:   -format experimental-full  -mode=preprocess-dependency-directives \
+// RUN:   -optimize-args -module-files-dir %t/build > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck -DPREFIX=%/t %s
+
+// Check the module commands actually build.
+// RUN: %deps-to-rsp %t/deps.json --module-name=Mod > %t/Mod.rsp
+// RUN: %deps-to-rsp %t/deps.json --module-name=F > %t/F.rsp
+// RUN: %clang @%t/Mod.rsp
+// RUN: %clang @%t/F.rsp
+
+// CHECK:      "modules": [
+// CHECK:        {
+// CHECK:          "clang-module-deps": [],
+// CHECK:          "clang-modulemap-file": "[[PREFIX]]/module/F.framework/Modules/module.modulemap"
+// CHECK:          "command-line": [
+// CHECK-NOT: symlink-to-module
+// CHECK:            "[[PREFIX]]/module/F.framework/Modules/module.modulemap"
+// CHECK-NOT: symlink-to-module
+// CHECK:          ]
+// CHECK:          "context-hash": "[[F_CONTEXT_HASH:[A-Z0-9]+]]"
+// CHECK:          "name": "F"
+// CHECK-NEXT:   }
+// CHECK-NEXT:   {
+// CHECK:          "clang-modulemap-file": "[[PREFIX]]/module/module.modulemap"
+// CHECK:          "command-line": [
+// CHECK-NOT: symlink-to-module
+// CHECK:            "[[PREFIX]]/module/module.modulemap"
+// CHECK-NOT: symlink-to-module
+// CHECK:          ]
+// CHECK:          "context-hash": "[[CONTEXT_HASH:[A-Z0-9]+]]"
+// CHECK:          "name": "Mod"
+// CHECK-NEXT:   }
+// CHECK-NEXT: ]
+// CHECK:      "translation-units": [
+// CHECK:              "clang-module-deps": [
+// CHECK:                {
+// CHECK:                  "context-hash": "[[CONTEXT_HASH]]"
+// CHECK:                  "module-name": "Mod"
+// CHECK:                }
+// CHECK-NEXT:         ],
+// CHECK:              "command-line": [
+// CHECK:                "-fmodule-map-file=[[PREFIX]]/module/module.modulemap"
+// CHECK:              ]
+// CHECK:              "clang-module-deps": [
+// CHECK:                {
+// CHECK:                  "context-hash": "[[CONTEXT_HASH]]"
+// CHECK:                  "module-name": "Mod"
+// CHECK:                }
+// CHECK-NEXT:         ]
+// CHECK:              "command-line": [
+// CHECK:                "-fmodule-map-file=[[PREFIX]]/module/module.modulemap"
+// CHECK:              ]
+// CHECK:              "clang-module-deps": [
+// CHECK:                {
+// CHECK:                  "context-hash": "[[F_CONTEXT_HASH]]"
+// CHECK:                  "module-name": "F"
+// CHECK:                }
+// CHECK-NEXT:         ]
+// CHECK:              "command-line": [
+// CHECK:                "-fmodule-map-file=[[PREFIX]]/module/F.framework/Modules/module.modulemap"
+// CHECK:              ]
+// CHECK:              "clang-module-deps": [
+// CHECK:                {
+// CHECK:                  "context-hash": "[[F_CONTEXT_HASH]]"
+// CHECK:                  "module-name": "F"
+// CHECK:                }
+// CHECK-NEXT:         ]
+// CHECK:              "command-line": [
+// CHECK:                "-fmodule-map-file=[[PREFIX]]/module/F.framework/Modules/module.modulemap"
+// CHECK:              ]
+
+//--- cdb.json.in
+[
+  {
+    "directory": "DIR",
+    "command": "clang -fsyntax-only DIR/tu1.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+    "file": "DIR/tu1.c"
+  },
+  {
+    "directory": "DIR",
+    "command": "clang -fsyntax-only DIR/tu2.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+    "file": "DIR/tu2.c"
+  },
+  {
+    "directory": "DIR",
+    "command": "clang -fsyntax-only -F DIR/symlink-to-module DIR/tu3.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+    "file": "DIR/tu3.c"
+  },
+  {
+    "directory": "DIR",
+    "command": "clang -fsyntax-only -F DIR/module DIR/tu3.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache",
+    "file": "DIR/tu3.c"
+  },
+]
+
+//--- actual.modulemap
+module Mod { header "header.h" }
+
+//--- module/header.h
+
+//--- tu1.c
+#include "symlink-to-module/header.h"
+
+//--- tu2.c
+#include "module/header.h"
+
+//--- module/F.framework/Versions/A/Modules/module.modulemap
+framework module F {
+  umbrella header "F.h"
+}
+
+//--- module/F.framework/Versions/A/Headers/F.h
+
+//--- tu3.c
+#include "F/F.h"