[llvm-libtool-darwin] Add -warnings_as_errors

[keith]

Summary

libtool can currently produce 2 warnings:

1. No symbols were in the object file
2. An object file with the same basename was specified multiple times

The first warning here is often harmless and may just mean you have some
translation units with no symbols for the target you're building for.
The second warning can lead to real issues like those mentioned in
https://reviews.llvm.org/D113130 where ODR violations can slip in.

This introduces a new -warnings_as_errors flag that can be used by build
systems that want to verify they never hit these warnings. For example
with bazel the libtool caller first uniques names to make sure the
duplicate base name case is not possible, but if that doesn't work as
expected, having it fail would be preferred.

It's also worth noting that llvm-libtool-darwin works around an issue
that cctools libtool experiences related to debug info and duplicate
basenames, the workaround is described here:
https://github.com/llvm/llvm-project/blob/30baa5d2a450d5e302d8cba3fc7a26a59d4b7ae1/llvm/lib/Object/ArchiveWriter.cpp#L424-L465
And it avoids this bug:
https://github.com/keith/radars/tree/f0cbbb1c37126ec6528c132510b29e08566377a7/DuplicateBasenameIssue

[smeenai]

Ideally we'd have a way to automatically turn warnings into errors past a certain threshold (similar to how LLD's error handler does it), but given that we only have two warnings right now, that's probably overkill. This LGTM.

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

379

Would creating this as an Error and passing it to WithColor::defaultWarningHandler (as is done in the other case) work here as well, just to reduce the duplication?

[keith]

Reduce duplication in warning / error string

[jhenderson]

Please add the new option to the command guide documentation.

[keith]

Add -warnings_as_errors to docs

[keith]

Remove stray :

[thevinster]

LGTM

[jhenderson]

LGTM.

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

.. option:: -L <dir>
   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:: -no_warning_for_no_symbols
 
    Do not warn about files that have no symbols.
 
+.. option:: -warnings_as_errors
+
+  Produce a non-zero exit status if any warnings are emitted.
+
 .. 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/create-static-lib.test

 # RUN: llvm-libtool-darwin -static -o %t.lib %t-input1.o %t-input2.o %t-input1.o 2>&1 | \
 # RUN:   FileCheck %s --check-prefix=DUPLICATE-INPUT -DFILE=%basename_t.tmp-input1.o \
 # RUN:     -DINPUTA=%t-input1.o -DINPUTB=%t-input1.o
 
 # DUPLICATE-INPUT:     warning: file '[[FILE]]' was specified multiple times.
 # DUPLICATE-INPUT-DAG: [[INPUTA]]
 # DUPLICATE-INPUT-DAG: [[INPUTB]]
 
+# RUN: not llvm-libtool-darwin -warnings_as_errors -static -o %t.lib %t-input1.o %t-input2.o %t-input1.o 2>&1 | \
+# RUN:   FileCheck %s --check-prefix=ERROR-DUPLICATE-INPUT -DFILE=%basename_t.tmp-input1.o \
+# RUN:     -DINPUTA=%t-input1.o -DINPUTB=%t-input1.o
+
+# ERROR-DUPLICATE-INPUT:     error: file '[[FILE]]' was specified multiple times.
+# ERROR-DUPLICATE-INPUT-DAG: [[INPUTA]]
+# ERROR-DUPLICATE-INPUT-DAG: [[INPUTB]]
+
 ## Make sure we can combine object files with the same name if
 ## they are for different architectures.
 # RUN: mkdir -p %t/arm64 %t/armv7
 # RUN: llvm-as %S/Inputs/arm64-ios.ll -o %t/arm64/out.bc
 # RUN: llvm-as %S/Inputs/armv7-ios.ll -o %t/armv7/out.bc
 ## Command output should be empty.
 # RUN: llvm-libtool-darwin -static %t/arm64/out.bc %t/armv7/out.bc -o %t.lib 2>&1 | \
 # RUN:   FileCheck %s --implicit-check-not=warning: --allow-empty

