[llvm-libtool-darwin] Add support for -filelist option

[sameerarora101]

Summary

Add support for -filelist option for llvm-libtool-darwin
-filelist option allows for passing in file containing a list of filenames.
Depends on D83520.

[jhenderson]

Documentation for the new option?

llvm/test/tools/llvm-libtool-darwin/filelist.test

29

Why are you trying to delete some files here?

33

I'm not sure this comment is accurate for what the error is indicating. It needs to provide more context.

67

This seems like essentially a duplicate of the earlier error case?

95

Additional possible test-cases:

1. Multiple file lists specified.
2. File is in working directory, but a directory name is specified.
3. Directory exists but does not contain the requested file.

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

47

Nit: your -o option doesn't include a trailing full stop in its description. Be consistent.

[jhenderson]

By the way, the pre-merge bot is saying your new test is failing on Windows.

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/filelist.test

29

I just wanted to make sure that %basename_t.tmp-input1.o didn't exist in the current directory. However, I have now removed the error test just below (and therefore removed RUN: rm -f %t-input1.o && rm -f %basename_t.tmp-input1.o) as it was similar to the "file not in filelist test" as you highlighted. Thanks!

67

yup, you are right. I have now removed the previous test case

95

3rd case is already there:

## Check that error is thrown when file in filelist doesn't exist (with dir):
# RUN: rm -rf %t/Invalid-Dir && mkdir -p %t/Invalid-Dir
# RUN: echo 'no-such-file' > %t.invalid-list.txt
# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.invalid-list.txt,%t/Invalid-Dir 2>&1 | \
# RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE=%t/Invalid-Dir/no-such-file

I have modified the comment to make it more clear. Added the first 2 cases now. Thanks.

[sameerarora101]

Add documentation and updating test file

[sameerarora101]

Unsupport a test on windows and thow blank-line error earlier.

[sameerarora101]

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

69–70

I was getting an assertion error that filenames can't be empty. Hence, now I am explicitly throwing the error here only.

[MaskRay]

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

61

line_iterator supports is_at_eof. You can delete E

188

Prefer return to exit in main

[jhenderson]

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

52

I'd suggest "Whitespace on a line is assumed..."

llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test

9 ↗

