[llvm-size] Tidy up error messages

[tetsuo-cpp]

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=42970

This patch is to clean up some formatting inconsistencies in the error messages of llvm-size and to correctly exit with non-zero in all error cases (there were a few which were exiting with zero). When working on the error messages, I based the formatting off the way that llvm-objdump does it.

[MaskRay]

llvm/tools/llvm-size/llvm-size.cpp

532

No -> no

Most llvm binary utilities have switched to lowercase error messages.

[MaskRay]

llvm/tools/llvm-size/llvm-size.cpp

110

I'd prefer error(Twine Message, StringRef File). Note the error below places the error (for error message) at the first argument position.

[grimar]

llvm/tools/llvm-size/llvm-size.cpp

110

Yeah. But it should be const Twine &
("Twines should only be used accepted as const references in arguments"
https://llvm.org/doxygen/classllvm_1_1Twine.html#details)

Also, why does it return bool? Can it be void?

[tetsuo-cpp]

Address review comments.

[MaskRay]

LGTM, but please wait for @grimar's opinion.

[grimar]

LGTM.

llvm/tools/llvm-size/llvm-size.cpp

113

FTR, I think error stream is unbuffered, i.e. no need to flush(), but it is probably unrelated to this patch.

[tetsuo-cpp]

Remove unnecessary flush.

[tetsuo-cpp]

Thanks for the reviews @MaskRay and @grimar.
If you're still happy with this patch, could one of you please commit on my behalf?

llvm/tools/llvm-size/llvm-size.cpp

113

Yep, it looks like it is indeed unbuffered so I've removed the flush.

[grimar]

In D68906#1707867, @tetsuo-cpp wrote:

Thanks for the reviews @MaskRay and @grimar.
If you're still happy with this patch, could one of you please commit on my behalf?

I am off today, but can do this tomorrow. If this will not happen earlier :]

[grimar]

Thanks, Fangrui!

[thakis]

This breaks check-llvm on mac, Linux, and windows. Can you please revert?

[thakis]

Reverted in r374780.

[rupprecht]

In D68906#1708057, @thakis wrote:

Reverted in r374780.

Many thanks for keeping the build green -- I've updated the failing test case (check-llvm, lld, clang all pass) & recommitted this in r374793.

[tetsuo-cpp]

I'm sorry for causing trouble. I was only running check-llvm-tools so I didn't notice this problem. I'll make sure I run everything in the future.

llvm/test/tools/llvm-size/invalid-input.test

 ## 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:
   - Type:    LinuxAuxv
     Size:    7
     Content: 11223344556600

llvm/test/tools/llvm-size/no-input.test

 ## 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.
 
 # RUN: rm -rf %t && mkdir -p %t
 # RUN: cd %t
 # RUN: yaml2obj %s -o a.out
 # RUN: llvm-size 2>&1 | FileCheck %s -DFILE=a.out
 

llvm/tools/llvm-size/llvm-size.cpp

 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #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>
 #include <system_error>
 
 using namespace llvm;
 using namespace object;
 

 
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
 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";
+  WithColor::error(errs(), ToolName) << "'" << File << "': " << Message << "\n";
   errs().flush();
-  return true;
 }
 
 // This version of error() prints the archive name and member name, for example:
 // "libx.a(foo.o)" after the ToolName before the error message.  It sets
 // HadError but returns allowing the code to move on to other archive members.
 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
   // the index of which archive member this is and or its offset in the
   // archive instead of "???" as the name.
   if (!NameOrErr) {
     consumeError(NameOrErr.takeError());
     errs() << "(" << "???" << ")";
   } else
     errs() << "(" << NameOrErr.get() << ")";
 
   if (!ArchitectureName.empty())
     errs() << " (for architecture " << ArchitectureName << ") ";
 
   std::string Buf;
   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
 // before the error message.  It sets HadError but returns allowing the code to
 // move on to other architecture slices.
 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 << ") ";
 
   std::string Buf;
   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
 /// leading 0x or 0 for hexadecimal and octal respectively.
 static size_t getNumLengthAsString(uint64_t num) {
   APInt conv(64, num);
   SmallString<32> result;
   conv.toString(result, Radix, false, true);
if (MachO->is64Bit()) {
     T = MachOObjectFile::getArchTriple(H_64.cputype, H_64.cpusubtype);
   } else {
     H = MachO->MachOObjectFile::getHeader();
     T = MachOObjectFile::getArchTriple(H.cputype, H.cpusubtype);
   }
   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;
 }
 
 /// Print the section sizes for @p file. If @p file is an archive, print the
 /// section sizes for each archive member.
 static void printFileSectionSizes(StringRef file) {
if (!ArchAll && !ArchFlags.empty()) {
                              << ")\n";
                   }
                 }
               }
               if (Err)
                 error(std::move(Err), UA->getFileName());
             } else {
               consumeError(AOrErr.takeError());
-              error("Mach-O universal file: " + file + " for architecture " +
+              error("mach-o universal file for architecture " +
                         StringRef(I->getArchFlagName()) +
-                    " is not a Mach-O file or an archive file");
+                        " 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;
         }
       }
       return;
     }
     // No architecture flags were specified so if this contains a slice that
     // matches the host architecture dump only that.
     if (!ArchAll) {
if (!ArchAll) {
                            << ")\n";
                 }
               }
             }
             if (Err)
               error(std::move(Err), UA->getFileName());
           } else {
             consumeError(AOrErr.takeError());
-            error("Mach-O universal file: " + file + " for architecture " +
+            error("mach-o universal file for architecture " +
                       StringRef(I->getArchFlagName()) +
-                   " is not a Mach-O file or an archive file");
+                      " is not a mach-o file or an archive file",
+                  file);
           }
           return;
         }
       }
     }
     // Either all architectures have been specified or none have been specified
     // and this does not contain the host architecture so dump all the slices.
     bool MoreThanOneArch = UB->getNumberOfObjects() > 1;
for (MachOUniversalBinary::object_iterator I = UB->begin_objects(),
                        << ")\n";
             }
           }
         }
         if (Err)
           error(std::move(Err), UA->getFileName());
       } else {
         consumeError(AOrErr.takeError());
-        error("Mach-O universal file: " + file + " for architecture " +
+        error("mach-o universal file for architecture " +
                   StringRef(I->getArchFlagName()) +
-               " is not a Mach-O file or an archive file");
+                  " is not a mach-o file or an archive file",
+              file);
       }
     }
   } else if (ObjectFile *o = dyn_cast<ObjectFile>(&Bin)) {
     if (!checkMachOAndArchFlags(o, file))
       return;
     MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(o);
     if (OutputFormat == sysv)
       outs() << o->getFileName() << "  :\n";
     else if (MachO && OutputFormat == darwin && MoreThanOneFile)
       outs() << o->getFileName() << ":\n";
     printObjectSectionSizes(o);
     if (OutputFormat == berkeley) {
       if (!MachO || MoreThanOneFile)
         outs() << o->getFileName();
       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)
     outs() << "\n";
 }
 
 static void printBerkeleyTotals() {
   std::string fmtbuf;