MachO: support `-syslibroot`

[compnerd]

Summary

This adds support for the -syslibroot option. This is required to
make the library search order actually function. With this, it is now
possible to link a test Darwin x86_64 program with lld on Darwin.

[int3]

lld/MachO/Driver.cpp

162

any particular reason why you picked 261?

174

suppresses

lld/test/MachO/syslibroot.test

2

s/non-existant/nonexistent/

4

s/NONEXISTANT/NONEXISTENT/

15

nit: could we have a shorter tmp directory name?

16

nit: most other tests use double-dash flags, i.e. --check-prefix

19

nit: would be better to pass the tmp dir to FileCheck via -DTMP=%t and use it here instead of {{.+}}. I believe the current check would pass even if there were only whitespace before /Users/...

[int3]

clearing my queue

[compnerd]

lld/MachO/Driver.cpp

162

MAX(256, 261) - 261 is the Windows path limit, 256 is the value used on Unix.

[compnerd]

lld/test/MachO/syslibroot.test

15

Sure, we can make it a system library path instead. IMO, the longer path immediately identifies that this is definitely not a default search path (/System/Library/Frameworks vs /Library/Frameworks is slightly easier to miss).

[compnerd]

Address review feedback

[int3]

Oh yeah, don't forget to remove the HelpHidden tag from syslibroot's definition in Options.td

lld/MachO/Driver.cpp

160

Will this work on Windows given the use of Style::posix?

162

Ah I see. Can we make it a named constant?

lld/test/MachO/syslibroot.test

22–23

which is the fuzzy match here? not sure I follow...

24

is /var/empty also going to be empty on Windows? might be better to create our own empty tmp directory instead

[int3]

lld/MachO/Driver.cpp

161

codebase convention is to avoid auto unless the type is immediately obvious

164

tested it out locally and realized that these isDirectory checks are now going to spew dir-not-found errors... we should probably factor out the error logging

424

avoid auto

[int3]

search-paths.test needs to be updated too

[compnerd]

lld/MachO/Driver.cpp

160

Yes, it should work on windows just as well. I've been running the tests on Windows :)

161

I think its obvious given that roots is just declared above, but Im fine with replacing it with StringRef.

162

Id rather do that in a follow up. There are other places where we have file path sized buffers.

164

Hmm, would you prefer to use llvm::sys::fs::is_directory instead?

424

const auto arg is pretty common, but sure, Arg *arg is shorter.

lld/test/MachO/syslibroot.test

22–23

/var/empty is not being checked.

24

No, /var/empty will not exist on Windows, but the fuzzy match takes care of that. The intent of the /var/empty is ensure that a non-sysroot is not treated as a sysroot.

[int3]

lgtm modulo those two questions. Thanks!

lld/MachO/Driver.cpp

160

I guess my question then is how does it work? I was under the impression that we would want the style to be native since the path separators are different on Windows.

