[lld-macho] Handle framework search path, alongside library search path

[gkm]

Summary

Add front-end support for lld::macho::Configuration::frameworkSearchPath.

Depends on D80582.

[int3]

lld/MachO/Config.h

33

maybe add a TODO to indicate this isn't currently used

lld/MachO/Driver.cpp

114

not a big deal, but how about erasing the invalid paths from the list?

though if we aren't doing that we should take paths by const ref at least

lld/test/MachO/search-paths.test

18

nit: do we really need 8 paths? wouldn't a test with one or two library/framework paths respectively be sufficient?

[gkm]

lld/MachO/Config.h

33

Si, señor

lld/MachO/Driver.cpp

114

You are quite correct that it is bone-headed to keep excreta in the search path.

lld/test/MachO/search-paths.test

18

I got kinda carried away. I will show more restraint ...

[gkm]

lld/MachO/Driver.cpp

114

You are quite correct that it is bone-headed to keep excreta in the search path. ld64 removes bogus paths.

Problem: in a Linux-hosted environment, the Appleish /Library/Frameworks and /System/Library/Frameworks do not exist. There is no benefit to keeping them, but issuing warnings about them seems rude. Moreover, there will be host-OS-specific deviation in expected test results. How do we handle that? There must be other corners of the toolchain that encounter these problems. I will dig.

[gkm]

• Use system library and frameworks search paths only Darwin hosts
• Only push good directories onto library and frameworks search paths
• Make better & simpler tests

[int3]

lld/MachO/Driver.cpp

363–365

this seems reasonable to me (and it looks like lld-ELF has isOS checks too) but cc @MaskRay @ruiu just in case

370

would StringRef work here instead of std::string?

[gkm]

rebase

[gkm]

lld/MachO/Driver.cpp

114

I predicate Frameworks paths on host OS = Darwin: Triple(sys::getProcessTriple()).isOSDarwin().

[gkm]

• incorporate review feedback

[int3]

minor nit, I'm fine if you prefer to keep things the way they are right now though

lld/MachO/Driver.cpp

125–157

nit: could we factor out the common logic into a helper function, something like getSearchPaths(const std::vector<StringRef>& argPaths, const std::vector<StringRef>& sysDefaultPaths)?

[gkm]

• factorcommon code into getSearchPaths()

[gkm]

• swap args of getLibrarySearchPaths() and getFrameworkSearchPaths() to match getSearchPaths()

[int3]

lld/MachO/Driver.cpp

114–131

nit: I think these make more sense as plain StringRefs instead of StringRef & since a ref-of-ref doesn't really seem useful

143

nit: this can just a ctor call instead of an assignment

