Additional clean ups on errors in bitcode parsing.

lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp

Index: lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
diff --git a/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp b/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
index d57caa9566ed92bddef4914df7eede2afebd7ff8..1ee07b3231a2523423aeae6ade0bea15dabc5b83 100644
--- a/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
+++ b/lib/Bitcode/NaCl/Analysis/NaClObjDump.cpp
@@ -589,25 +589,15 @@ public:
   }
 
   /// 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;
 
@@ -1365,12 +1355,9 @@ protected:
     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 {