Reapply: [llvm-size] Tidy up error messages (PR42970)

Clean up some formatting inconsistencies in the error messages and correctly exit with non-zero in all error cases.

Originally submitted as r374771 and then reverted as r374780, this patch fixes the libObject test case in Object/macho-invalid.test.

Patch by Alex Cameron

Differential Revision: https://reviews.llvm.org/D68906

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@374793 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/test/Object/macho-invalid.test b/test/Object/macho-invalid.test
index da71ced2cb9696345aff11337ac718f8540d9ae1..1cbd7469d736ae15aae76fcd412a0aabe383b17f 100644 (file)
--- a/test/Object/macho-invalid.test
+++ b/test/Object/macho-invalid.test
@@ -110,8 +110,8 @@ INVALID-SEGMENT-FILEOFF: macho-invalid-segment-fileoff': truncated or malformed
 RUN: not llvm-nm %p/Inputs/macho-invalid-segment-fileoff 2>&1 | FileCheck -check-prefix INVALID-SEGMENT-FILEOFF-NM %s
 INVALID-SEGMENT-FILEOFF-NM: macho-invalid-segment-fileoff truncated or malformed object (load command 0 fileoff field in LC_SEGMENT extends past the end of the file)
 
-RUN: not llvm-size %p/Inputs/macho-invalid-segment-fileoff 2>&1 | FileCheck -check-prefix INVALID-SEGMENT-FILEOFF-SIZE %s
-INVALID-SEGMENT-FILEOFF-SIZE: macho-invalid-segment-fileoff truncated or malformed object (load command 0 fileoff field in LC_SEGMENT extends past the end of the file)
+RUN: not llvm-size %p/Inputs/macho-invalid-segment-fileoff 2>&1 | FileCheck -DFILE=%p/Inputs/macho-invalid-segment-fileoff -check-prefix INVALID-SEGMENT-FILEOFF-SIZE %s
+INVALID-SEGMENT-FILEOFF-SIZE: error: '[[FILE]]': truncated or malformed object (load command 0 fileoff field in LC_SEGMENT extends past the end of the file)
 
 RUN: not llvm-objdump --macho --private-headers %p/Inputs/macho-invalid-segment-filesize 2>&1 | FileCheck -check-prefix INVALID-SEGMENT-FILESIZE %s
 INVALID-SEGMENT-FILESIZE: macho-invalid-segment-filesize': truncated or malformed object (load command 0 fileoff field plus filesize field in LC_SEGMENT extends past the end of the file)

diff --git a/test/tools/llvm-size/invalid-input.test b/test/tools/llvm-size/invalid-input.test
index a27c7806ab02193d10352fe93bfff20ab257f543..568bd8201c2f1de91712f0196082d8d48676005a 100644 (file)
--- a/test/tools/llvm-size/invalid-input.test
+++ b/test/tools/llvm-size/invalid-input.test
@@ -1,19 +1,15 @@
 ## Show that llvm-size reports an error when passed an input file in an
 ## unknown format.
 
-## FIXME: The error messages tested here are not consistently formatted, and the
-##        second one doesn't even return with a non-zero exit code.
-##        See https://bugs.llvm.org/show_bug.cgi?id=42970.
-
 # RUN: not llvm-size %s 2>&1 | FileCheck %s -DFILE=%s --check-prefix=UNRECOGNIZED
-# UNRECOGNIZED: {{.*}}llvm-size{{(.*)}}: [[FILE]] The file was not recognized as a valid object file
+# UNRECOGNIZED: {{.*}}llvm-size{{(.*)}}: error: '[[FILE]]': The file was not recognized as a valid object file
 
 ## Show that llvm-size reports an error when passed an input file in an
 ## unsupported format.
 
 # RUN: yaml2obj %s -o %t
-# RUN: llvm-size %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=NOTSUPPORTED
-# NOTSUPPORTED: {{.*}}llvm-size{{(.*)}}: [[FILE]]: Unrecognized file type.
+# RUN: not llvm-size %t 2>&1 | FileCheck %s -DFILE=%t --check-prefix=NOTSUPPORTED
+# NOTSUPPORTED: {{.*}}llvm-size{{(.*)}}: error: '[[FILE]]': unsupported file type
 
 --- !minidump
 Streams:

diff --git a/test/tools/llvm-size/no-input.test b/test/tools/llvm-size/no-input.test
index d4a145ff27039b759861d480b408735e5fc2abab..37e5745bf177376aa454ef791140b36c3c301f21 100644 (file)
--- a/test/tools/llvm-size/no-input.test
+++ b/test/tools/llvm-size/no-input.test
@@ -1,7 +1,7 @@
 ## Show that llvm-size emits an error if passed in a non-existent file.
 
 # RUN: not llvm-size %t.blah 2>&1 | FileCheck %s -DFILE=%t.blah --check-prefix=ENOENT
-# ENOENT: {{.*}}llvm-size{{.*}}: [[FILE]] {{[Nn]}}o such file or directory
+# ENOENT: {{.*}}llvm-size{{.*}}: error: '[[FILE]]': {{[Nn]}}o such file or directory
 
 ## Show that llvm-size reads a.out if not passed any file.
 