(actually the local variable itself isn't necessary if we take the parameter by const ref)

[gkm]

lld/MachO/Driver.cpp

143

What is the syntax for eliding the local variable?

[int3]

lld/MachO/Driver.cpp

143

ah, it looks like SmallVectorImpl doesn't have a ctor that takes an initializer list, but making the parameter a const SmallVectorImpl<StringRef, 2> & works

[int3]

lld/MachO/Driver.cpp

143

Sorry, I meant const SmallVector<StringRef, 2> &

lld/MachO/Config.h

 
 struct Configuration {
   Symbol *entry;
   bool hasReexports = false;
   llvm::StringRef installName;
   llvm::StringRef outputFile;
   llvm::MachO::Architecture arch;
   llvm::MachO::HeaderFileType outputType;
-  std::vector<llvm::StringRef> searchPaths;
+  std::vector<llvm::StringRef> librarySearchPaths;
+  // TODO: use the framework search paths
+  std::vector<llvm::StringRef> frameworkSearchPaths;
   llvm::DenseMap<llvm::StringRef, SymbolPriorityEntry> priorities;
 };
 
 // The symbol with the highest priority should be ordered first in the output
 // section (modulo input section contiguity constraints). Using priority
 // (highest first) instead of order (lowest first) has the convenient property
 // that the default-constructed zero priority -- for symbols/sections without a
 // user-defined order -- naturally ends up putting them at the end of the

lld/MachO/Driver.cpp

 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::sys;
 using namespace llvm::opt;
 using namespace lld;

 }
 
 static Optional<std::string> findLibrary(StringRef name) {
   std::string stub = (llvm::Twine("lib") + name + ".tbd").str();
   std::string shared = (llvm::Twine("lib") + name + ".dylib").str();
   std::string archive = (llvm::Twine("lib") + name + ".a").str();
   llvm::SmallString<260> location;
 
-  for (StringRef dir : config->searchPaths) {
+  for (StringRef dir : config->librarySearchPaths) {
     for (StringRef library : {stub, shared, archive}) {
       location = dir;
       llvm::sys::path::append(location, library);
       if (fs::exists(location))
         return location.str().str();
     }
   }
   return {};
 }
 
 static TargetInfo *createTargetInfo(opt::InputArgList &args) {
   StringRef arch = args.getLastArgValue(OPT_arch, "x86_64");
   config->arch = llvm::MachO::getArchitectureFromName(
       args.getLastArgValue(OPT_arch, arch));
   switch (config->arch) {
   case llvm::MachO::AK_x86_64:
   case llvm::MachO::AK_x86_64h:
     return createX86_64TargetInfo();
   default:
     fatal("missing or unsupported -arch " + arch);
   }
 }
 
-static std::vector<StringRef> getSearchPaths(opt::InputArgList &args) {
-  std::vector<StringRef> ret{args::getStrings(args, OPT_L)};
-  if (!args.hasArg(OPT_Z)) {
-    ret.push_back("/usr/lib");
-    ret.push_back("/usr/local/lib");
+static bool isDirectory(StringRef option, StringRef path) {
+  if (!fs::exists(path)) {
+    warn("directory not found for option -" + option + path);
+    return false;
+  } else if (!fs::is_directory(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 SmallVector<StringRef, 2> &systemPaths) {
+  StringRef optionLetter{(optionCode == OPT_F ? "F" : "L")};
+  for (auto const &path : args::getStrings(args, optionCode)) {
+    if (isDirectory(optionLetter, path))
+      paths.push_back(path);
   }
-  return ret;
+  if (!args.hasArg(OPT_Z) && Triple(sys::getProcessTriple()).isOSDarwin()) {
+    for (auto const &path : systemPaths) {
+      if (isDirectory(optionLetter, path))
+        paths.push_back(path);
+    }
+  }
+}
+
+static void getLibrarySearchPaths(std::vector<StringRef> &paths,
+                                  opt::InputArgList &args) {
+  getSearchPaths(paths, OPT_L, args, {"/usr/lib", "/usr/local/lib"});
+}
+
+static void getFrameworkSearchPaths(std::vector<StringRef> &paths,
+                                    opt::InputArgList &args) {
+  getSearchPaths(paths, OPT_F, args,
+                 {"/Library/Frameworks", "/System/Library/Frameworks"});
 }
 
 static void addFile(StringRef path) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
 
   switch (identify_magic(mbref.getBuffer())) {
   case file_magic::archive: {
     std::unique_ptr<object::Archive> file = CHECK(
         object::Archive::create(mbref), path + ": failed to parse archive");
 
     if (!file->isEmpty() && !file->hasSymbolTable())
       error(path + ": archive has no index; run ranlib to add one");
 
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);
-  config->searchPaths = getSearchPaths(args);
+  getLibrarySearchPaths(config->librarySearchPaths, args);
+  getFrameworkSearchPaths(config->frameworkSearchPaths, args);
   config->outputType = args.hasArg(OPT_dylib) ? MH_DYLIB : MH_EXECUTE;
 
   if (args.hasArg(OPT_v)) {
     message(getLLDVersion());
-    std::vector<StringRef> &searchPaths = config->searchPaths;
-    message("Library search paths:\n" +
-            llvm::join(searchPaths.begin(), searchPaths.end(), "\n"));
+    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")
+                 : ""));
     freeArena();
     return !errorCount();
   }
 
   for (const auto &arg : args) {
     const auto &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     switch (arg->getOption().getID()) {

lld/test/MachO/search-paths.test

-RUN: mkdir -p %t
+UNSUPPORTED: darwin
 
-RUN: lld -flavor darwinnew -v -L%t 2>&1 | FileCheck -DDIR=%t %s
+RUN: mkdir -p %t1 %t2
+
+RUN: lld -flavor darwinnew -v -L%t1 -F%t2 2>&1 | FileCheck -DLDIR=%t1 -DFDIR=%t2 %s
 CHECK:      Library search paths:
-CHECK-NEXT: [[DIR]]
-CHECK-NEXT: /usr/lib
-CHECK-NEXT: /usr/local/lib
+CHECK-NEXT: [[LDIR]]
+CHECK-NEXT: Framework search paths:
+CHECK-NEXT: [[FDIR]]
 
-RUN: lld -flavor darwinnew -v -L%t -Z 2>&1 | FileCheck -DDIR=%t --check-prefix=CHECK_Z %s
+RUN: lld -flavor darwinnew -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: [[DIR]]
-CHECK_Z-NOT:  /usr/
+CHECK_Z-NEXT: [[LDIR]]
+CHECK_Z-NEXT: Framework search paths:
+CHECK_Z-NEXT: [[FDIR]]