[llvm-libtool-darwin] Add support for -l and -L

[sameerarora101]

Summary

Add support for passing in libraries via -l and -L options. Depends
on D85334.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

2

It doesn't make sense to test -L option individually as the directory specified under that option is used (prepended) only if some library is also specified with option -l.

To test -l individually, I would need to create a library in one of the standard directories (/lib/, /usr/lib, /usr/local/lib). Is there a way I can do that in a test file? Thanks.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

84–88

I defined StandarDirs based on cctools' libtool:

/* the standard directories to search for -lx names */
char *standard_dirs[] = {
    "/lib/",
    "/usr/lib/",
    "/usr/local/lib/",
    NULL
};

[smeenai]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

77

How about LibrarySearchDirs?

84

Nit: I'd prefer a std::array. I think you can make it constexpr too?

I'd also prefer StandardSearchDirs.

85–87

I don't think you need the trailing slashes.

124

const &

[smeenai]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

2

I don't think so. Other tools have options (like --sysroot) to re-root the search, but it doesn't appear that libtool has anything like that, so I don't think we'll be able to test that case.

8

I'd prefer for a file ending in .o to actually be an object file and not an archive :)

86

Other test ideas:

• If multiple directories specified with -L have the same named file in them, the file from the first directory should be selected.
• You should be able to combine -l with input files specified normally (i.e. as paths).
• What happens if we specify the same -l option twice? (It's okay if it differs from cctools' behavior, but we should make a note.)

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

101

Any reason for creating this variable? SmallString can be implicitly converted to StringRef instead, and it can also be converted to std::string explicitly (e.g. via std::string(Path)).

103

Nit: you don't need the braces

110

The duplication between the two loops is unfortunate, but I don't think the body of the loops is large enough to justify moving it out into a function (especially if you get rid of the PathString variable). What we really want is a C++ equivalent to Python's itertools.chain, and I think we might get that with ranges in C++20.

125

One of the following should work, I think.

StringRef BaseName = s;
StringRef BaseName(s);

129

I would construct a Twine and have searchForFile take const Twine & instead.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

8

I just wanted to check that a library ending in .o is not prepended/appended with some additional string lol. How about the name archive-object.o?

86

Thanks, added now.

For the second test case, I assumed you meant passing in a library with -l along with a normal input file? Or did you mean prepending -l to an input file name?

For the third case, libtool throws a warning (similar to what it does when we specify the same input file twice). I have added a comment about that in my test case above now.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

101

Ah forgot about the explicit conversion, thanks!

125

Both work 😄

129

Seems like I can't construct a Twine before the if condition and modify it appropriately in the corresponding branches (which is needed so as to have the Twine variable in scope after the if condition). Furthermore, I also cannot directly call searchForFile(<Twine>); within each branch and assign the result to Expected<std::string> FullPath, as I cannot declare Expected<std::string> FullPath; before the if branch.

Sorry if I didn't get what you were trying to explain here ?

[sameerarora101]

Add more tests.

[smeenai]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

8