(I'm working on setting up a Windows VM so I can answer these things myself in the future 😅)

162

sgtm

164

seems better, thanks!

lld/test/MachO/syslibroot.test

22–23

ah I see. Maybe make that explicit in the comment?

Also, shouldn't we be able to portably check that /var/empty isn't present? My understanding is that the -syslibroot / will make -syslibroot /var/empty a no-op on all platforms... is that right?

[compnerd]

lld/MachO/Driver.cpp

160

Windows paths are dark and full of terrors. The Win32 APIs will re-normalize the paths and accept / and \ (though not the NT APIs). I opted for the posix representation to make the printing work out better. We can always change this in the future.

lld/test/MachO/syslibroot.test

22–23

Sure, I can make that more explicit in the comment.

The problem is that -syslibroot / will then pick up /usr/lib and /usr/local/lib which makes the test more brittle. This was basically my attempt to force the test to be more uniform across Linux/BSD/Darwin/Windows.

lld/MachO/Driver.cpp

if (!fs::exists(path)) {
     warn("option -" + option + path + " references a non-directory path");
     return false;
   }
   return true;
 }
 
 static void getSearchPaths(std::vector<StringRef> &paths, unsigned optionCode,
                            opt::InputArgList &args,
+                           const std::vector<StringRef> &roots,
                            const SmallVector<StringRef, 2> &systemPaths) {
   StringRef optionLetter{(optionCode == OPT_F ? "F" : "L")};
   for (auto const &path : args::getStrings(args, optionCode)) {
+    // NOTE: only absolute paths are re-rooted to syslibroot(s)
+    if (llvm::sys::path::is_absolute(path, llvm::sys::path::Style::posix)) {
+      for (StringRef root : roots) {
+        SmallString<261> buffer(root);
+        llvm::sys::path::append(buffer, path);
+        // Do not warn about paths that are computed via the syslib roots
+        if (llvm::sys::fs::is_directory(buffer))
+          paths.push_back(saver.save(buffer.str()));
+      }
+    } else {
       if (isDirectory(optionLetter, path))
         paths.push_back(path);
     }
-  if (!args.hasArg(OPT_Z) && Triple(sys::getProcessTriple()).isOSDarwin()) {
-    for (auto const &path : systemPaths) {
-      if (isDirectory(optionLetter, path))
-        paths.push_back(path);
+  }
+
+  // `-Z` suppresses the standard "system" search paths.
+  if (args.hasArg(OPT_Z))
+    return;
+
+    for (auto root : roots) {
+      SmallString<261> buffer(root);
+      llvm::sys::path::append(buffer, path);
+      if (isDirectory(optionLetter, buffer))
+        paths.push_back(saver.save(buffer.str()));
     }
   }
 }
 
-static void getLibrarySearchPaths(std::vector<StringRef> &paths,
-                                  opt::InputArgList &args) {
-  getSearchPaths(paths, OPT_L, args, {"/usr/lib", "/usr/local/lib"});
+static void getLibrarySearchPaths(opt::InputArgList &args,
+                                  const std::vector<StringRef> &roots,
+                                  std::vector<StringRef> &paths) {
+  getSearchPaths(paths, OPT_L, args, roots, {"/usr/lib", "/usr/local/lib"});
 }
 
-static void getFrameworkSearchPaths(std::vector<StringRef> &paths,
-                                    opt::InputArgList &args) {
-  getSearchPaths(paths, OPT_F, args,
+static void getFrameworkSearchPaths(opt::InputArgList &args,
+                                    const std::vector<StringRef> &roots,
+                                    std::vector<StringRef> &paths) {
+  getSearchPaths(paths, OPT_F, args, roots,
                  {"/Library/Frameworks", "/System/Library/Frameworks"});
 }
 
 static void addFile(StringRef path) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
   symtab = make<SymbolTable>();
   target = createTargetInfo(args);
 
   config->entry = symtab->addUndefined(args.getLastArgValue(OPT_e, "_main"));
   config->outputFile = args.getLastArgValue(OPT_o, "a.out");
   config->installName =
       args.getLastArgValue(OPT_install_name, config->outputFile);
   config->headerPad = args::getHex(args, OPT_headerpad, /*Default=*/32);
-  getLibrarySearchPaths(config->librarySearchPaths, args);
-  getFrameworkSearchPaths(config->frameworkSearchPaths, args);
   config->outputType = args.hasArg(OPT_dylib) ? MH_DYLIB : MH_EXECUTE;
 
+  std::vector<StringRef> roots;
+  for (const Arg *arg : args.filtered(OPT_syslibroot))
+    roots.push_back(arg->getValue());
+  // NOTE: the final `-syslibroot` being `/` will ignore all roots
+  if (roots.size() && roots.back() == "/")
+    roots.clear();
+  // NOTE: roots can never be empty - add an empty root to simplify the library
+  // and framework search path computation.
+  if (roots.empty())
+    roots.emplace_back("");
+
+  getLibrarySearchPaths(args, roots, config->librarySearchPaths);
+  getFrameworkSearchPaths(args, roots, config->frameworkSearchPaths);
+
   if (args.hasArg(OPT_v)) {
     message(getLLDVersion());
     message(StringRef("Library search paths:") +
             (config->librarySearchPaths.size()
                  ? "\n\t" + llvm::join(config->librarySearchPaths, "\n\t")
                  : ""));
     message(StringRef("Framework search paths:") +
             (config->frameworkSearchPaths.size()
for (const auto &arg : args) {
     case OPT_dylib:
     case OPT_e:
     case OPT_F:
     case OPT_L:
     case OPT_headerpad:
     case OPT_install_name:
     case OPT_Z:
     case OPT_arch:
+    case OPT_syslibroot:
       // handled elsewhere
       break;
     default:
       warnIfUnimplementedOption(opt);
       break;
     }
   }
 

lld/test/MachO/search-paths.test

 UNSUPPORTED: darwin
 
 RUN: mkdir -p %t1 %t2
 
-RUN: lld -flavor darwinnew -v -L%t1 -F%t2 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 %s
+RUN: lld -flavor darwinnew -Z -v -L%t1 -F%t2 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 %s
 CHECK:      Library search paths:
 CHECK-NEXT: [[LDIR]]
 CHECK-NEXT: Framework search paths:
 CHECK-NEXT: [[FDIR]]
 
-RUN: lld -flavor darwinnew -v -L%t1 -F%t2 -Z 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 --check-prefix=CHECK_Z %s
+RUN: lld -flavor darwinnew -Z -v -L%t1 -F%t2 -Z 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 --check-prefix=CHECK_Z %s
 CHECK_Z:      Library search paths:
 CHECK_Z-NEXT: [[LDIR]]
 CHECK_Z-NEXT: Framework search paths:
 CHECK_Z-NEXT: [[FDIR]]

lld/test/MachO/syslibroot.test

+# Ensure that a nonexistent path is ignored with a syslibroot
+
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty | FileCheck %s -check-prefix CHECK-NONEXISTENT-SYSLIBROOT
+
+CHECK-NONEXISTENT-SYSLIBROOT: Library search paths:
+CHECK-NONEXISTENT-SYSLIBROOT-NEXT: Framework search paths:
+
+RUN: mkdir -p %t/usr/lib
+RUN: lld -flavor darwinnew -v -syslibroot %t | FileCheck %s -check-prefix CHECK-SYSLIBROOT -DROOT=%t
+
+CHECK-SYSLIBROOT: Library search paths:
+CHECK-SYSLIBROOT-NEXT: [[ROOT]]/usr/lib
+
+RUN: mkdir -p %t/Library/libxml2-development
+RUN: lld -flavor darwinnew -v -syslibroot %t -L /Library/libxml2-development | FileCheck %s -check-prefix CHECK-ABSOLUTE-PATH-REROOTED -DROOT=%t
+
+CHECK-ABSOLUTE-PATH-REROOTED: Library search paths:
+CHECK-ABSOLUTE-PATH-REROOTED: [[ROOT]]/Library/libxml2-development
+CHECK-ABSOLUTE-PATH-REROOTED: [[ROOT]]/usr/lib
+
+# NOTE: the match here is fuzzy because the default search paths exist on Linux
+# and macOS, but not on Windows (that is we ignore `/var/empty`).  This allows
+# us to run the test uniformly on all the platforms.
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty -syslibroot / 2>&1 | FileCheck %s -check-prefix CHECK-SYSLIBROOT-IGNORED
+
+CHECK-SYSLIBROOT-IGNORED: /usr/lib
+CHECK-SYSLIBROOT-IGNORED: /usr/local/lib
+
+RUN: mkdir -p %t.2/usr/lib
+RUN: lld -flavor darwinnew -v -syslibroot %t -syslibroot %t.2 | FileCheck %s -check-prefix CHECK-SYSLIBROOT-MATRIX -DROOT=%t
+
+CHECK-SYSLIBROOT-MATRIX: Library search paths:
+CHECK-SYSLIBROOT-MATRIX: [[ROOT]]/usr/lib
+CHECK-SYSLIBROOT-MATRIX: [[ROOT]].2/usr/lib
+
+RUN: mkdir -p %t/System/Library/Frameworks
+RUN: lld -flavor darwinnew -v -syslibroot %t | FileCheck %s -check-prefix CHECK-SYSLIBROOT-FRAMEWORK -DROOT=%t
+
+CHECK-SYSLIBROOT-FRAMEWORK: Framework search paths:
+CHECK-SYSLIBROOT-FRAMEWORK: [[ROOT]]/System/Library/Frameworks
+
+# NOTE: the match here is fuzzy because the default search paths exist on Linux
+# and macOS, but not on Windows (that is we ignore `/var/empty`).  This allows
+# us to run the test uniformly on all the platforms.
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty -syslibroot / 2>&1 | FileCheck %s -check-prefix CHECK-SYSLIBROOT-FRAMEWORK-IGNORED
+
+CHECK-SYSLIBROOT-FRAMEWORK-IGNORED: /System/Library/Framework
+
+RUN: mkdir -p %t/Library/Frameworks
+RUN: mkdir -p %t.2/Library/Frameworks
+RUN: lld -flavor darwinnew -v -syslibroot %t -syslibroot %t.2 -F /Library/Frameworks | FileCheck %s -check-prefix CHECK-SYSLIBROOT-FRAMEWORK-MATRIX -DROOT=%t
+
+CHECK-SYSLIBROOT-FRAMEWORK-MATRIX: Framework search paths:
+CHECK-SYSLIBROOT-FRAMEWORK-MATRIX: [[ROOT]]/Library/Frameworks
+CHECK-SYSLIBROOT-FRAMEWORK-MATRIX: [[ROOT]].2/Library/Frameworks