(On Diff #279573)

Could you just use echo '' >> %t.blank-line.txt to get your blank line, so that this can run on Windows?

15 ↗

(On Diff #279573)

Similarly, here, use echo -n " " here. Why the -n at all in fact in this case?

17 ↗

(On Diff #279573)

I'd suggest using FileCheck --strict-whitespace in this case to be able to match the filename properly.

llvm/test/tools/llvm-libtool-darwin/filelist.test

60

error -> an error
file -> a file
filelist -> the filelist
cwd -> the cwd

67

an error
the directory

68

I'd recommend a different name for the directory. The directory itself is not invalid, so calling it that is a little confusing. How about just reusing the directory from before?

75

an error
a file
the cwd

95

Thanks. What about one for the directory not existing at all?

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

69–70

I'd be tempted to use a different error message to show that this error isn't a system error. Perhaps simply "filename cannot be empty".

[sameerarora101]

llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test

9 ↗

(On Diff #279573)

oh nice, thanks!

15 ↗

(On Diff #279573)

Yup, works without -n.

llvm/test/tools/llvm-libtool-darwin/filelist.test

95

Added now.

[sameerarora101]

• Supporting test on windows
• Using is_at_eof for line_iterator

[jhenderson]

LGTM, with nit, thanks!

llvm/test/tools/llvm-libtool-darwin/filelist.test

96

Let's check the full error line. Presumably, this at least has error: file list file: or something similar before it?

[sameerarora101]

Updating the error message.

[smeenai]

LGTM

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

78

Nit: SmallString has an operator std::string, so you should be able to convert directly without the intermediate Twine. I'm not sure if just InputFiles.push_back(Path) will compile, but a cast should definitely trigger the conversion, e.g. InputFiles.push_back(static_cast<std::string>(Path)).

[sameerarora101]

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

78

Yup, casting works. Thanks.

[sameerarora101]

Twine(Path).str() -> static_cast<std::string>(Path)

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

 .. option:: -o <filename>
 
   Specify the output file name. Must be specified exactly once.
 
 .. option:: -static
 
  Produces a static library from the input files.
 
+.. option:: -filelist <listfile[,dirname]>
+
+ Read input file names from `<listfile>`. File names are specified in `<listfile>`
+ one per line, separated only by newlines. Whitespace on a line is assumed
+ to be part of the filename. If the directory name, `dirname`, is also
+ specified then it is prepended to each file name in the `<listfile>`.
+
 EXIT STATUS
 -----------
 
 :program:`llvm-libtool-darwin` exits with a non-zero exit code if there is an error.
 Otherwise, it exits with code 0.
 
 BUGS
 ----
 
 To report bugs, please visit <https://bugs.llvm.org/>.
 
 SEE ALSO
 --------
 
 :manpage:`llvm-ar(1)`

llvm/test/tools/llvm-libtool-darwin/filelist.test

+## This test checks that the -filelist option works correctly.
+
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t-input1.o
+# RUN: yaml2obj %S/Inputs/input2.yaml -o %t-input2.o
+
+## Passing files in a listfile:
+# RUN: echo %t-input1.o >  %t.files.txt
+# RUN: echo %t-input2.o >> %t.files.txt
+# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files.txt
+
+## Check that binaries are present:
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+
+# CHECK-NAMES:      [[PREFIX]]-input1.o
+# CHECK-NAMES-NEXT: [[PREFIX]]-input2.o
+
+## Check that symbols are present:
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=CHECK-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# CHECK-SYMBOLS:      Archive map
+# CHECK-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# CHECK-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# CHECK-SYMBOLS-EMPTY:
+
+# RUN: rm -rf %t/dirname && mkdir -p %t/dirname
+# RUN: yaml2obj %S/Inputs/input1.yaml -o %t/dirname/%basename_t.tmp-input1.o
+# RUN: echo %basename_t.tmp-input1.o > %t.files.txt
+
+## Passing in dirname:
+# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files.txt,%t/dirname
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DIRNAME-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=DIRNAME-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# DIRNAME-NAMES:      [[PREFIX]]-input1.o
+
+# DIRNAME-SYMBOLS:      Archive map
+# DIRNAME-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# DIRNAME-SYMBOLS-EMPTY:
+
+## Passing both -filelist option and object file as input:
+# RUN: llvm-libtool-darwin -static -o %t.lib -filelist %t.files.txt,%t/dirname %t-input2.o
+# RUN: llvm-ar t %t.lib | \
+# RUN:   FileCheck %s --check-prefix=REVERSE-NAMES --implicit-check-not={{.}} -DPREFIX=%basename_t.tmp
+# RUN: llvm-nm --print-armap %t.lib | \
+# RUN:   FileCheck %s --check-prefix=REVERSE-SYMBOLS -DPREFIX=%basename_t.tmp --match-full-lines
+
+# REVERSE-NAMES:      [[PREFIX]]-input2.o
+# REVERSE-NAMES-NEXT: [[PREFIX]]-input1.o
+
+# REVERSE-SYMBOLS:      Archive map
+# REVERSE-SYMBOLS-NEXT: _symbol2 in [[PREFIX]]-input2.o
+# REVERSE-SYMBOLS-NEXT: _symbol1 in [[PREFIX]]-input1.o
+# REVERSE-SYMBOLS-EMPTY:
+
+## Check that an error is thrown when a file in the filelist doesn't exist in the cwd and no dirname is specified:
+# RUN: echo 'no-such-file' > %t.invalid-list.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.invalid-list.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE=no-such-file
+
+# FILE-ERROR: error: '[[FILE]]': {{[nN]}}o such file or directory
+
+## Check that an error is thrown when the directory exists but does not contain the requested file:
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.invalid-list.txt,%t/dirname 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DIR-ERROR -DDIR=%t/dirname -DFILE=no-such-file
+
+# DIR-ERROR: error: '[[DIR]]{{[/\\]}}[[FILE]]': {{[nN]}}o such file or directory
+
+## Check that an error is thrown when a file is in the cwd but dirname is specified:
+# RUN: yaml2obj %S/Inputs/input2.yaml -o %basename_t.tmp-input2.o
+# RUN: echo %basename_t.tmp-input2.o > %t.files-cwd.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.files-cwd.txt,%t/dirname 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DIR-ERROR -DDIR=%t/dirname -DFILE=%basename_t.tmp-input2.o
+
+## Check that an error is thrown when the directory doesn't exist:
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.files-cwd.txt,%t/Invalid-Dir 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DIR-ERROR -DDIR=%t/Invalid-Dir -DFILE=%basename_t.tmp-input2.o
+
+## Check that an error is thrown when the filelist is empty:
+# RUN: touch %t.empty-list
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.empty-list 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=EMPTY-ERROR -DFILE=%t.empty-list
+
+# EMPTY-ERROR: error: file list file: '[[FILE]]' is empty
+
+## Check that an error is thrown when the filelist contains a blank line:
+# RUN: echo %t-input2.o > %t.blank-line.txt
+# RUN: echo '' >> %t.blank-line.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.blank-line.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=EMPTY-FILENAME -DFILE=%t.blank-line.txt
+
+# EMPTY-FILENAME: error: file list file: '[[FILE]]': filename cannot be empty
+
+## Check that an error is thrown when the filelist contains a line with only spaces:
+# RUN: echo %t-input2.o > %t.space-line.txt
+# RUN: echo "     " >> %t.space-line.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.space-line.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE='     ' --strict-whitespace
+
+## Filelist option specified more than once:
+# RUN: touch %t.list1.txt and %t.list2.txt
+# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.list1.txt -filelist %t.list2.txt 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=DUPLICATE-ERROR
+
+# DUPLICATE-ERROR: for the --filelist option: may only occur zero or one times!

llvm/test/tools/llvm-libtool-darwin/invalid-input-output-args.test

 ## This test checks that an error is thrown in case of invalid input/output args.
 
 ## Missing input file:
 # RUN: not llvm-libtool-darwin -static -o %t.lib 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=NO-INPUT
 
-# NO-INPUT: Must specify at least 1 positional argument
+# NO-INPUT: error: no input files specified
 
 ## Missing output file:
 # RUN: not llvm-libtool-darwin -static %t.input 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=NO-OUTPUT
 
 # NO-OUTPUT: for the -o option: must be specified at least once!
 
 ## Missing argument to -o:

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

 //===----------------------------------------------------------------------===//
 
 #include "llvm/BinaryFormat/Magic.h"
 #include "llvm/Object/ArchiveWriter.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/InitLLVM.h"
+#include "llvm/Support/LineIterator.h"
 #include "llvm/Support/WithColor.h"
 
 using namespace llvm;
 using namespace llvm::object;
 
 cl::OptionCategory LibtoolCategory("llvm-libtool-darwin Options");
 
 static cl::opt<std::string> OutputFile("o", cl::desc("Specify output filename"),
                                        cl::value_desc("filename"), cl::Required,
                                        cl::cat(LibtoolCategory));
 
 static cl::list<std::string> InputFiles(cl::Positional,
                                         cl::desc("<input files>"),
-                                        cl::OneOrMore,
+                                        cl::ZeroOrMore,
                                         cl::cat(LibtoolCategory));
 
 enum class Operation { Static };
 
 static cl::opt<Operation> LibraryOperation(
     cl::desc("Library Type: "),
     cl::values(
         clEnumValN(Operation::Static, "static",
                    "Produce a statically linked library from the input files")),
     cl::Required, 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 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())
+    return createFileError(FileName, errorCodeToError(EC));
+  const MemoryBuffer &Ref = *FileOrErr.get();
+
+  line_iterator I(Ref, /*SkipBlanks=*/false);
+  if (I.is_at_eof())
+    return createStringError(std::errc::invalid_argument,
+                             "file list file: '%s' is empty",
+                             FileName.str().c_str());
+  for (; !I.is_at_eof(); ++I) {
+    StringRef Line = *I;
+    if (Line.empty())
+      return createStringError(std::errc::invalid_argument,
+                               "file list file: '%s': filename cannot be empty",
+                               FileName.str().c_str());
+
+    SmallString<128> Path;
+    if (!DirName.empty())
+      sys::path::append(Path, DirName, Line);
+    else
+      sys::path::append(Path, Line);
+    InputFiles.push_back(static_cast<std::string>(Path));
+  }
+  return Error::success();
+}
+
 static Error verifyMachOObject(const NewArchiveMember &Member) {
   auto MBRef = Member.Buf->getMemBufferRef();
   Expected<std::unique_ptr<object::ObjectFile>> ObjOrErr =
       object::ObjectFile::createObjectFile(MBRef);
 
   // Throw error if not a valid object file.
   if (!ObjOrErr)
     return createFileError(Member.MemberName, ObjOrErr.takeError());
if (Error E = writeArchive(OutputFile, NewMembers,
     return E;
   return Error::success();
 }
 
 int main(int Argc, char **Argv) {
   InitLLVM X(Argc, Argv);
   cl::HideUnrelatedOptions({&LibtoolCategory, &ColorCategory});
   cl::ParseCommandLineOptions(Argc, Argv, "llvm-libtool-darwin\n");
+  if (!FileList.empty()) {
+    if (Error E = processFileList()) {
+      WithColor::defaultErrorHandler(std::move(E));
+      return EXIT_FAILURE;
+    }
+  }
+
+  if (InputFiles.empty()) {
+    Error E = createStringError(std::errc::invalid_argument,
+                                "no input files specified");
+    WithColor::defaultErrorHandler(std::move(E));
+    return EXIT_FAILURE;
+  }
 
   switch (LibraryOperation) {
   case Operation::Static:
     if (Error E = createStaticLibrary()) {
       WithColor::defaultErrorHandler(std::move(E));
-      exit(EXIT_FAILURE);
+      return EXIT_FAILURE;
     }
     break;
   }
 }