[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

31

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

lld/MachO/Driver.cpp

103

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

6

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

31

Si, señor

lld/MachO/Driver.cpp

103

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

lld/test/MachO/search-paths.test

6

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

[gkm]

lld/MachO/Driver.cpp

103

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

420–423

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

428

would StringRef work here instead of std::string?

[gkm]

rebase

[gkm]

lld/MachO/Driver.cpp

103

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

114–146

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

103–145

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

157

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

157

What is the syntax for eliding the local variable?

[int3]

lld/MachO/Driver.cpp

157

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

157

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

lld/MachO/Config.h

 struct SymbolPriorityEntry;
 
 struct Configuration {
   Symbol *entry;
   bool hasReexports = false;
   llvm::StringRef installName;
   llvm::StringRef outputFile;
   llvm::MachO::HeaderFileType outputType;
-  std::vector<llvm::StringRef> searchPaths;
+  // TODO: use the library and framework search paths
+  std::vector<llvm::StringRef> librarySearchPaths;
+  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"
 
 #include <unordered_map>
 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::sys;

 void MachOOptTable::printHelp(const char *argv0) {
   PrintHelp(lld::outs(), (std::string(argv0) + " [options] file...").c_str(),
             "LLVM Linker", false);
   lld::outs() << "\n";
 }
 
 // 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) {
+  for (StringRef dir : config->librarySearchPaths) {
     std::string path = (dir + "/lib" + name + ".dylib").str();
     if (fs::exists(path))
       return path;
   }
   error("library not found for -l" + name);
   return None;
 }
 
 static TargetInfo *createTargetInfo(opt::InputArgList &args) {
   StringRef s = args.getLastArgValue(OPT_arch, "x86_64");
   if (s != "x86_64")
     error("missing or unsupported -arch " + s);
   return createX86_64TargetInfo();
 }
 
-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 std::vector<StringRef> getLibrarySearchPaths(opt::InputArgList &args,
+                                                    bool isOSDarwin) {
+  std::vector<StringRef> paths;
+  for (auto const &path : args::getStrings(args, OPT_L)) {
+    if (isDirectory("L", path))
+      paths.push_back(path);
+  }
+  if (isOSDarwin && !args.hasArg(OPT_Z)) {
+    StringRef usrLib("/usr/lib");
+    StringRef usrLocalLib("/usr/local/lib");
+    if (isDirectory("L", usrLib))
+      paths.push_back(usrLib);
+    if (isDirectory("L", usrLocalLib))
+      paths.push_back(usrLocalLib);
+  }
+  return paths;
+}
+
+static std::vector<StringRef> getFrameworkSearchPaths(opt::InputArgList &args,
+                                                      bool isOSDarwin) {
+  std::vector<StringRef> paths;
+  for (auto const &path : args::getStrings(args, OPT_F)) {
+    if (isDirectory("F", path))
+      paths.push_back(path);
+  }
+  if (isOSDarwin && !args.hasArg(OPT_Z)) {
+    StringRef libFrames("/Library/Frameworks");
+    StringRef sysLibFrames("/System/Library/Frameworks");
+    if (isDirectory("F", libFrames))
+      paths.push_back(libFrames);
+    if (isDirectory("F", sysLibFrames))
+      paths.push_back(sysLibFrames);
   }
-  return ret;
+  return paths;
 }
 
 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");
 
     inputFiles.push_back(make<ArchiveFile>(std::move(file)));
     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);
-  config->searchPaths = getSearchPaths(args);
+  bool isOSDarwin = Triple(sys::getProcessTriple()).isOSDarwin();
+  config->librarySearchPaths = getLibrarySearchPaths(args, isOSDarwin);
+  config->frameworkSearchPaths = getFrameworkSearchPaths(args, isOSDarwin);
   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("Library search paths:" +
+            std::string(config->librarySearchPaths.size() ? "\n\t" : "") +
+            llvm::join(config->librarySearchPaths, "\n\t"));
+    message("Framework search paths:" +
+            std::string(config->frameworkSearchPaths.size() ? "\n\t" : "") +
+            llvm::join(config->frameworkSearchPaths, "\n\t"));
     freeArena();
     return !errorCount();
   }
 
   for (auto &arg : args) {
     const auto &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     switch (opt.getID()) {
     case OPT_o:
     case OPT_dylib:
+    case OPT_L:
+    case OPT_F:
       // handled elsewhere ...
       break;
     default:
       warnIfUnimplementedOption(opt);
       break;
     case OPT_INPUT:
       addFile(arg->getValue());
       break;

lld/test/MachO/search-paths.test

-RUN: mkdir -p %t
+RUN: rm -fr %t1 %t2 %t3
+RUN: mkdir -p %t1
+RUN: touch %t2
+
+RUN: not lld -flavor darwinnew -L%t1 -L%t2 -L%t3 2>&1 \
+RUN:     | FileCheck --check-prefix=CHECK_LF -DT1=%t1 -DT2=%t2 -DT3=%t3 %s
+RUN: not lld -flavor darwinnew -L %t1 -L %t2 -L %t3 2>&1 \
+RUN:     | FileCheck --check-prefix=CHECK_LF -DT1=%t1 -DT2=%t2 -DT3=%t3 %s
+RUN: not lld -flavor darwinnew -F%t1 -F%t2 -F%t3 2>&1 \
+RUN:     | FileCheck --check-prefix=CHECK_LF -DT1=%t1 -DT2=%t2 -DT3=%t3 %s
+RUN: not lld -flavor darwinnew -F %t1 -F %t2 -F %t3 2>&1 \
+RUN:     | FileCheck --check-prefix=CHECK_LF -DT1=%t1 -DT2=%t2 -DT3=%t3 %s
+
+CHECK_LF: warning: option -{{[LF]}}[[T2]] references a non-directory path
+CHECK_LF-NEXT: warning: directory not found for option -{{[LF]}}[[T3]]
+CHECK_LF-NEXT: undefined symbol: _main
+
+RUN: lld -flavor darwinnew -v -L %t1 2>&1 \
+RUN:     | FileCheck --check-prefix=CHECK_vL -DT1=%t1 %s
+
+CHECK_vL: LLD {{[0-9.]+ .*}}
+CHECK_vL-NEXT: Library search paths:
+CHECK_vL-NEXT: [[T1]]
+CHECK_vL-NEXT: Framework search paths:
+
+RUN: lld -flavor darwinnew -v -F %t1 2>&1 \
+RUN:     | FileCheck --check-prefix=CHECK_vF -DT1=%t1 %s
+
+CHECK_vF: LLD {{[0-9.]+ .*}}
+CHECK_vF-NEXT: Library search paths:
+CHECK_vF-NEXT: Framework search paths:
+CHECK_vF-NEXT: [[T1]]
 
-RUN: lld -flavor darwinnew -v -L%t 2>&1 | FileCheck -DDIR=%t %s
-CHECK:      Library search paths:
-CHECK-NEXT: [[DIR]]
-CHECK-NEXT: /usr/lib
-CHECK-NEXT: /usr/local/lib
-
-RUN: lld -flavor darwinnew -v -L%t -Z 2>&1 | FileCheck -DDIR=%t --check-prefix=CHECK_Z %s
-CHECK_Z:      Library search paths:
-CHECK_Z-NEXT: [[DIR]]
-CHECK_Z-NOT:  /usr/