[ELF2] Add support for -L and -l command line switches

[ikudrin]

Summary

Just a basic variant.

[• rafael]

ELF/Driver.cpp

63

Just emit the error from this function instead of returning ErrorOr

65

I don't think it is safe to use Twine link this:

http://llvm.org/docs/ProgrammersManual.html#the-twine-class

197

Not much value in adding this TODO now :-)

ELF/Driver.h

40 ↗

(On Diff #35625)

prepare is a bit too generic.

How about something like openInputFiles?

42 ↗

(On Diff #35625)

These 2 can be static helpers.

test/elf2/libsearch.s

21

You don't actually need any instructions in the test.

[ruiu]

ELF/Driver.cpp

63

Please write a brief function comment saying that this function searches a given file path from paths specified with -L.

65

I'd use shorter name -- StaticName and DynamicName would be probably enough.

136

Most command line options are handled directly inside this function for clarity, and I'd think this should be done in the same way rather than delegating a task to a different function. We could do like this here.

for (auto *Arg : Args.filtered(OPT_l, OPT_INPUT)) {
  if (OPT_l) {
    Inputs.push_back(openLibrary(Arg->getValue());
    continue;
  }
  Inputs.push_back(openObject(Arg->getValue());
}

[ikudrin]

• Rebased to the top
• Addressed review comments

[ikudrin]

ELF/Driver.cpp

63

The function will be used later for search libraries from handler of linker scripts. Error messages should be different in that case.

136

The loop will be extended to handle additional switches like -T, --no-whole-archive, --whole-archive, --as-needed, --no-as-needed and so on. I think that a smaller function for a specific subtask is a bit more comprehensible than one big function which does everything.

197

The comment clarifies the existence of the method with only one line of code within.

ELF/Driver.h

40 ↗

(On Diff #35625)

Accepted. Thanks!

42 ↗

(On Diff #35625)

They can't as they use the openFile() method.

test/elf2/libsearch.s

20

Thanks. I wonder, if we still need the 'REQUIRES: x86' instruction here?

[denis-protivensky]

ELF/Driver.cpp

72

Maybe better to have SmallVector<SmallString<64>, 2> Libs instead?
This solves couple of problems:

1. Removes ambiguity when HasColonPrefix == true, because you'll put just one name into the vector instead of having two equal names like now.
2. Removes code duplication in the search dir loop, because you'll have both dynamic and static names in the vector and will iterate over them in the unified way.
3. When -static is added, you won't need to make complex checks or code changes. Just check for static flag and don't add dynamic name to the vector.

95

This check is not correct. Colon prefix affects only name being searched, but it doesn't allow to specify absolute name which should be used.

190

You need a negative test for this error case.

test/elf2/libsearch.s

9

Need to check that lld -flavor gnu2 -o %t3 -L%T -lls %t.o also fails with undefined symbol.

12

Check also this combination as correct: lld -flavor gnu2 -o %t3 %t.o -lls -L%T

[ikudrin]

• Reworked the function searchLibrary
• Added two test cases

[ikudrin]

ELF/Driver.cpp

72

Thanks! I think the function is much better now.

89

Thanks for pointing it out. The logic was copied from the old ELF driver, but I agree that it's better to follow the behaviour of ld here.

test/elf2/libsearch.s

10

It won't fail with the current approach to symbol resolution.

[denis-protivensky]

ELF/Driver.cpp

77

Looks like you broke the logic a bit: you must put .so file first to check for it first.

This also means we don't have a test case to cover order of picking DSOs before archive files.
You may do lld -flavor gnu2 -o %t3 %t.o -L%T -lls command before creating archive file in the libsearch.s test and ensure no errors produced.

[• rafael]

ELF/Driver.cpp

70

Do not return a ErrorOr. If we ever have to handle it differently, we will do evaluate how when needed.

For now just call error from within this function.

[• rafael]

ELF/Driver.cpp

197

The TODO is not relevant to this patch. Please leave it out.

[ruiu]

This patch doesn't seem very consistent with existing code in minor points such as naming convention, error handling, use of TODO, or notation used in comments. Could you please check other code and mimic what they do?

ELF/Driver.cpp

136

At least for now, we don't need that separate function. We can create any function when we need it. Please keep it in-line with other code.

ELF/Driver.h

43–45 ↗

(On Diff #35715)

These function names are too generic. Please name them after what they actually do.

[ikudrin]

• Fixed priority in library search. The corresponding test is added.
• Simplified the code by removing additional methods and a composite return value.

[ikudrin]

In D13135#253473, @ruiu wrote:

This patch doesn't seem very consistent with existing code in minor points such as naming convention, error handling, use of TODO, or notation used in comments. Could you please check other code and mimic what they do?

Thank you for your comments! I tried my best to follow the current style, but if I miss something I'll be very pleased to be pointed to.

[ruiu]

ELF/Driver.cpp

66

Better to use std::vector<std::string> because Twine::str() returns an std::string. Converting that to SmallString (through StringRef) is a bit odd.

75

Please write the real type instead of auto if the real type is not apparent in a very narrow context (for example, on RHS).

82

error(Twine("Unable to find library -l") + Path)

(because we wrote like this in other places.)

132–133

These two lines can be written in one line.

[ikudrin]

Addressed latest comments.

[ruiu]

LGTM with this nit. Hold on for a while for other reviewers. Thanks!

ELF/Driver.cpp

70

Please do not try to optimize unless proved to be necessary. I'm pretty sure that this is marginal or even slightly negative.

[ikudrin]

Remove premature optimization.

[• rafael]

LGTM.

[ikudrin]

r248708

ELF/Config.h

 
 namespace lld {
 namespace elf2 {
 
 struct Configuration {
   llvm::StringRef OutputFile;
   llvm::StringRef DynamicLinker;
   std::string RPath;
+  std::vector<llvm::StringRef> InputSearchPaths;
   bool Shared = false;
   bool DiscardAll = false;
   bool DiscardLocals = false;
   bool DiscardNone = false;
   bool ExportDynamic = false;
   bool NoInhibitExec = false;
 };
 
 extern Configuration *Config;
 
 } // namespace elf2
 } // namespace lld
 
 #endif

ELF/Driver.cpp

 #include "Config.h"
 #include "Error.h"
 #include "InputFiles.h"
 #include "SymbolTable.h"
 #include "Writer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 
 using namespace llvm;
 
 using namespace lld;
 using namespace lld::elf2;
 
 namespace lld {
 namespace elf2 {
if (Magic == file_magic::archive)
     return make_unique<ArchiveFile>(MB);
 
   if (Magic == file_magic::elf_shared_object)
     return createELFFile<SharedFile>(MB);
 
   return createELFFile<ObjectFile>(MB);
 }
 
+// Searches a given library from input search paths, which are filled
+// from -L command line switches. Returns a path to an existent library file.
+static std::string searchLibrary(StringRef Path) {
+  std::vector<std::string> Names;
+  if (Path[0] == ':') {
+    Names.push_back(Path.drop_front().str());
+  } else {
+    Names.push_back((Twine("lib") + Path + ".so").str());
+    Names.push_back((Twine("lib") + Path + ".a").str());
+  }
+  SmallString<128> FullPath;
+  for (StringRef Dir : Config->InputSearchPaths) {
+    for (const std::string &Name : Names) {
+      FullPath = Dir;
+      sys::path::append(FullPath, Name);
+      if (sys::fs::exists(FullPath.str()))
+        return FullPath.str().str();
+    }
+  }
+  error(Twine("Unable to find library -l") + Path);
+}
+
 void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Parse command line options.
   opt::InputArgList Args = Parser.parse(ArgsArr);
 
   // Handle -o
   if (auto *Arg = Args.getLastArg(OPT_output))
     Config->OutputFile = Arg->getValue();
   if (Config->OutputFile.empty())
     error("-o must be specified.");
 
   // Handle -dynamic-linker
   if (auto *Arg = Args.getLastArg(OPT_dynamic_linker))
     Config->DynamicLinker = Arg->getValue();
 
   std::vector<StringRef> RPaths;
   for (auto *Arg : Args.filtered(OPT_rpath))
     RPaths.push_back(Arg->getValue());
   if (!RPaths.empty())
     Config->RPath = llvm::join(RPaths.begin(), RPaths.end(), ":");
 
+  for (auto *Arg : Args.filtered(OPT_L))
+    Config->InputSearchPaths.push_back(Arg->getValue());
+
   if (Args.hasArg(OPT_shared))
     Config->Shared = true;
 
   if (Args.hasArg(OPT_discard_all))
     Config->DiscardAll = true;
 
   if (Args.hasArg(OPT_discard_locals))
     Config->DiscardLocals = true;
 
   if (Args.hasArg(OPT_discard_none))
     Config->DiscardNone = true;
 
   if (Args.hasArg(OPT_export_dynamic))
     Config->ExportDynamic = true;
 
   if (Args.hasArg(OPT_noinhibit_exec))
     Config->NoInhibitExec = true;
 
   // Create a list of input files.
   std::vector<MemoryBufferRef> Inputs;
 
-  for (auto *Arg : Args.filtered(OPT_INPUT)) {
+  for (auto *Arg : Args.filtered(OPT_l, OPT_INPUT)) {
     StringRef Path = Arg->getValue();
+    if (Arg->getOption().getID() == OPT_l) {
+      Inputs.push_back(openFile(searchLibrary(Path)));
+      continue;
+    }
     Inputs.push_back(openFile(Path));
   }
 
   if (Inputs.empty())
     error("no input files.");
 
   // Create a symbol table.
   SymbolTable Symtab;
 
   // Parse all input files and put all symbols to the symbol table.
case ELF32BEKind:
     return;
   case ELF64LEKind:
     writeResult<object::ELF64LE>(&Symtab);
     return;
   case ELF64BEKind:
     writeResult<object::ELF64BE>(&Symtab);
     return;
   }
 }

ELF/Options.td

def noinhibit_exec : Flag<["--"], "noinhibit-exec">,
      HelpText<"Retain the executable output file whenever"
               " it is still usable">;
 
 def discard_none : Flag<["-"], "discard-none">,
      HelpText<"Keep all symbols in the symbol table">;
 
 def export_dynamic : Flag<["--"], "export-dynamic">,
      HelpText<"Put symbols in the dynamic symbol table">;
+
+def L : Joined<["-"], "L">, MetaVarName<"<dir>">,
+     HelpText<"Directory to search for libraries">;
+
+def alias_L : Joined<["--"], "library-path=">,
+     Alias<L>;
+
+def l : Joined<["-"], "l">, MetaVarName<"<libName>">,
+     HelpText<"Root name of library to use">;
+
+def alias_l : Joined<["--"], "library=">,
+     Alias<l>;

test/elf2/Inputs/libsearch-dyn.s

+.globl _bar,_dynamic;
+_bar:
+_dynamic:

test/elf2/Inputs/libsearch-st.s

+.globl _bar,_static;
+_bar:
+_static:

test/elf2/libsearch.s

+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+// RUN:     %p/Inputs/libsearch-dyn.s -o %tdyn.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux \
+// RUN:     %p/Inputs/libsearch-st.s -o %tst.o
+// RUN: lld -flavor gnu2 -shared %tdyn.o -o %T/libls.so
+// RUN: rm -f %T/libls.a
+// RUN: llvm-ar rcs %T/libls.a %tst.o
+// REQUIRES: x86
+
+// Should not link because of undefined symbol _bar
+// RUN: not lld -flavor gnu2 -o %t3 %t.o 2>&1 \
+// RUN:     | FileCheck --check-prefix=UNDEFINED %s
+// UNDEFINED: undefined symbol: _bar
+
+// Should fail if cannot find specified library (without -L switch)
+// RUN: not lld -flavor gnu2 -o %t3 %t.o -lls 2>&1 \
+// RUN:     | FileCheck --check-prefix=NOLIB %s
+// NOLIB: Unable to find library -lls
+
+// Should use explicitly specified static library
+// RUN: lld -flavor gnu2 -o %t3 %t.o -L%T -l:libls.a
+// RUN: llvm-readobj --symbols %t3 | FileCheck --check-prefix=STATIC %s
+// STATIC: Symbols [
+// STATIC: Name: _static
+// STATIC: ]
+
+// Should use explicitly specified dynamic library
+// RUN: lld -flavor gnu2 -o %t3 %t.o -L%T -l:libls.so
+// RUN: llvm-readobj --symbols %t3 | FileCheck --check-prefix=DYNAMIC %s
+// DYNAMIC: Symbols [
+// DYNAMIC-NOT: Name: _static
+// DYNAMIC: ]
+
+// Should prefer dynamic to static
+// RUN: lld -flavor gnu2 -o %t3 %t.o -L%T -lls
+// RUN: llvm-readobj --symbols %t3 | FileCheck --check-prefix=DYNAMIC %s
+
+// -L can be placed after -l
+// RUN: lld -flavor gnu2 -o %t3 %t.o -lls -L%T
+
+// Check long forms as well
+// RUN: lld -flavor gnu2 -o %t3 %t.o --library-path=%T --library=ls
+
+.globl _start,_bar;
+_start: