Fix the NaCl bitstream reader to report fatal errors.

include/llvm/Bitcode/NaCl/NaClBitcodeParser.h

Index: include/llvm/Bitcode/NaCl/NaClBitcodeParser.h
diff --git a/include/llvm/Bitcode/NaCl/NaClBitcodeParser.h b/include/llvm/Bitcode/NaCl/NaClBitcodeParser.h
index 3adc5fa76709f16f73fee41a36e1810736685729..44376c1e1bbc7cf2ddfc0945af226809afee4f50 100644
--- a/include/llvm/Bitcode/NaCl/NaClBitcodeParser.h
+++ b/include/llvm/Bitcode/NaCl/NaClBitcodeParser.h
@@ -413,8 +413,24 @@ class NaClBitcodeParser {
   // Allow listener privledges, so that it can update/call the parser
   // using a clean API.
   friend class NaClBitcodeParserListener;
-public:
 
+  // Implements an error handler for errors in the bitstream reader.
+  // Redirects bitstream reader errors to corresponding parrser error

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

parrser -> parser

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

parrser -> parse

+  // reporting function.
+  class ErrorHandler : public NaClBitstreamCursor::ErrorHandler {
+    NaClBitcodeParser *Parser;
+  public:
+    ErrorHandler(NaClBitcodeParser *Parser,
+                 NaClBitstreamCursor &Cursor):
+        NaClBitstreamCursor::ErrorHandler(Cursor), Parser(Parser) {}
+    LLVM_ATTRIBUTE_NORETURN
+    void Fatal(const std::string &ErrorMessage) const final {
+      Parser->FatalAt(getCurrentBitNo(), ErrorMessage);
+    }
+    ~ErrorHandler() override {}
+  };
+
+public:
   // Creates a parser to parse the the block at the given cursor in
   // the PNaCl bitcode stream. This instance is a "dummy" instance
   // that starts the parser.
@@ -423,8 +439,11 @@ public:
         Block(ILLEGAL_BLOCK_ID, Cursor),
         Record(Block),
         Listener(0),
-        ErrStream(&errs())
-  {}
+        ErrStream(&errs()) {
+    std::unique_ptr<NaClBitstreamCursor::ErrorHandler>
+        ErrHandler(new ErrorHandler(this, Cursor));

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

Sometimes multiple instances of NaClBitcodeParsers are created (to parse nested
blocks). I just want to make sure that doesn't end up allocating and
deallocating multiple error handlers for each level (that would seem wasteful).

Otherwise, I wonder if the ErrHandler field can just be the actual ErrorHandler
class so that it's allocated along with the parser (however that allocation
happens on the stack or heap), but you override the parser pointer.

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

Note that the comment states that this constructor is the "dummy" instance that
starts the parser. As such, it creates the error handler and installs it. The
constructor in the protected section (below) that is called by all nested
parsers, doesn't create an error handler.

Hence, we only generate one copy of the error handler. I did think of making it
a field of the parser. However, since we use the same base class for top-level
parsers and nested parsers, I did not do that. To add as a field, would create
multiple instances.

To add it as a field, it would require a refactoring of bitcode parsers (to
separate out top-level from nested).

In addition, I wanted to simplify changes by not having to figure out all
callers of a NaClBitstreamReader. For this reason, I used an owning model of the
error handler. The NaClBitstreamReader creates the default error handler, and
the caller replaces it if it wants to use its own.

In particular, the NaClBitstreamReader is built before it is passed the parsers
(a NaClBitstreamCursor is passed to the parser).

For these reasons, I thought that the solution I've written is a good
compromise, based on the different ways the bitstream reader is used.

+    Cursor.setErrorHandler(ErrHandler);
+  }
 
   virtual ~NaClBitcodeParser();
 
@@ -483,11 +502,40 @@ public:
     return OldErrStream;
   }
 
-  // Called when error occurs. Message is the error to report. Always
+  // Called when an error occurs. BitPosition is the bit position the
+  // error was found, and Message is the error to report. Always
   // returns true (the error return value of Parse).
-  virtual bool Error(const std::string &Message) {
-    *ErrStream << "Error: " << Message << "\n";
-    return true;
+  virtual bool ErrorAt(uint64_t BitPosition, const std::string &Message);
+
+  // Called when an error occurs. Message is the error to
+  // report. Always returns true (the error return value of Parse).
+  bool Error(const std::string &Message) {
+    // Overrides NaClBitcodeParser::Error and defines the reporting
+    // point to be the beginning of the current record being parsed,
+    // instead of at the end of the record.
+    return ErrorAt(Record.GetStartBit(), Message);
+  }
+
+  // Called when a fatal error occurs. BitPosition is the bit position
+  // the error was found, and Message is the error to report. Does not
+  // return.
+  LLVM_ATTRIBUTE_NORETURN
+  virtual void FatalAt(uint64_t BitPosition, const std::string &Message);
+
+  // Called when a fatal error occurs. Message is the error to
+  // report. Does not return.
+  LLVM_ATTRIBUTE_NORETURN
+  void Fatal(const std::string &Message) {
+    // Overrides NaClBitcodeParser::Fatal and defines the reporting
+    // point to be the beginning of the current record being parsed,
+    // instead of at the end of the record.
+    FatalAt(Record.GetStartBit(), Message);
+  }
+
+  // Generates fatal generic error message.
+  LLVM_ATTRIBUTE_NORETURN
+  void Fatal() {
+    Fatal("Fatal error occurred!");
   }
 
   // Returns the number of bits in this block, including nested blocks.
@@ -600,7 +648,6 @@ private:
   // block.
   bool ParseBlockInternal();
 
-
   void operator=(const NaClBitcodeParser &Parser) LLVM_DELETED_FUNCTION;
   NaClBitcodeParser(const NaClBitcodeParser &Parser) LLVM_DELETED_FUNCTION;