Additional clean ups on errors in bitcode parsing.

lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp

 //===-- NaClObjDump.cpp - Dump PNaCl bitcode contents ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/STLExtras.h"
@@ skipping 571 matching lines @@
     // Be sure to flush any remaining errors.
     ObjDump.Flush();
   }
 
   // Returns the number of errors that were sent to the ObjDump.
   unsigned GetNumErrors() {
     return ObjDump.GetNumErrors();
   }
 
   /// Generates an error with the given message.
-  bool ErrorAt(uint64_t Bit, const std::string &Message) final {
-    // Use local error routine so that all errors are treated uniformly.
-    ObjDump.Error(Bit) << Message << "\n";
+  bool ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
+               const std::string &Message) final {
+    if (Level == naclbitc::Fatal)

[jvoung (off chromium), 2015/03/06 22:30:28]

How come Fatal is called out specially?

[Karl, 2015/03/06 22:54:20]

Under the new scheme, within a bitcode parser, all error reporting (including
fatal errors) are passed through this single virtual.

However, for a dump stream, ErrorAt and FatalAt work differently.

That is, to make reporting (non-fatal) errors, ErrorAt only adds the
"Error(...): " prefix, and returns the stream to write the rest of the message
to. It is then the responsibility of the calling routine to fill in the rest of
the message. This makes error reporting easier.

For fatal errors, just printing out the prefix "Fatal(...): " is not enough. It
must also terminate the executable. Hence, the obj stream expects the error
message as an argument. This allows method FatalAt to do the necessary shutdown
of the executable.

I did toy with the idea of making the Error and Fatal methods behave the same,
requiring a second call (or special fatal "eoln" token) to cause the shutdown
separately. However, I did not implement it (yet).

WDYT about this alternative approach? The main advantage of the new approach
would be simplified errors (one can just redirect the rest of the error message
to the stream generated by Error and Fatal, rather than having to generate it
into a raw_string_ostream). The down side is that you must remember to make the
second shutdown call, or the fatal error will not be handled correctly.

[jvoung (off chromium), 2015/03/07 00:04:35]

I think if (a) only this method needs to worry about remembering the second
shutdown call and (b) the # of methods goes down or are simplified, then the
alternative approach would be a win. However, there is currently a second
FatalAt() method that also dumps a record and I'm not sure how that would fit
in... so (b) might not happen.

Otherwise it would be good to leave a comment about why it's a special case.

[Karl, 2015/03/09 18:10:54]

Ok. I was able to clean things up and remove both Fatal and FatalAt from an
NaClObjStream class. All errors now go through the single ErrorAt method. For
Fatal errors you call ErrorAt(naclbitc::Fatal, ....) followed by a call to
FlushThenQuit. This pattern is only needed in one spot (NaClObjDump::ErrorAt).

+      ObjDump.FatalAt(Bit, Message);
+    else
+      ObjDump.ErrorAt(Level, Bit) << Message << "\n";
     return true;
   }
 
-  /// Flushes out objdump and then exits with fatal error.
-  LLVM_ATTRIBUTE_NORETURN
-  void Fatal() {
-    NaClBitcodeParser::Fatal();
-  }
-
-  /// Flushes out objdump and then exits with fatal error, using
-  /// the given message.
-  LLVM_ATTRIBUTE_NORETURN
-  void FatalAt(uint64_t Bit, const std::string &Message) final {
-    ObjDump.Fatal(Bit, Message);
-  }
-
   /// Parses the top-level module block.
   bool ParseBlock(unsigned BlockID) override;
 
   /// Installs the given type to the next available type index.
   void InstallType(Type *Ty) {
     TypeIdType.push_back(Ty);
   }
 
   /// Returns the type associated with the given type index.
   Type *GetType(uint32_t Index) {
@@ skipping 737 matching lines @@
   }
 
   raw_ostream &Errors() {
     return Context->Errors();
   }
 
   raw_ostream &Warnings() {
     return Context->Warnings();
   }
 
-  void Fatal() {
-    return Context->Fatal();
-  }
-
-  void FatalAt(uint64_t Bit, const std::string &Message) override {
-    return Context->FatalAt(Bit, Message);
+  bool ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit,
+               const std::string &Message) final {
+    return Context->ErrorAt(Level, Bit, Message);
   }
 
   const std::string &GetAssemblyIndent() const {
     return Context->GetAssemblyIndent();
   }
 
   unsigned GetAssemblyNumTabs() const {
     return Context->GetAssemblyNumTabs();
   }
 
@@ skipping 2188 matching lines @@
     ObjDump.Error() << "Expected 1 top level block in bitcode: Found:"
                     << NumBlocksRead << "\n";
     ErrorsFound = true;
   }
 
   ObjDump.Flush();
   return ErrorsFound || Parser.GetNumErrors() > 0;
 }
 
 }