[lld] check cache in loadDylib before real_path

[rmaz]

In #140791 an attempt was made to defer the cost of real_path to after checking
the dylib cache. This was reverted due to issues incorrect handling of the
reference after the map updated.

In this PR the approach is changed to avoid mutating the dylib cache when
checking for real path presence, and updating the map only at the end of the
function.

[llvmbot]

@llvm/pr-subscribers-lld-macho

Author: Richard Howell (rmaz)

Changes

In #140791 an attempt was made to defer the cost of real_path to after checking
the dylib cache. This was reverted due to issues incorrect handling of the
reference after the map updated.

In this PR the approach is changed to recursively call loadDylib with a
resolved symlink path if it differs from the original path. This avoids the
complicated logic holding on to two references that need to be updated
together.

---

Full diff: https://github.com/llvm/llvm-project/pull/143595.diff

1 Files Affected:

• (modified) lld/MachO/DriverUtils.cpp (+22-6)

diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp
index f7f6be049f0e1..30cb83f8d7a2e 100644
--- a/lld/MachO/DriverUtils.cpp
+++ b/lld/MachO/DriverUtils.cpp
@@ -15,12 +15,14 @@
 #include "lld/Common/Reproduce.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/Path.h"
 #include "llvm/TextAPI/InterfaceFile.h"
 #include "llvm/TextAPI/TextAPIReader.h"
@@ -227,12 +229,7 @@ static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
 
 DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
                             bool isBundleLoader, bool explicitlyLinked) {
-  // Frameworks can be found from different symlink paths, so resolve
-  // symlinks before looking up in the dylib cache.
-  SmallString<128> realPath;
-  std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
-  CachedHashStringRef path(!err ? uniqueSaver().save(StringRef(realPath))
-                                : mbref.getBufferIdentifier());
+  CachedHashStringRef path(mbref.getBufferIdentifier());
   DylibFile *&file = loadedDylibs[path];
   if (file) {
     if (explicitlyLinked)
@@ -240,6 +237,25 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
     return file;
   }
 
+  // Frameworks can be found from different symlink paths, so resolve
+  // symlinks and look up in the dylib cache.
+  SmallString<128> realPath;
+  std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
+  if (!err && !StringRef(realPath).ends_with(mbref.getBufferIdentifier())) {
+    // Symlink has resolved to a different path, load the dylib from there
+    // and cache the result for the symlink too.
+    if (auto *realFile =
+            loadDylib(MemoryBufferRef(mbref.getBuffer(),
+                                      uniqueSaver().save(StringRef(realPath))),
+                      umbrella, isBundleLoader, explicitlyLinked)) {
+      loadedDylibs[path] = realFile;
+      return realFile;
+    }
+
+    // The map may have mutated, so update the reference.
+    file = loadedDylibs[path];
+  }
+
   DylibFile *newFile;
   file_magic magic = identify_magic(mbref.getBuffer());
   if (magic == file_magic::tapi_file) {

[llvmbot]

@llvm/pr-subscribers-lld

Author: Richard Howell (rmaz)

Changes

In #140791 an attempt was made to defer the cost of real_path to after checking
the dylib cache. This was reverted due to issues incorrect handling of the
reference after the map updated.

In this PR the approach is changed to recursively call loadDylib with a
resolved symlink path if it differs from the original path. This avoids the
complicated logic holding on to two references that need to be updated
together.

---

Full diff: https://github.com/llvm/llvm-project/pull/143595.diff

1 Files Affected:

• (modified) lld/MachO/DriverUtils.cpp (+22-6)

diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp
index f7f6be049f0e1..30cb83f8d7a2e 100644
--- a/lld/MachO/DriverUtils.cpp
+++ b/lld/MachO/DriverUtils.cpp
@@ -15,12 +15,14 @@
 #include "lld/Common/Reproduce.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBufferRef.h"
 #include "llvm/Support/Path.h"
 #include "llvm/TextAPI/InterfaceFile.h"
 #include "llvm/TextAPI/TextAPIReader.h"
@@ -227,12 +229,7 @@ static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
 
 DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
                             bool isBundleLoader, bool explicitlyLinked) {
-  // Frameworks can be found from different symlink paths, so resolve
-  // symlinks before looking up in the dylib cache.
-  SmallString<128> realPath;
-  std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
-  CachedHashStringRef path(!err ? uniqueSaver().save(StringRef(realPath))
-                                : mbref.getBufferIdentifier());
+  CachedHashStringRef path(mbref.getBufferIdentifier());
   DylibFile *&file = loadedDylibs[path];
   if (file) {
     if (explicitlyLinked)
@@ -240,6 +237,25 @@ DylibFile *macho::loadDylib(MemoryBufferRef mbref, DylibFile *umbrella,
     return file;
   }
 
+  // Frameworks can be found from different symlink paths, so resolve
+  // symlinks and look up in the dylib cache.
+  SmallString<128> realPath;
+  std::error_code err = fs::real_path(mbref.getBufferIdentifier(), realPath);
+  if (!err && !StringRef(realPath).ends_with(mbref.getBufferIdentifier())) {
+    // Symlink has resolved to a different path, load the dylib from there
+    // and cache the result for the symlink too.
+    if (auto *realFile =
+            loadDylib(MemoryBufferRef(mbref.getBuffer(),
+                                      uniqueSaver().save(StringRef(realPath))),
+                      umbrella, isBundleLoader, explicitlyLinked)) {
+      loadedDylibs[path] = realFile;
+      return realFile;
+    }
+
+    // The map may have mutated, so update the reference.
+    file = loadedDylibs[path];
+  }
+
   DylibFile *newFile;
   file_magic magic = identify_magic(mbref.getBuffer());
   if (magic == file_magic::tapi_file) {

[ellishg]

Can we add a test case that would have crashed the previous PR?

[rmaz]

Can we add a test case that would have crashed the previous PR?

Tricky to manage in a unit test, but i'll try and come up with a test that fails without this change involving re-exported library names and symlinks.

[drodriguez]

@zmodem I am not sure if you have been able to test this in your internal systems. I understand this was complicated to reproduce, but if it is even more difficult to reproduce now, it might mean it is removed completely. Obviously landing and waiting for your internal systems to catch up is also a possibility.

[zmodem]

I didn't try it, but the code looks fine so I don't think there should be any problems.

[zmodem]

I tried the patch on the reproducer I uploaded, and that links without issue.