I think the intent is, even though the option is -l (with the l presumably standing for library), you can still use it to specify input object files (so that you don't have to specify their full path). What I meant was to just have an object file and use -l to add that, instead of creating an archive and giving it a .o extension.

86

Yup, that's what I meant for the second case.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

129

Hmm, good points. Lemme think if there's anything else we can do here.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

8

Ah I see. Ok, I'll update it.

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

8

You should place lines like this that are tied to an individual test-case, below the comment that goes with the case, as that explains the purpose of the line.

27

117

I might suggest changing the -l file's name here to something even more arbitrary such as "-lfile-will-not-exist". Since libtool searches certain directories by default, there's a (small) chance the file being searched for is in one of those directories. The weirder we make the name, the smaller this chance, and therefore the less likely this test case will fail on somebody's system.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

72–74

I just want to make sure the behaviour here is definitely what's documented and that it doesn't search for libfoo.o.a for -lfoo.o? We probably need a test case for that.

97

Could this be moved into the loop body, to avoid the Path.clear() line?

110

Perhaps a lambda could work okay here:

auto FindLib = [](ArrayRef<StringRef> SearchDirs) {
  for (StringRef Dir : SearchDirs) {
    SmallString<128> Path;
    sys::path::append(Path, Dir, FileName);

    if (sys::fs::exists(Path))
      return std::string(Path);
  }
  return "";
};

std::string Found = FindLib(LibrarySearchDirs);
if (Found.empty())
  Found = FindLib(StandardSearchDirs);
if (!Found.empty())
  return Found;
// report error

116

121

Can't you just do for(StringRef BaseName : Libraries)?

If not, at least do s -> S

129

How about something like:

Expected<std::string> FullPath = searchForFile(BaseName.endswith(".o") ?
                                               BaseName.str() :
                                               "lib" + BaseName + ".a");

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

72–74

Yup, the behavior here is definitely what's documented. My first test case is testing this only where I pass in -larchive-object.o. Also, I have now changed that test by passing in -l%basename_t.tmp-input1.o instead of archive-object as per @smeenai's recommendation above.

110

I like the lambda too, updated now. Thanks!

116

it is not trying to locate '%s' but might add lib and .a to the string and try locating lib%s.a, right? That's what I meant up there. I can change it to what you're suggesting if it doesn't make sense?

129

works, thanks.

[sameerarora101]

Use lambda function.

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

84

Seems like I cannot have constexpr with std::array<std::string, 3>. It says, "constexpr variable cannot have non-literal type 'const std::array<std::string, 3>' (aka 'const array<basic_string<char>, 3>')".

[smeenai]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

72–74

I think @jhenderson meant that we should also add a case where you have e.g. a file called libfoo.o.a, you pass -lfoo.o, and you make sure that you get an error about a missing file (i.e. confirming that when you have a -l argument ending in .o, you only search for the name as-is).

[sameerarora101]

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

72–74

Oh, i see now. I'll add a test. Thanks for clarifying!

[sameerarora101]

Add another test to check that files ending in '.o' are matched as-is.

[smeenai]

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

65

Nit: remove the trailing slash on /lib/

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

100

Can you test this for a name ending with .a as well?

102

This one too?

111

You need the -L%t/dirname as well, right?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

84

Ah, oh well.

106

I'd prefer an Optional<std::string> instead of having the empty string represent the absence.

116

FileName will have already had the lib and .a additions at this point though, right?

129

Ah, I keep forgetting about ternary expressions.

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

23

Maybe worth naming archives with a single member in them after the member's name, e.g. here libinput2.a

I think that helps readability, also possibly reusability (see below).

57–58

Rather than create two new archives here, you could just create one, in otherDirname, containing input1.o. You already have a library in dirname containing input2.o, so if you name the new one the same, you can use that one too.

109

Let's also add a test case with a file extension other than these two, e.g. .ext and show that the behaviour is specific to '.o'.

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

121

It's not particularly idiomatic to put const with StirngRef.

129

I avoid them in most cases, but this seems like a sensible usage.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

100

Did you mean for a file to which '.a' is appended, right? (The name itself doesn't end in '.a' but since the name doesn't end in '.o', 'lib' and '.a' are added to it)?

If that is what you meant, added a test for it now. Thanks.

111

yup, added. thanks

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

116

ah yes, you are right. Replaced as suggested.

[sameerarora101]

Add more tests.

[sameerarora101]

Rebase.

[jhenderson]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

96–99

One more test for this area: a file called something like file, and show that -lfile doesn't find it (since lib and .a are added). Relatedly, maybe also show that -llibfile.a looks for liblibfile.a not libfile.a?

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

96–99

Added now, thanks.

[sameerarora101]

Add 2 more tests for file not found.

[jhenderson]

No more comments from me, thanks!

[smeenai]

LGTM

[sameerarora101]

Rebase.

[jrtc27]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

96–99

This doesn't work if you do have a libfile.a, which is a real library that exists; on FreeBSD it comes as part of schilybase, and so having that package installed breaks this test.

[smeenai]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

96–99

Heh, fixed in rG22b5fe74782a

[jrtc27]

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

96–99

Great, thanks for the quick follow-up

llvm/docs/CommandGuide/llvm-libtool-darwin.rst

.. option:: -h, -help
 
   Show help and usage for this command.
 
 .. option:: -help-list
 
   Show help and usage for this command without grouping the options
   into categories.
 
+.. option:: -l <x>
+
+  Searches for the library libx.a in the library search path. If the string `<x>`
+  ends with '.o', then the library 'x' is searched for without prepending 'lib'
+  or appending '.a'. If the library is found, it is added to the list of input
+  files. Otherwise, an error is raised.
+
+.. option:: -L <dir>
+
+  Adds `<dir>` to the list of directories in which to search for libraries. The
+  directories are searched in the order in which they are specified with
+  :option:`-L` and before the default search path. The default search path
+  includes directories `/lib/`, `/usr/lib` and `/usr/local/lib`.
+
 .. option:: -o <filename>
 
   Specify the output file name. Must be specified exactly once.
 
 .. option:: -static
 
   Produces a static library from the input files.
 

llvm/test/tools/llvm-libtool-darwin/L-and-l.test

+## This test checks that -l and -L options work correctly.
+
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
+# RUN: yaml2obj %S/Inputs/input2.yaml -o %t-input2.o
+
+## Check that the library is recognised when it ends with '.o':
+# RUN: llvm-libtool-darwin -static -o %t.lib -l%basename_t.tmp-input1.o -l%basename_t.tmp-input2.o -L%T
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# CHECK-NAMES:      [[PREFIX]]-input1.o
+# CHECK-NAMES-NEXT: [[PREFIX]]-input2.o
+
+# CHECK-SYMBOLS:      Archive map
+# CHECK-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# CHECK-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# CHECK-SYMBOLS-EMPTY:
+
+## Check that the library is recognised when prepended with 'lib' and appended with '.a':
+# RUN: rm -rf %t/dirname && mkdir -p %t/dirname
+# RUN: llvm-ar cr %t/dirname/libsingle-archive.a %t-input2.o
+
+# RUN: llvm-libtool-darwin -static -o %t.lib -lsingle-archive -L%t/dirname
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=SINGLE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=SINGLE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# SINGLE-NAMES: [[PREFIX]]-input2.o
+
+# SINGLE-SYMBOLS:      Archive map
+# SINGLE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# SINGLE-SYMBOLS-EMPTY:
+
+## -l and -L option specified multiple times:
+# RUN: rm -rf %t/otherDirname && mkdir -p %t/otherDirname
+# RUN: llvm-ar cr %t/otherDirname/libother-single-archive.a %t-input1.o
+
+# RUN: llvm-libtool-darwin -static -o %t.lib -lsingle-archive -lother-single-archive -L%t/dirname -L%t/otherDirname
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=OTHER-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=OTHER-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# OTHER-NAMES:      [[PREFIX]]-input2.o
+# OTHER-NAMES-NEXT: [[PREFIX]]-input1.o
+
+# OTHER-SYMBOLS:      Archive map
+# OTHER-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# OTHER-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# OTHER-SYMBOLS-EMPTY:
+
+## Check that if multiple directories specified with -L have the same named file
+## in them, the file from the first directory is selected.
+# RUN: llvm-ar cr %t/dirname/libmultiple-dirs.a %t-input1.o
+# RUN: llvm-ar cr %t/otherDirname/libmultiple-dirs.a %t-input2.o
+
+# RUN: llvm-libtool-darwin -static -o %t.lib -lmultiple-dirs -L%t/dirname -L%t/otherDirname
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=MULTIPLE-DIR-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=MULTIPLE-DIR-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# MULTIPLE-DIR-NAMES: [[PREFIX]]-input1.o
+
+# MULTIPLE-DIR-SYMBOLS:      Archive map
+# MULTIPLE-DIR-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# MULTIPLE-DIR-SYMBOLS-EMPTY:
+
+## -l option combined with an input file:
+# RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o -lsingle-archive -L%t/dirname
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+## Specify same -l option twice:
+## cctools' libtool raises a warning in this case.
+## The warning is not yet implemented for llvm-libtool-darwin.
+# RUN: llvm-libtool-darwin -static -o %t.lib -l%basename_t.tmp-input2.o -l%basename_t.tmp-input2.o -L%T
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DOUBLE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DOUBLE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# DOUBLE-NAMES:      [[PREFIX]]-input2.o
+# DOUBLE-NAMES-NEXT: [[PREFIX]]-input2.o
+
+# DOUBLE-SYMBOLS:      Archive map
+# DOUBLE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# DOUBLE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# DOUBLE-SYMBOLS-EMPTY:
+
+## Check that an error is thrown when the input library cannot be found:
+# RUN: not llvm-libtool-darwin -static -o %t.lib -lfile-will-not-exist.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-FOUND -DFILE=file-will-not-exist.o
+
+# NOT-FOUND: error: cannot locate file for '[[FILE]]'
+
+## Check that an error is thrown when the input library is not valid:
+# RUN: touch %t/dirname/not-valid.o
+# RUN: not llvm-libtool-darwin -static -o %t.lib -lnot-valid.o -L%t/dirname 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-VALID -DFILE=not-valid.o
+
+# NOT-VALID: error: '[[FILE]]': The file was not recognized as a valid object file
+
+## Check that 'lib' and '.a' are not added to a file ending in '.o':
+# RUN: llvm-ar cr %t/dirname/libfoo.o.a %t-input1.o
+# RUN: not llvm-libtool-darwin -static -o %t.lib -lfoo.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=NOT-FOUND -DFILE=foo.o

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp

static cl::opt<bool>
     NonDeterministicOption("U", cl::desc("Use actual timestamps and UIDs/GIDs"),
                            cl::init(false), cl::cat(LibtoolCategory));
 
 static cl::opt<std::string>
     FileList("filelist",
              cl::desc("Pass in file containing a list of filenames"),
              cl::value_desc("listfile[,dirname]"), cl::cat(LibtoolCategory));
 
+static cl::list<std::string> Libraries(
+    "l",
+    cl::desc(
+        "l<x> searches for the library libx.a in the library search path. If"
+        " the string 'x' ends with '.o', then the library 'x' is searched for"
+        " without prepending 'lib' or appending '.a'"),
+    cl::ZeroOrMore, cl::Prefix, cl::cat(LibtoolCategory));
+
+static cl::list<std::string> LibrarySearchDirs(
+    "L",
+    cl::desc(
+        "L<dir> adds <dir> to the list of directories in which to search for"
+        " libraries"),
+    cl::ZeroOrMore, cl::Prefix, cl::cat(LibtoolCategory));
+
+static const std::array<std::string, 3> StandardSearchDirs{
+    "/lib",
+    "/usr/lib",
+    "/usr/local/lib",
+};
+
 struct Config {
   bool Deterministic = true; // Updated by 'D' and 'U' modifiers.
   uint32_t ArchCPUType;
   uint32_t ArchCPUSubtype;
 };
 
+static Expected<std::string> searchForFile(const Twine &FileName) {
+
+  auto FindLib = [FileName](ArrayRef<std::string> SearchDirs) {
+    for (StringRef Dir : SearchDirs) {
+      SmallString<128> Path;
+      sys::path::append(Path, Dir, FileName);
+
+      if (sys::fs::exists(Path))
+        return std::string(Path);
+    }
+    return std::string();
+  };
+
+  std::string Found = FindLib(LibrarySearchDirs);
+  if (Found.empty())
+    Found = FindLib(StandardSearchDirs);
+  if (!Found.empty())
+    return Found;
+
+  return createStringError(std::errc::invalid_argument,
+                           "cannot locate file for '%s'",
+                           FileName.str().c_str());
+}
+
+static Error processCommandLineLibraries() {
+  for (const StringRef BaseName : Libraries) {
+    Expected<std::string> FullPath = searchForFile(
+        BaseName.endswith(".o") ? BaseName.str() : "lib" + BaseName + ".a");
+    if (!FullPath)
+      return FullPath.takeError();
+    InputFiles.push_back(FullPath.get());
+  }
+
+  return Error::success();
+}
+
 static Error processFileList() {
   StringRef FileName, DirName;
   std::tie(FileName, DirName) = StringRef(FileList).rsplit(",");
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr =
       MemoryBuffer::getFileOrSTDIN(FileName, /*FileSize=*/-1,
                                    /*RequiresNullTerminator=*/false);
   if (std::error_code EC = FileOrErr.getError())
static Expected<Config> parseCommandLine(int Argc, char **Argv) {
   cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool-darwin\n");
 
   if (DeterministicOption && NonDeterministicOption)
     return createStringError(std::errc::invalid_argument,
                              "cannot specify both -D and -U flags");
   else if (NonDeterministicOption)
     C.Deterministic = false;
 
+  if (!Libraries.empty())
+    if (Error E = processCommandLineLibraries())
+      return std::move(E);
+
   if (!FileList.empty())
     if (Error E = processFileList())
       return std::move(E);
 
   if (InputFiles.empty())
     return createStringError(std::errc::invalid_argument,
                              "no input files specified");