Additional clean ups on errors in bitcode parsing.

lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp

Index: lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp
diff --git a/lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp b/lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp
index e4de192e71e2067dd556e4173769cfab750a7571..3fe8ce61671659ac4c0c3f9bf352b7b93491bf0e 100644
--- a/lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp
+++ b/lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp
@@ -152,6 +152,22 @@ Allocate(TextFormatter *Formatter, const std::string &Text) {
   return Dir;
 }
 
+std::string RecordTextFormatter::getBitAddress(uint64_t Bit) {
+  std::string Address = naclbitc::getBitAddress(Bit);
+  size_t AddressSize = Address.size();
+  if (AddressSize >= AddressWriteWidth)
+    return Address;
+
+  // Pad address with leading spaces.
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  for (size_t i = AddressWriteWidth - AddressSize; i > 0; --i) {
+    StrBuf << " ";
+  }
+  StrBuf << Address;
+  return StrBuf.str();
+}
+
 RecordTextFormatter::RecordTextFormatter(ObjDumpStream *ObjDump)
     : TextFormatter(ObjDump->Records(), 0, "  "),
       OpenBrace(this, "<"),
@@ -163,7 +179,7 @@ RecordTextFormatter::RecordTextFormatter(ObjDumpStream *ObjDump)
       FinishCluster(this) {
   // Handle fact that 64-bit values can take up to 21 characters.
   MinLineWidth = 21;
-  Label = NaClBitstreamReader::getBitAddress(0, AddressWriteWidth);
+  Label = getBitAddress(0);
 }
 
 std::string RecordTextFormatter::GetEmptyLabelColumn() {
@@ -189,8 +205,7 @@ void RecordTextFormatter::WriteLineIndents() {
 void RecordTextFormatter::WriteValues(uint64_t Bit,
                                       const llvm::NaClBitcodeValues &Values,
                                       int32_t AbbrevIndex) {
-  Label = NaClBitstreamReader::getBitAddress(
-      Bit, RecordTextFormatter::AddressWriteWidth);
+  Label = getBitAddress(Bit);
   if (AbbrevIndex != ABBREV_INDEX_NOT_SPECIFIED) {
     TextStream << AbbrevIndex << ":" << Space;
   }
@@ -237,30 +252,17 @@ ObjDumpStream::ObjDumpStream(raw_ostream &Stream,
   }
 }
 
-raw_ostream &ObjDumpStream::Error(uint64_t Bit) {
-  LastKnownBit = Bit;
+raw_ostream &ObjDumpStream::ErrorAt(naclbitc::ErrorLevel Level, uint64_t Bit) {
+  // Note: Since this method only prints the error line prefix, and
+  // lets the caller fill in the rest of the error, we can't stop the
+  // executable at this point. Instead, we let Flush empty the buffers,
+  // and print out existing errors, and then exit the executable.
   if (NumErrors >= MaxErrors)
-    Fatal(Bit, "Too many errors");
-  ++NumErrors;
-  return PrintMessagePrefix("Error", Bit);
-}
-
-void ObjDumpStream::Fatal(uint64_t Bit, const std::string &Message) {
-  LastKnownBit = Bit;
-  if (!Message.empty())
-    PrintMessagePrefix("Error", Bit) << Message << "\n";
-  Flush();
-  llvm::report_fatal_error("Unable to continue");
-}
-
-void ObjDumpStream::Fatal(uint64_t Bit,
-                          const llvm::NaClBitcodeRecordData &Record,
-                          const std::string &Message) {
+    Flush();

[jvoung (off chromium), 2015/03/09 18:50:23]

How does the caller know that the MaxErrors is hit? Otherwise will it just take
the returned raw_ostream and continue to write to it?

[Karl, 2015/03/09 19:49:25]

I guess my comment wasn't clear enough. I thought that he sentence "Instead, we
let Flush empty the buffers, and print out existing errors, and then exit the
executable." was explaining this. That is why we call Flush.

Note: I fixed this sentence to: "Instead, we let Flush empty the buffers, print
out reported errors, and then exits the executable."

[jvoung (off chromium), 2015/03/09 20:03:01]

Okay, I think I understand now. I didn't look at Flush() closely enough. Flush()
itself does the check again and exits the executable.

I guess I was confused by "we can't stop the executable *at this point*". I
wasn't sure what the later point would be, which made me think that this would
return to the caller, and the caller would exit (but the caller wouldn't know!).
What you meant by a different point in time is the end of Flush(). It wasn't
clear to me that Flush would exit, so how about put an llvm_unreachable() after
Flush()?

"exits" ---back-to---> "exit" ?

Otherwise, would it also help to not-really talk about what can't be done and
just say: "If we've hit the maximum number of errors, let Flush empty the
buffers, print out reported errors, and then exit the executable."?

[Karl, 2015/03/12 16:32:26]

Followed your suggestion and used your suggested change. Also added
llvm_unreachable.

   LastKnownBit = Bit;
-  PrintMessagePrefix("Error", Bit) << Message;
-  Write(Bit, Record);
-  Flush();
-  llvm::report_fatal_error("Unable to continue");
+  if (Level >= naclbitc::Error)
+    ++NumErrors;
+  return naclbitc::ErrorAt(Comments(), Level, Bit);
 }
 
 // Dumps the next line of text in the buffer. Returns the number of characters
@@ -324,15 +326,15 @@ void ObjDumpStream::Flush() {
 
   // Print out messages and reset buffers.
   Stream << MessageBuffer;
-  ResetBuffers();
-  if (NumErrors >= MaxErrors) {
-    // Note: we don't call Fatal here because that will call Flush, causing
-    // an infinite loop.
-    Stream << "Error(" << NaClBitstreamReader::getBitAddress(LastKnownBit)
-           << "): Too many errors\n";
-    llvm::report_fatal_error("Unable to continue");
-  }
   Stream.flush();
+  ResetBuffers();
+  if (NumErrors >= MaxErrors)
+    report_fatal_error("Too many errors, Unable to continue");
+}
+
+void ObjDumpStream::FlushThenQuit() {
+  Flush();
+  report_fatal_error("Unable to continue");
 }
 
 }