Fix the NaCl bitstream reader to report fatal errors.

lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp

Index: lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
index 2eba5bb0cfe6ba200ea7ae58291e479a3d88a08e..fb2cc373b4c5857f03319ecc149151e443902c03 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
@@ -9,14 +9,40 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Bitcode/NaCl/NaClBitstreamReader.h"
+#include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
+std::string NaClBitstreamReader::getBitAddress(uint64_t Bit,
+                                               unsigned MinByteWidth) {
+  std::string Buffer;
+  raw_string_ostream Stream(Buffer);
+  Stream << '%' << MinByteWidth << PRIu64 << ":%u";
+  Stream.flush();
+  std::string FormatString(Buffer);
+  Buffer.clear();
+  Stream << format(FormatString.c_str(),
+                   (Bit / 8),
+                   static_cast<unsigned>(Bit % 8));
+  return Stream.str();
+}
+
 //===----------------------------------------------------------------------===//
 //  NaClBitstreamCursor implementation
 //===----------------------------------------------------------------------===//
 
+void NaClBitstreamCursor::ErrorHandler::
+Fatal(const std::string &ErrorMessage) const {
+  // Default implementation is simply print message, and the bit where
+  // the error occurred. This allows old code to continue to work.

[jvoung (off chromium), 2015/02/20 00:12:42]

I'm not sure it's necessary to talk about "old code" -- at some point we won't
know what that version refers to?

[Karl, 2015/02/20 21:41:17]

Removed comment.

+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Error(" << Cursor.getBitAddress(Cursor.GetCurrentBitNo()) << "): "
+         << ErrorMessage;
+  report_fatal_error(StrBuf.str());
+}
+
 void NaClBitstreamCursor::freeState() {
   // Free all the Abbrevs.
   for (size_t i = 0, e = CurAbbrevs.size(); i != e; ++i)
@@ -32,6 +58,20 @@ void NaClBitstreamCursor::freeState() {
   BlockScope.clear();
 }
 
+void NaClBitstreamCursor::reportInvalidAbbrevNumber(unsigned AbbrevNo) const {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Invalid abbreviation # " << AbbrevNo << " defined for record";
+  ErrHandler->Fatal(StrBuf.str());
+}
+
+void NaClBitstreamCursor::reportInvalidJumpToBit(uint64_t BitNo) const {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Invalid jump to bit " << BitNo;
+  ErrHandler->Fatal(StrBuf.str());
+}
+
 /// EnterSubBlock - Having read the ENTER_SUBBLOCK abbrevid, enter
 /// the block, and return true if the block has an error.
 bool NaClBitstreamCursor::EnterSubBlock(unsigned BlockID, unsigned *NumWordsP) {
@@ -66,7 +106,8 @@ void NaClBitstreamCursor::skipAbbreviatedField(const NaClBitCodeAbbrevOp &Op) {
   // Decode the value as we are commanded.
   switch (Op.getEncoding()) {
   case NaClBitCodeAbbrevOp::Literal:
-    report_fatal_error("Not to be used with literals!");
+    llvm_unreachable("Not to be used with literals!");

[jvoung (off chromium), 2015/02/20 00:12:42]

How come these are changed to unreachable, instead of a form of fatal error?

Pretty confident they will be unreachable (callers already check this)?

[Karl, 2015/02/20 21:41:17]

These should be asserts (if they are reached, it is an error in the code). I
tried to follow previous advice (and changes made by JF) to use
llvm_unreachable, so that we still get an error if asserts are turned off. The
idea being that it is clearer that it should be unreachable. See NaClBitCodes.h
for examples of this.

+    break;
   case NaClBitCodeAbbrevOp::Fixed:
     (void)Read((unsigned)Op.getValue());
     break;
@@ -74,7 +115,8 @@ void NaClBitstreamCursor::skipAbbreviatedField(const NaClBitCodeAbbrevOp &Op) {
     (void)ReadVBR64((unsigned)Op.getValue());
     break;
   case NaClBitCodeAbbrevOp::Array:
-    report_fatal_error("Bad array abbreviation encoding!");
+    llvm_unreachable("Bad array abbreviation encoding!");
+    break;
   case NaClBitCodeAbbrevOp::Char6:
     (void)Read(6);
     break;
@@ -147,13 +189,15 @@ uint64_t NaClBitstreamCursor::readArrayAbbreviatedField(
   // Decode the value as we are commanded.
   switch (Op.getEncoding()) {
   case NaClBitCodeAbbrevOp::Literal:
-    report_fatal_error("Not to be used with literals!");
+    llvm_unreachable("Not to be used with literals!");
+    break;
   case NaClBitCodeAbbrevOp::Fixed:
     return Read((unsigned)Op.getValue());
   case NaClBitCodeAbbrevOp::VBR:
     return ReadVBR64((unsigned)Op.getValue());
   case NaClBitCodeAbbrevOp::Array:
-    report_fatal_error("Bad array abbreviation encoding!");
+    llvm_unreachable("Bad array abbreviation encoding!");
+    break;
   case NaClBitCodeAbbrevOp::Char6:
     return NaClBitCodeAbbrevOp::DecodeChar6(Read(6));
   }
@@ -188,7 +232,8 @@ unsigned NaClBitstreamCursor::readRecord(unsigned AbbrevID,
   unsigned Code;
   if (readRecordAbbrevField(Abbv->getOperandInfo(0), Value)) {
     // Array found, use to read all elements.
-    assert(Value > 0 && "No code found for record!");
+    if (Value == 0)
+      ErrHandler->Fatal("No code found for record!");
     const NaClBitCodeAbbrevOp &Op = Abbv->getOperandInfo(1);
     Code = readArrayAbbreviatedField(Op);
     readArrayAbbrev(Op, Value - 1, Vals);
@@ -209,21 +254,18 @@ unsigned NaClBitstreamCursor::readRecord(unsigned AbbrevID,
 }
 
 
-namespace {
-
-static NaClBitCodeAbbrevOp::Encoding getEncoding(uint64_t Encoding) {
-  if (!NaClBitCodeAbbrevOp::isValidEncoding(Encoding)) {
+NaClBitCodeAbbrevOp::Encoding NaClBitstreamCursor::
+getEncoding(uint64_t Value) {
+  if (!NaClBitCodeAbbrevOp::isValidEncoding(Value)) {
     std::string Buffer;
     raw_string_ostream StrBuf(Buffer);
-    StrBuf << "Invalid abbreviation encoding " << Encoding
-           << "specified in bitcode file";
-    report_fatal_error(StrBuf.str());
+    StrBuf << "Invalid abbreviation encoding specified in bitcode file: "
+           << Value;
+    ErrHandler->Fatal(StrBuf.str());
   }
-  return NaClBitCodeAbbrevOp::Encoding(Encoding);
+  return NaClBitCodeAbbrevOp::Encoding(Value);
 }
 
-} // end of anonymous space.
-
 void NaClBitstreamCursor::ReadAbbrevRecord(bool IsLocal,
                                            NaClAbbrevListener *Listener) {
   NaClBitCodeAbbrev *Abbv = new NaClBitCodeAbbrev();