llvm/test/tools/llvm-libtool-darwin/no-symbols-warning.test

 ## This test verifies that the tool emits a warning for object files
 ## without symbols.
 
 # RUN: yaml2obj --docnum=1 %s -o %t-x86_64-empty.o
 # RUN: yaml2obj --docnum=2 %s -o %t-armv7-empty.o
 # RUN: echo 'target triple = "arm64-apple-ios8.0.0"' | llvm-as -o %t-empty.bc
 # RUN: yaml2obj %S/Inputs/input1.yaml -o %t-non-empty.o
 
 # RUN: llvm-libtool-darwin -static -o %t.lib %t-x86_64-empty.o 2>&1 | \
 # RUN:   FileCheck --check-prefix=WARNING %s -DPREFIX=%basename_t.tmp
 
 # WARNING: warning: '[[PREFIX]]-x86_64-empty.o': has no symbols for architecture x86_64
 
+# RUN: not llvm-libtool-darwin -static -warnings_as_errors -o %t-error.lib %t-x86_64-empty.o 2>&1 | \
+# RUN:   FileCheck --check-prefix=ERROR %s -DPREFIX=%basename_t.tmp
+
+# ERROR: error: '[[PREFIX]]-x86_64-empty.o': has no symbols for architecture x86_64
+
 # RUN: llvm-libtool-darwin -no_warning_for_no_symbols -static -o %t.lib \
 # RUN:   %t-x86_64-empty.o 2>&1 | \
 # RUN:   FileCheck %s --allow-empty --implicit-check-not='warning:'
 
 # RUN: llvm-libtool-darwin -static -o %t.lib %t-non-empty.o 2>&1 | \
 # RUN:   FileCheck %s --allow-empty --implicit-check-not='warning:'
 
 # RUN: llvm-libtool-darwin -arch_only x86_64 -static -o %t.lib \

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

static cl::opt<bool>
     VersionOption("V", cl::desc("Print the version number and exit"),
                   cl::cat(LibtoolCategory));
 
 static cl::opt<bool> NoWarningForNoSymbols(
     "no_warning_for_no_symbols",
     cl::desc("Do not warn about files that have no symbols"),
     cl::cat(LibtoolCategory), cl::init(false));
 
+static cl::opt<bool> WarningsAsErrors("warnings_as_errors",
+                                      cl::desc("Treat warnings as errors"),
+                                      cl::cat(LibtoolCategory),
+                                      cl::init(false));
+
 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.
Error verifyAndAddMachOObject(NewArchiveMember Member) {
           MachO::getArchitectureFromName(O->getArchTriple().getArchName()));
 
       // If -arch_only is specified then skip this file if it doesn't match
       // the architecture specified.
       if (!ArchType.empty() && !acceptFileArch(FileCPUType, FileCPUSubtype)) {
         return Error::success();
       }
 
-      if (!NoWarningForNoSymbols && O->symbols().empty())
-        WithColor::warning() << "'" + Member.MemberName +
-                                    "': has no symbols for architecture " +
-                                    O->getArchTriple().getArchName() + "\n";
+      if (!NoWarningForNoSymbols && O->symbols().empty()) {
+        Error E = createFileError(
+            Member.MemberName,
+            createStringError(std::errc::invalid_argument,
+                              "has no symbols for architecture %s",
+                              O->getArchTriple().getArchName().str().c_str()));
+
+        if (WarningsAsErrors)
+          return E;
+        WithColor::defaultWarningHandler(std::move(E));
+      }
 
       uint64_t FileCPUID = getCPUID(FileCPUType, FileCPUSubtype);
       Builder.Data.MembersPerArchitecture[FileCPUID].push_back(
           std::move(Member), FileName);
       return Error::success();
     }
 
     Error verifyAndAddIRObject(NewArchiveMember Member) {

 static Error createStaticLibrary(const Config &C) {
   MembersBuilder Builder(C);
   auto DataOrError = Builder.build();
   if (auto Error = DataOrError.takeError())
     return Error;
 
   const auto &NewMembers = DataOrError->MembersPerArchitecture;
 
-  if (Error E = checkForDuplicates(NewMembers))
+  if (Error E = checkForDuplicates(NewMembers)) {
+    if (WarningsAsErrors)
+      return E;
     WithColor::defaultWarningHandler(std::move(E));
+  }
 
   if (NewMembers.size() == 1)
     return writeArchive(OutputFile, NewMembers.begin()->second.getMembers(),
                         /*WriteSymtab=*/true,
                         /*Kind=*/object::Archive::K_DARWIN, C.Deterministic,
                         /*Thin=*/false);
 
   SmallVector<OwningBinary<Archive>, 2> OutputBinaries;