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

136

any particular reason why you picked 261?

147

suppresses

lld/test/MachO/syslibroot.test

1

s/non-existant/nonexistent/

3

s/NONEXISTANT/NONEXISTENT/

14

nit: could we have a shorter tmp directory name?

15

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

18

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

136

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

[compnerd]

lld/test/MachO/syslibroot.test

14

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

134

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

136

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

135

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

138

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

439

avoid auto

[int3]

search-paths.test needs to be updated too

[compnerd]

lld/MachO/Driver.cpp

134

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

135

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

136

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

138

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

439

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

134

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 😅)

136

sgtm

138

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

134

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 (auto root : roots) {
+        SmallString<261> buffer(root);
+        llvm::sys::path::append(buffer, path);
+        if (isDirectory(optionLetter, buffer.str()))
+          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` supresses 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;
static void warnIfUnimplementedOption(const opt::Option &opt) {
   case OPT_grp_undocumented:
     warn("Option `" + opt.getPrefixedName() +
          "' is undocumented. Should lld implement it?");
     break;
   case OPT_grp_obsolete:
     warn("Option `" + opt.getPrefixedName() +
          "' is obsolete. Please modernize your usage.");
     break;
   case OPT_grp_ignored:
     warn("Option `" + opt.getPrefixedName() + "' is ignored.");
     break;
   default:
     warn("Option `" + opt.getPrefixedName() +
          "' is not yet implemented. Stay tuned...");
     break;
   }
 }
bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
   config = make<Configuration>();
   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);
-  getLibrarySearchPaths(config->librarySearchPaths, args);
-  getFrameworkSearchPaths(config->frameworkSearchPaths, args);
+
+  std::vector<StringRef> roots;
+  for (const auto 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);
   config->outputType = args.hasArg(OPT_dylib) ? MH_DYLIB : MH_EXECUTE;
 
   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()
                  ? "\n\t" + llvm::join(config->frameworkSearchPaths, "\n\t")
                  : ""));
case OPT_platform_version:
       handlePlatformVersion(arg);
       break;
     case OPT_o:
     case OPT_dylib:
     case OPT_e:
     case OPT_L:
     case OPT_Z:
     case OPT_arch:
+    case OPT_syslibroot:
       // handled elsewhere
       break;
     default:
       warnIfUnimplementedOption(opt);
       break;
     }
   }
 

lld/test/MachO/syslibroot.test

+# Ensure that a non-existant path is ignored with a syslibroot
+
+RUN: lld -flavor darwinnew -v -syslibroot /var/empty | FileCheck %s -check-prefix CHECK-NONEXISTANT-SYSLIBROOT
+
+CHECK-NONEXISTANT-SYSLIBROOT: Library search paths:
+CHECK-NONEXISTANT-SYSLIBROOT-NEXT: Framework search paths:
+
+RUN: mkdir -p %t/usr/lib
+RUN: lld -flavor darwinnew -v -syslibroot %t | FileCheck %s -check-prefix CHECK-SYSLIBROOT
+
+CHECK-SYSLIBROOT: Library search paths:
+CHECK-SYSLIBROOT-NEXT: {{.+}}/usr/lib
+
+RUN: mkdir -p %t/Users/compnerd/Library/libxml2-development
+RUN: lld -flavor darwinnew -v -syslibroot %t -L /Users/compnerd/Library/libxml2-development | FileCheck %s -check-prefix CHECK-ABSOLUTE-PATH-REROOTED
+
+CHECK-ABSOLUTE-PATH-REROOTED: Library search paths:
+CHECK-ABSOLUTE-PATH-REROOTED: {{.+}}/Users/compnerd/Library/libxml2-development
+CHECK-ABSOLUTE-PATH-REROOTED: {{.+}}/usr/lib
+
+# NOTE: the match here is fuzzy because the default search paths exist on Linux
+# and macOS, but not on Windows.
+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
+
+CHECK-SYSLIBROOT-MATRIX: Library search paths:
+CHECK-SYSLIBROOT-MATRIX: {{.+}}/usr/lib
+CHECK-SYSLIBROOT-MATRIX: {{.+}}/usr/lib
+
+RUN: mkdir -p %t/System/Library/Frameworks
+RUN: lld -flavor darwinnew -v -syslibroot %t | FileCheck %s -check-prefix CHECK-SYSLIBROOT-FRAMEWORK
+
+CHECK-SYSLIBROOT-FRAMEWORK: Framework search paths:
+CHECK-SYSLIBROOT-FRAMEWORK: {{.+}}/System/Library/Frameworks
+
+# NOTE: the match here is fuzzy because the default search paths exist on Linux
+# and macOS, but not on Windows.
+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/Users/compnerd/Library/Frameworks
+RUN: mkdir -p %t.2/Users/compnerd/Library/Frameworks
+RUN: lld -flavor darwinnew -v -syslibroot %t -syslibroot %t.2 -F /Users/compnerd/Library/Frameworks | FileCheck %s -check-prefix CHECK-SYSLIBROOT-FRAMEWORK-MATRIX
+
+CHECK-SYSLIBROOT-FRAMEWORK-MATRIX: Framework search paths:
+CHECK-SYSLIBROOT-FRAMEWORK-MATRIX: {{.*}}/Users/compnerd/Library/Frameworks
+CHECK-SYSLIBROOT-FRAMEWORK-MATRIX: {{.*}}/Users/compnerd/Library/Frameworks