[clang][deps][lex] Avoid canonicalization of remapped framework directories

[jansvoboda11]

Summary

In D134923, the scanner introduced canonicalization of framework directories when reporting module map paths in order to increase module sharing. However, if we canonicalize framework directory that plays a role in a VFS remapping, and later try to use that module map to build the module, header lookup can fail. This happens when the module headers are remapped using the original framework path.

This patch fixes that. The implementation relies on the fact that the chain of directories in VFS remapping are assigned DirectoryEntry objects distinct from their on-disk counterparts. If we detect that case, we avoid the canonicalization.

[benlangmuir]

Please add a comment about why this equality check is necessary.

clang/lib/Lex/ModuleMap.cpp

1307

Wouldn't this indicate the particular VFS has a bug?

[benlangmuir]

clang/lib/Lex/ModuleMap.cpp

1307

^ Sorry this question was an old draft, you can disregard.

clang/lib/Lex/ModuleMap.cpp

ModuleMap::canonicalizeModuleMapPath(SmallVectorImpl<char> &Path) {
   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) {
+    auto CanonicalDirEntry = FM.getDirectory(CanonicalDir);
+    // Only use the canonicalized path if it resolves to the same entry as the
+    // original. This is not true if there's a VFS overlay on top of a FS where
+    // the directory is a symlink. The overlay would not remap the target path
+    // of the symlink to the same directory entry in that case.
+    if (CanonicalDirEntry && *CanonicalDirEntry == *DirEntry) {
       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);

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

+// This test checks that we're not canonicalizing framework directories that
+// play a role in VFS remapping. This could lead header search to fail when
+// building that module.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// REQUIRES: shell
+
+// RUN: mkdir -p %t/frameworks-symlink
+// RUN: ln -s %t/frameworks/FW.framework %t/frameworks-symlink/FW.framework
+
+// RUN: mkdir -p %t/copy
+// RUN: cp %t/frameworks/FW.framework/Headers/FW.h     %t/copy
+// RUN: cp %t/frameworks/FW.framework/Headers/Header.h %t/copy
+
+//--- frameworks/FW.framework/Modules/module.modulemap
+framework module FW { umbrella header "FW.h" }
+//--- frameworks/FW.framework/Headers/FW.h
+#import <FW/Header.h>
+//--- frameworks/FW.framework/Headers/Header.h
+// empty
+
+//--- tu.m
+@import FW;
+
+//--- overlay.json.template
+{
+  "version": 0,
+  "case-sensitive": "false",
+  "roots": [
+    {
+      "contents": [
+        {
+          "external-contents": "DIR/copy/Header.h",
+          "name": "Header.h",
+          "type": "file"
+        },
+        {
+          "external-contents": "DIR/copy/FW.h",
+          "name": "FW.h",
+          "type": "file"
+        }
+      ],
+      "name": "DIR/frameworks-symlink/FW.framework/Headers",
+      "type": "directory"
+    }
+  ]
+}
+
+//--- cdb.json.template
+[{
+  "directory": "DIR",
+  "file": "DIR/tu.m",
+  "command": "clang -fmodules -fmodules-cache-path=DIR/cache -ivfsoverlay DIR/overlay.json -F DIR/frameworks-symlink -c DIR/tu.m -o DIR/tu.o -Werror=non-modular-include-in-framework-module"
+}]
+
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: sed -e "s|DIR|%/t|g" %t/overlay.json.template > %t/overlay.json
+
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json
+// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:      {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [],
+// CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/frameworks-symlink/FW.framework/Modules/module.modulemap",
+// CHECK-NEXT:       "command-line": [
+// CHECK-NEXT:         "-cc1",
+// CHECK:              "-emit-module",
+// CHECK-NEXT:         "-x",
+// CHECK-NEXT:         "objective-c",
+// CHECK-NEXT:         "[[PREFIX]]/frameworks-symlink/FW.framework/Modules/module.modulemap",
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK-NEXT:         "[[PREFIX]]/copy/FW.h",
+// CHECK-NEXT:         "[[PREFIX]]/copy/Header.h",
+// CHECK-NEXT:         "[[PREFIX]]/frameworks-symlink/FW.framework/Modules/module.modulemap"
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "name": "FW"
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK:        ]
+// CHECK:      }
+
+// RUN: %deps-to-rsp %t/result.json --module-name=FW > %t/FW.cc1.rsp
+// RUN: %clang @%t/FW.cc1.rsp