[clang] 935a07e - [clang][deps][lex] Avoid canonicalization of remapped framework directories

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2022-12-01T19:59:54-08:00
New Revision: 935a07ed21434825a96eb6d3acd2163edd0abe88

URL: https://github.com/llvm/llvm-project/commit/935a07ed21434825a96eb6d3acd2163edd0abe88
DIFF: https://github.com/llvm/llvm-project/commit/935a07ed21434825a96eb6d3acd2163edd0abe88.diff

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

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.

Reviewed By: benlangmuir

Differential Revision: https://reviews.llvm.org/D135841

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

Modified: 
    clang/lib/Lex/ModuleMap.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 9ff0204941c02..4e86c5c694441 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -1303,9 +1303,16 @@ ModuleMap::canonicalizeModuleMapPath(SmallVectorImpl<char> &Path) {
   // 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");
+    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

diff  --git a/clang/test/ClangScanDeps/modules-symlink-dir-vfs.c b/clang/test/ClangScanDeps/modules-symlink-dir-vfs.c
new file mode 100644
index 0000000000000..bc384b015e6f9
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-symlink-dir-vfs.c
@@ -0,0 +1,90 @@
+// 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
+ at 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