diff --git a/tools/llvm-size/llvm-size.cpp b/tools/llvm-size/llvm-size.cpp
index da56199fe3c2428c9ba2cc94478e8fd655976cb8..7c63bc291f1b5b6afdf999a44656e8ff609e798b 100644 (file)
--- a/tools/llvm-size/llvm-size.cpp
+++ b/tools/llvm-size/llvm-size.cpp
@@ -24,6 +24,7 @@
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <string>
@@ -106,11 +107,9 @@ static bool HadError = false;
 
 static std::string ToolName;
 
-static bool error(Twine Message) {
+static void error(const Twine &Message, StringRef File) {
   HadError = true;
-  errs() << ToolName << ": " << Message << ".\n";
-  errs().flush();
-  return true;
+  WithColor::error(errs(), ToolName) << "'" << File << "': " << Message << "\n";
 }
 
 // This version of error() prints the archive name and member name, for example:
@@ -119,7 +118,7 @@ static bool error(Twine Message) {
 static void error(llvm::Error E, StringRef FileName, const Archive::Child &C,
                   StringRef ArchitectureName = StringRef()) {
   HadError = true;
-  errs() << ToolName << ": " << FileName;
+  WithColor::error(errs(), ToolName) << "'" << FileName << "'";
 
   Expected<StringRef> NameOrErr = C.getName();
   // TODO: if we have a error getting the name then it would be nice to print
@@ -138,7 +137,7 @@ static void error(llvm::Error E, StringRef FileName, const Archive::Child &C,
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
   OS.flush();
-  errs() << " " << Buf << "\n";
+  errs() << ": " << Buf << "\n";
 }
 
 // This version of error() prints the file name and which architecture slice it // is from, for example: "foo.o (for architecture i386)" after the ToolName
@@ -147,7 +146,7 @@ static void error(llvm::Error E, StringRef FileName, const Archive::Child &C,
 static void error(llvm::Error E, StringRef FileName,
                   StringRef ArchitectureName = StringRef()) {
   HadError = true;
-  errs() << ToolName << ": " << FileName;
+  WithColor::error(errs(), ToolName) << "'" << FileName << "'";
 
   if (!ArchitectureName.empty())
     errs() << " (for architecture " << ArchitectureName << ") ";
@@ -156,7 +155,7 @@ static void error(llvm::Error E, StringRef FileName,
   raw_string_ostream OS(Buf);
   logAllUnhandledErrors(std::move(E), OS);
   OS.flush();
-  errs() << " " << Buf << "\n";
+  errs() << ": " << Buf << "\n";
 }
 
 /// Get the length of the string that represents @p num in Radix including the
@@ -529,7 +528,7 @@ static bool checkMachOAndArchFlags(ObjectFile *O, StringRef Filename) {
   if (none_of(ArchFlags, [&](const std::string &Name) {
         return Name == T.getArchName();
       })) {
-    error(Filename + ": No architecture specified");
+    error("no architecture specified", Filename);
     return false;
   }
   return true;
@@ -658,15 +657,15 @@ static void printFileSectionSizes(StringRef file) {
                 error(std::move(Err), UA->getFileName());
             } else {
               consumeError(AOrErr.takeError());
-              error("Mach-O universal file: " + file + " for architecture " +
-                    StringRef(I->getArchFlagName()) +
-                    " is not a Mach-O file or an archive file");
+              error("mach-o universal file for architecture " +
+                        StringRef(I->getArchFlagName()) +
+                        " is not a mach-o file or an archive file",
+                    file);
             }
           }
         }
         if (!ArchFound) {
-          errs() << ToolName << ": file: " << file
-                 << " does not contain architecture" << ArchFlags[i] << ".\n";
+          error("file does not contain architecture " + ArchFlags[i], file);
           return;
         }
       }
@@ -740,9 +739,10 @@ static void printFileSectionSizes(StringRef file) {
               error(std::move(Err), UA->getFileName());
           } else {
             consumeError(AOrErr.takeError());
-            error("Mach-O universal file: " + file + " for architecture " +
-                   StringRef(I->getArchFlagName()) +
-                   " is not a Mach-O file or an archive file");
+            error("mach-o universal file for architecture " +
+                      StringRef(I->getArchFlagName()) +
+                      " is not a mach-o file or an archive file",
+                  file);
           }
           return;
         }
@@ -816,9 +816,10 @@ static void printFileSectionSizes(StringRef file) {
           error(std::move(Err), UA->getFileName());
       } else {
         consumeError(AOrErr.takeError());
-        error("Mach-O universal file: " + file + " for architecture " +
-               StringRef(I->getArchFlagName()) +
-               " is not a Mach-O file or an archive file");
+        error("mach-o universal file for architecture " +
+                  StringRef(I->getArchFlagName()) +
+                  " is not a mach-o file or an archive file",
+              file);
       }
     }
   } else if (ObjectFile *o = dyn_cast<ObjectFile>(&Bin)) {
@@ -836,8 +837,7 @@ static void printFileSectionSizes(StringRef file) {
       outs() << "\n";
     }
   } else {
-    errs() << ToolName << ": " << file << ": "
-           << "Unrecognized file type.\n";
+    error("unsupported file type", file);
   }
   // System V adds an extra newline at the end of each file.
   if (OutputFormat == sysv)