Additional clean ups on errors in bitcode parsing.

lib/Bitcode/NaCl/Analysis/NaClObjDumpStream.cpp

 //===-- NaClObjDumpStream.cpp --------------------------------------------===//
 //      Implements an objdump stream (bitcode records/assembly code).
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
@@ skipping 134 matching lines @@
 }
 
 
 TextFormatter::Directive *TextFormatter::GetTokenDirective::
 Allocate(TextFormatter *Formatter, const std::string &Text) {
   GetTokenDirective *Dir = Formatter->GetTokenFreeList.Allocate(Formatter);
   Dir->Text = 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, "<"),
       CloseBrace(this, ">"),
       Comma(this, ","),
       Space(this),
       Endline(this),
       StartCluster(this),
       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() {
   std::string Buffer;
   raw_string_ostream StrmBuffer(Buffer);
   for (size_t i = 0; i < Label.size(); ++i) {
     StrmBuffer << ' ';
   }
   StrmBuffer << '|';
   return StrmBuffer.str();
 }
 
 void RecordTextFormatter::WriteLineIndents() {
   if (AtInstructionBeginning) {
     BaseStream << Label << '|';
   } else {
     BaseStream << GetEmptyLabelColumn();
   }
   LinePosition += Label.size() + 1;
   TextFormatter::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;
   }
   TextStream << OpenBrace;
   for (size_t i = 0; i < Values.size(); ++i) {
     if (i > 0) {
       TextStream << Comma << FinishCluster << Space;
     }
     TextStream << StartCluster << Values[i];
   }
@@ skipping 26 matching lines @@
       RecordStream(RecordBuffer),
       RecordFormatter(this) {
   if (DumpRecords) {
     RecordWidth = DumpAssembly
         ? ComboObjDumpSeparatorColumn
         : RecordObjectDumpLength;
     RecordFormatter.SetLineWidth(RecordWidth);
   }
 }
 
-raw_ostream &ObjDumpStream::Error(uint64_t 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)
+    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;
-  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) {
-  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
 // printed.
 static size_t DumpLine(raw_ostream &Stream,
                        const std::string &Buffer,
                        size_t &Index,
                        size_t Size) {
   size_t Count = 0;
   while (Index < Size) {
@@ skipping 43 matching lines @@
         Stream << ColumnSeparator;
       }
       // Dump next assembly line.
       DumpLine(Stream, AssemblyBuffer, AssemblyIndex, AssemblySize);
       Stream << '\n';
     }
   }
 
   // Print out messages and reset buffers.
   Stream << MessageBuffer;
+  Stream.flush();
   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();
+  if (NumErrors >= MaxErrors)
+    report_fatal_error("Too many errors, Unable to continue");
+}
+
+void ObjDumpStream::FlushThenQuit() {
+  Flush();
+  report_fatal_error("Unable to continue");
 }
 
 }
 }