[lld/mac] When handling @loader_path, use realpath() of symlinks

[thakis]

Summary

This is important for Frameworks, which are usually symlinks.

ld64 gets this right for @rpath that's replaced with @loader_path, but not for
bare @loader_path -- ld64's code calls realpath() in that case too, but ignores
the result.

ld64 somehow manages to find libbar1.dylib in the test without the
explicit -rpath in Foo1. I don't understand why or how. But this
change is a step forward and fixes an immediate problem I'm having,
so let's start with this :)

[int3]

lld/MachO/InputFiles.cpp

794–796

Would it make sense to call realPath before constructing MemoryBuffers in readFile, so that getName() always returns a canonicalized path? Or would that mess up other things?

798

intentional newline?

lld/test/MachO/link-search-at-loader-path-symlink.s

25

[thakis]

comments

[thakis]

lld/MachO/InputFiles.cpp

794–796

I think that'd be good. It'd also make us not load libSystem twice when doing -lSystem -lpthreads. I'm a bit scared of changing that though (for no good reason), so I'd like to land the smaller change (and the test) here first if that's ok. I hereby pledge to send you a follow-up that tries moving this call earlier :)

(ld64 has the realpath call in its code corresponding to the code here -- but parts of ld64 are all over the place so that doesn't necessarily mean much.)

798

No, thanks :)

lld/test/MachO/link-search-at-loader-path-symlink.s

25

Maybe I meant "..." :P

[int3]

lld/MachO/InputFiles.cpp

794–796

Fair enough :)

lld/MachO/InputFiles.cpp

static DylibFile *findDylib(StringRef path, DylibFile *umbrella,
 
   // TODO: Handle -dylib_file
 
   SmallString<128> newPath;
   if (config->outputType == MH_EXECUTE &&
       path.consume_front("@executable_path/")) {
     // ld64 allows overriding this with the undocumented flag -executable_path.
     // lld doesn't currently implement that flag.
-    path::append(newPath, sys::path::parent_path(config->outputFile), path);
+    path::append(newPath, path::parent_path(config->outputFile), path);
     path = newPath;
   } else if (path.consume_front("@loader_path/")) {
-    path::append(newPath, sys::path::parent_path(umbrella->getName()), path);
+    fs::real_path(umbrella->getName(), newPath);
+    path::remove_filename(newPath);
+    path::append(newPath, path);
     path = newPath;
   } else if (path.startswith("@rpath/")) {
     for (StringRef rpath : umbrella->rpaths) {
       newPath.clear();
-      if (rpath.consume_front("@loader_path/"))
-        path::append(newPath, sys::path::parent_path(umbrella->getName()));
+      if (rpath.consume_front("@loader_path/")) {
+        fs::real_path(umbrella->getName(), newPath);
+        path::remove_filename(newPath);
+      }
       path::append(newPath, rpath, path.drop_front(strlen("@rpath/")));
       if (Optional<std::string> dylibPath = resolveDylibPath(newPath))
         return loadDylib(*dylibPath, umbrella);
     }
   }
 
   if (currentTopLevelTapi) {
     for (InterfaceFile &child :

lld/test/MachO/link-search-at-loader-path-symlink.s

+# REQUIRES: x86, shell
+
+# RUN: rm -rf %t; split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %t/bar.s -o %t/bar.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/test.s -o %t/test.o
+
+## 1. Test symlink with reexport to @rpath and rpath pointing to @loader_path.
+## The @loader_path-relative path is looked after resolving the symlink.
+
+# RUN: mkdir -p %t/Foo1.framework/Versions/A
+# RUN: %lld -dylib -install_name @rpath/libbar1.dylib %t/bar.o -o %t/Foo1.framework/Versions/A/libbar1.dylib
+
+# RUN: %lld -dylib -install_name %t/Foo1.framework/Versions/A/Foo1 %t/foo.o \
+# RUN:     -reexport_library %t/Foo1.framework/Versions/A/libbar1.dylib \
+# RUN:     -rpath @loader_path/. \
+# RUN:     -o %t/Foo1.framework/Versions/A/Foo1
+# RUN: ln -sf A %t/Foo1.framework/Versions/Current
+# RUN: ln -sf Versions/Current/Foo1 %t/Foo1.framework/Foo1
+
+# RUN: %lld -lSystem -F%t -framework Foo1 %t/test.o -o %t/test1
+
+## 2. Test symlink with reexport to @loader_path-relative path directly.
+## The @loader_path-relative path is looked after resolving the symlink.
+## ld64 gets this wrong -- it calls realpath() but ignores the result.
+## (ld64-609, Options.cpp, Options::findFile(), "@loader_path/" handling.)
+
+# RUN: mkdir -p %t/Foo2.framework/Versions/A
+# RUN: %lld -dylib -install_name @loader_path/libbar2.dylib %t/bar.o -o %t/Foo2.framework/Versions/A/libbar2.dylib
+
+# RUN: %lld -dylib -install_name %t/Foo2.framework/Versions/A/Foo2 %t/foo.o \
+# RUN:     -reexport_library %t/Foo2.framework/Versions/A/libbar2.dylib \
+# RUN:     -o %t/Foo2.framework/Versions/A/Foo2
+# RUN: ln -sf A %t/Foo2.framework/Versions/Current
+# RUN: ln -sf Versions/Current/Foo2 %t/Foo2.framework/Foo2
+
+# RUN: %lld -lSystem -F%t -framework Foo2 %t/test.o -o %t/test2
+
+#--- foo.s
+.globl _foo
+_foo:
+  ret
+
+#--- bar.s
+.globl _bar
+_bar:
+  ret
+
+#--- test.s
+.globl _main
+.text
+_main:
+  callq _foo
+  callq _bar
+  ret
+