lld: use modern library search ordering

[compnerd]

Summary

This merges the static and shared library and behaves as if
-search_paths_first was specified which is also the default beahviour
on ld64 (and now lld). Unify the paths, and use llvm::sys::path to
deal with the path to be truly agnostic to the host.

[smeenai]

Could you upload with full context? It's hard to read this as-is.

[gkm]

lld/MachO/Driver.cpp

79

Nit: I'd rather see PATH_MAX, but literal ints like this are the norm.

85

Nit: check for fs::is_regular_file()

lld/test/MachO/link-search-order.s

17

Please add an archive that will be found, alongside the one that is shadowed by a dylib.

[MaskRay]

lld/MachO/Driver.cpp

79

Do we omit llvm:: for commonly used LLVM ADT/Support classes?

lld/test/MachO/link-search-order.s

12

Move it to the top

22

Looks misaligned.

You may need to convert some tabs to spaces.

[compnerd]

lld/MachO/Driver.cpp

79

PATH_MAX is different from MAX_PATH ... 260 is the larger of the two and there is no macro to unify the two spellings on different platforms.

85

That was not checked for previously, why introduce that with this change?

lld/test/MachO/link-search-order.s

17

I think that the existing check for the static archive covers that part of the test. Arguably, because this change is about cleaning up that path, it _could_ make sense to remove the test case from the change and do the testing improvements later. What case is it that is missing testing?

[gkm]

lld/MachO/Driver.cpp

79

Very well.

85

It's an opportunistic improvement: a tighter validity check.

lld/test/MachO/link-search-order.s

17

Tested:

• use a dylib (does not shadow an archive)
• use a dylib (shadows an archive)

Untested:

• use an archive (not shadowed by a dylib)

[compnerd]

lld/MachO/Driver.cpp

85

Seems better as a separate change as it technically changes the semantics.

lld/test/MachO/link-search-order.s

17

The last case already tested in a different test that I added previously.

[compnerd]

rGe78431354bcb6bec5be9adf4ea37d860445f8c16

[MaskRay]

In D81124#2077251, @compnerd wrote:

rGe78431354bcb6bec5be9adf4ea37d860445f8c16

You need to retain Reviewed by: and Differential Revision: to close the revision automatically when you push the commit to master.

arcfilter () {
  arc amend
  git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people (https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html). You should keep the tag. (I started to use --date=now because some people find author date != committer date annoying. The committer date is usually what people care.))

lld/MachO/Driver.cpp

   return args;
 }
 
-// This is for -lfoo. We'll look for libfoo.dylib from search paths.
-static Optional<std::string> findDylib(StringRef name) {
-  for (StringRef dir : config->searchPaths) {
-    std::string path = (dir + "/lib" + name + ".dylib").str();
-    if (fs::exists(path))
-      return path;
-  }
-  return None;
-}
+static Optional<std::string> findLibrary(StringRef name) {
+  std::string shared = (llvm::Twine("lib") + name + ".dylib").str();
+  std::string archive = (llvm::Twine("lib") + name + ".a").str();
+  llvm::SmallString<260> location;
 
-static Optional<std::string> findArchive(StringRef name) {
   for (StringRef dir : config->searchPaths) {
-    std::string path = (dir + "/lib" + name + ".a").str();
-    if (fs::exists(path))
-      return path;
+    for (StringRef library : {shared, archive}) {
+      location = dir;
+      llvm::sys::path::append(location, library);
+      if (fs::exists(location))
+        return location.str().str();
+    }
   }
   return None;
 }

       break;
     case OPT_l: {
       StringRef name = arg->getValue();
-      if (Optional<std::string> path = findDylib(name))
+      if (Optional<std::string> path = findLibrary(name)) {
         addFile(*path);
-      else if (Optional<std::string> path = findArchive(name))
-        addFile(*path);
-      else
-        error("library not found for -l" + name);
+        break;
+      }
+      error("library not found for -l" + name);
       break;
     }
     case OPT_platform_version: {

lld/test/MachO/link-search-order.s

+# REQUIRES: x86
+
+# RUN: mkdir -p %t
+#
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %p/Inputs/libhello.s -o %t/hello.o
+# RUN: lld -flavor darwinnew -dylib -install_name @executable_path/libhello.dylib %t/hello.o -o %t/libhello.dylib
+#
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %p/Inputs/libgoodbye.s -o %t/goodbye.o
+# RUN: lld -flavor darwinnew -dylib -install_name @executable_path/libgoodbye.dylib %t/goodbye.o -o %t/libgoodbye.dylib
+# RUN: llvm-ar --format=darwin crs %t/libgoodbye.a %t/goodbye.o
+#
+# RUN: llvm-mc -filetype obj -triple x86_64-apple-darwin %s -o %t/test.o
+# RUN: lld -flavor darwinnew -o %t/test -Z -L%t -lhello -lgoodbye %t/test.o
+#
+# RUN: llvm-objdump --macho --dylibs-used %t/test | FileCheck %s
+
+# CHECK: @executable_path/libhello.dylib
+# CHECK: @executable_path/libgoodbye.dylib
+# CHECK: /usr/lib/libSystem.B.dylib
+
+.section __TEXT,__text
+.global _main
+
+_main:
+  movl $0x2000004, %eax                         # write()
+  mov $1, %rdi                                  # stdout
+  movq _hello_world@GOTPCREL(%rip), %rsi
+  mov $13, %rdx                                 # length
+  syscall
+
+  movl $0x2000004, %eax                         # write()
+  mov $1, %rdi                                  # stdout
+  movq _hello_its_me@GOTPCREL(%rip), %rsi
+  mov $15, %rdx                                 # length
+  syscall
+
+  movl $0x2000004, %eax                         # write()
+  mov $1, %rdi                                  # stdout
+  movq _goodbye_world@GOTPCREL(%rip), %rsi
+  mov $15, %rdx                                 # length
+  syscall
+  mov $0, %rax
+  ret