BitStream reader: propagate errors

The bitstream reader handles errors poorly. This has two effects:

 * Bugs in file handling (especially modules) manifest as an "unexpected end of
   file" crash
 * Users of clang as a library end up aborting because the code unconditionally
   calls `report_fatal_error`

The bitstream reader should be more resilient and return Expected / Error as
soon as an error is encountered, not way late like it does now. This patch
starts doing so and adopting the error handling where I think it makes sense.
There's plenty more to do: this patch propagates errors to be minimally useful,
and follow-ups will propagate them further and improve diagnostics.

https://bugs.llvm.org/show_bug.cgi?id=42311
<rdar://problem/33159405>

Differential Revision: https://reviews.llvm.org/D63518

llvm-svn: 364464

clang-tools-extra/clang-doc/BitcodeReader.cpp

@@ -491,24 +491,27 @@ template <typename T>
 llvm::Error ClangDocBitcodeReader::readRecord(unsigned ID, T I) {
   Record R;
   llvm::StringRef Blob;
-  unsigned RecID = Stream.readRecord(ID, R, &Blob);
-  return parseRecord(R, RecID, Blob, I);
+  llvm::Expected<unsigned> MaybeRecID = Stream.readRecord(ID, R, &Blob);
+  if (!MaybeRecID)
+    return MaybeRecID.takeError();
+  return parseRecord(R, MaybeRecID.get(), Blob, I);
 }
 
 template <>
 llvm::Error ClangDocBitcodeReader::readRecord(unsigned ID, Reference *I) {
   Record R;
   llvm::StringRef Blob;
-  unsigned RecID = Stream.readRecord(ID, R, &Blob);
-  return parseRecord(R, RecID, Blob, I, CurrentReferenceField);
+  llvm::Expected<unsigned> MaybeRecID = Stream.readRecord(ID, R, &Blob);
+  if (!MaybeRecID)
+    return MaybeRecID.takeError();
+  return parseRecord(R, MaybeRecID.get(), Blob, I, CurrentReferenceField);
 }
 
 // Read a block of records into a single info.
 template <typename T>
 llvm::Error ClangDocBitcodeReader::readBlock(unsigned ID, T I) {
-  if (Stream.EnterSubBlock(ID))
-    return llvm::make_error<llvm::StringError>("Unable to enter subblock.\n",
-                                               llvm::inconvertibleErrorCode());
+  if (llvm::Error Err = Stream.EnterSubBlock(ID))
+    return Err;
 
   while (true) {
     unsigned BlockOrCode = 0;
@@ -521,9 +524,9 @@ llvm::Error ClangDocBitcodeReader::readBlock(unsigned ID, T I) {
     case Cursor::BlockEnd:
       return llvm::Error::success();
     case Cursor::BlockBegin:
-      if (auto Err = readSubBlock(BlockOrCode, I)) {
-        if (!Stream.SkipBlock())
-          continue;
+      if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+        if (llvm::Error Skipped = Stream.SkipBlock())
+          return joinErrors(std::move(Err), std::move(Skipped));
         return Err;
       }
       continue;
@@ -605,18 +608,34 @@ ClangDocBitcodeReader::skipUntilRecordOrBlock(unsigned &BlockOrRecordID) {
   BlockOrRecordID = 0;
 
   while (!Stream.AtEndOfStream()) {
-    unsigned Code = Stream.ReadCode();
+    Expected<unsigned> MaybeCode = Stream.ReadCode();
+    if (!MaybeCode) {
+      // FIXME this drops the error on the floor.
+      consumeError(MaybeCode.takeError());
+      return Cursor::BadBlock;
+    }
 
-    switch ((llvm::bitc::FixedAbbrevIDs)Code) {
+    // FIXME check that the enum is in range.
+    auto Code = static_cast<llvm::bitc::FixedAbbrevIDs>(MaybeCode.get());
+
+    switch (Code) {
     case llvm::bitc::ENTER_SUBBLOCK:
-      BlockOrRecordID = Stream.ReadSubBlockID();
+      if (Expected<unsigned> MaybeID = Stream.ReadSubBlockID())
+        BlockOrRecordID = MaybeID.get();
+      else {
+        // FIXME this drops the error on the floor.
+        consumeError(MaybeID.takeError());
+      }
       return Cursor::BlockBegin;
     case llvm::bitc::END_BLOCK:
       if (Stream.ReadBlockEnd())
         return Cursor::BadBlock;
       return Cursor::BlockEnd;
     case llvm::bitc::DEFINE_ABBREV:
-      Stream.ReadAbbrevRecord();
+      if (llvm::Error Err = Stream.ReadAbbrevRecord()) {
+        // FIXME this drops the error on the floor.
+        consumeError(std::move(Err));
+      }
       continue;
     case llvm::bitc::UNABBREV_RECORD:
       return Cursor::BadBlock;
@@ -634,17 +653,24 @@ llvm::Error ClangDocBitcodeReader::validateStream() {
                                                llvm::inconvertibleErrorCode());
 
   // Sniff for the signature.
-  if (Stream.Read(8) != BitCodeConstants::Signature[0] ||
-      Stream.Read(8) != BitCodeConstants::Signature[1] ||
-      Stream.Read(8) != BitCodeConstants::Signature[2] ||
-      Stream.Read(8) != BitCodeConstants::Signature[3])
-    return llvm::make_error<llvm::StringError>("Invalid bitcode signature.\n",
-                                               llvm::inconvertibleErrorCode());
+  for (int Idx = 0; Idx != 4; ++Idx) {
+    Expected<llvm::SimpleBitstreamCursor::word_t> MaybeRead = Stream.Read(8);
+    if (!MaybeRead)
+      return MaybeRead.takeError();
+    else if (MaybeRead.get() != BitCodeConstants::Signature[Idx])
+      return llvm::make_error<llvm::StringError>(
+          "Invalid bitcode signature.\n", llvm::inconvertibleErrorCode());
+  }
   return llvm::Error::success();
 }
 
 llvm::Error ClangDocBitcodeReader::readBlockInfoBlock() {
-  BlockInfo = Stream.ReadBlockInfoBlock();
+  Expected<Optional<llvm::BitstreamBlockInfo>> MaybeBlockInfo =
+      Stream.ReadBlockInfoBlock();
+  if (!MaybeBlockInfo)
+    return MaybeBlockInfo.takeError();
+  else
+    BlockInfo = MaybeBlockInfo.get();
   if (!BlockInfo)
     return llvm::make_error<llvm::StringError>(
         "Unable to parse BlockInfoBlock.\n", llvm::inconvertibleErrorCode());
@@ -687,11 +713,16 @@ ClangDocBitcodeReader::readBitcode() {
 
   // Read the top level blocks.
   while (!Stream.AtEndOfStream()) {
-    unsigned Code = Stream.ReadCode();
-    if (Code != llvm::bitc::ENTER_SUBBLOCK)
+    Expected<unsigned> MaybeCode = Stream.ReadCode();
+    if (!MaybeCode)
+      return MaybeCode.takeError();
+    if (MaybeCode.get() != llvm::bitc::ENTER_SUBBLOCK)
       return llvm::make_error<llvm::StringError>(
           "No blocks in input.\n", llvm::inconvertibleErrorCode());
-    unsigned ID = Stream.ReadSubBlockID();
+    Expected<unsigned> MaybeID = Stream.ReadSubBlockID();
+    if (!MaybeID)
+      return MaybeID.takeError();
+    unsigned ID = MaybeID.get();
     switch (ID) {
     // NamedType and Comment blocks should not appear at the top level
     case BI_TYPE_BLOCK_ID:
@@ -720,8 +751,11 @@ ClangDocBitcodeReader::readBitcode() {
         return std::move(Err);
       continue;
     default:
-      if (!Stream.SkipBlock())
-        continue;
+      if (llvm::Error Err = Stream.SkipBlock()) {
+        // FIXME this drops the error on the floor.
+        consumeError(std::move(Err));
+      }
+      continue;
     }
   }
   return std::move(Infos);

clang-tools-extra/clang-doc/BitcodeWriter.cpp

@@ -214,7 +214,7 @@ static const std::vector<std::pair<BlockId, std::vector<RecordId>>>
 
 // AbbreviationMap
 
-constexpr char BitCodeConstants::Signature[];
+constexpr unsigned char BitCodeConstants::Signature[];
 
 void ClangDocBitcodeWriter::AbbreviationMap::add(RecordId RID,
                                                  unsigned AbbrevID) {

clang-tools-extra/clang-doc/BitcodeWriter.h

@@ -44,7 +44,7 @@ struct BitCodeConstants {
   static constexpr unsigned ReferenceTypeSize = 8U;
   static constexpr unsigned USRLengthSize = 6U;
   static constexpr unsigned USRBitLengthSize = 8U;
-  static constexpr char Signature[4] = {'D', 'O', 'C', 'S'};
+  static constexpr unsigned char Signature[4] = {'D', 'O', 'C', 'S'};
   static constexpr int USRHashSize = 20;
 };
 

clang-tools-extra/clangd/ClangdUnit.cpp

@@ -422,8 +422,9 @@ ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
   // Collect tokens of the main file.
   syntax::TokenCollector Tokens(Clang->getPreprocessor());
 
-  if (!Action->Execute())
-    log("Execute() failed when building AST for {0}", MainInput.getFile());
+  if (llvm::Error Err = Action->Execute())
+    log("Execute() failed when building AST for {0}: {1}", MainInput.getFile(),
+        toString(std::move(Err)));
 
   std::vector<Decl *> ParsedDecls = Action->takeTopLevelDecls();
   // AST traversals should exclude the preamble, to avoid performance cliffs.

clang-tools-extra/clangd/CodeComplete.cpp

@@ -1105,8 +1105,9 @@ bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   if (Includes)
     Clang->getPreprocessor().addPPCallbacks(
         collectIncludeStructureCallback(Clang->getSourceManager(), Includes));
-  if (!Action.Execute()) {
-    log("Execute() failed when running codeComplete for {0}", Input.FileName);
+  if (llvm::Error Err = Action.Execute()) {
+    log("Execute() failed when running codeComplete for {0}: {1}",
+        Input.FileName, toString(std::move(Err)));
     return false;
   }
   Action.EndSourceFile();

clang-tools-extra/clangd/index/Background.cpp

@@ -456,9 +456,9 @@ llvm::Error BackgroundIndex::index(tooling::CompileCommand Cmd,
   if (!Action->BeginSourceFile(*Clang, Input))
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                    "BeginSourceFile() failed");
-  if (!Action->Execute())
-    return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                   "Execute() failed");
+  if (llvm::Error Err = Action->Execute())
+    return Err;
+
   Action->EndSourceFile();
   if (Clang->hasDiagnostics() &&
       Clang->getDiagnostics().hasUncompilableErrorOccurred()) {

clang-tools-extra/clangd/unittests/HeadersTests.cpp

@@ -68,7 +68,7 @@ class HeadersTest : public ::testing::Test {
     IncludeStructure Includes;
     Clang->getPreprocessor().addPPCallbacks(
         collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
-    EXPECT_TRUE(Action.Execute());
+    EXPECT_FALSE(Action.Execute());
     Action.EndSourceFile();
     return Includes;
   }

clang/include/clang/Basic/Diagnostic.h

@@ -25,6 +25,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Error.h"
 #include <cassert>
 #include <cstdint>
 #include <limits>
@@ -1303,6 +1304,12 @@ inline DiagnosticBuilder DiagnosticsEngine::Report(SourceLocation Loc,
   return DiagnosticBuilder(this);
 }
 
+inline const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
+                                           llvm::Error &&E) {
+  DB.AddString(toString(std::move(E)));
+  return DB;
+}
+
 inline DiagnosticBuilder DiagnosticsEngine::Report(unsigned DiagID) {
   return Report(SourceLocation(), DiagID);
 }

clang/include/clang/Frontend/FrontendAction.h

@@ -23,6 +23,7 @@
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/FrontendOptions.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include <memory>
 #include <string>
 #include <vector>
@@ -229,7 +230,7 @@ class FrontendAction {
   bool BeginSourceFile(CompilerInstance &CI, const FrontendInputFile &Input);
 
   /// Set the source manager's main input file, and run the action.
-  bool Execute();
+  llvm::Error Execute();
 
   /// Perform any per-file post processing, deallocate per-file
   /// objects, and run statistics and output file cleanup code.

clang/include/clang/Serialization/ASTReader.h

@@ -1437,6 +1437,7 @@ class ASTReader
   void Error(StringRef Msg) const;
   void Error(unsigned DiagID, StringRef Arg1 = StringRef(),
              StringRef Arg2 = StringRef()) const;
+  void Error(llvm::Error &&Err) const;
 
 public:
   /// Load the AST file and validate its contents against the given
@@ -2379,7 +2380,8 @@ class ASTRecordReader {
 
   /// Reads a record with id AbbrevID from Cursor, resetting the
   /// internal state.
-  unsigned readRecord(llvm::BitstreamCursor &Cursor, unsigned AbbrevID);
+  Expected<unsigned> readRecord(llvm::BitstreamCursor &Cursor,
+                                unsigned AbbrevID);
 
   /// Is this a module file for a module (rather than a PCH or similar).
   bool isModule() const { return F->isModule(); }
@@ -2679,7 +2681,10 @@ struct SavedStreamPosition {
       : Cursor(Cursor), Offset(Cursor.GetCurrentBitNo()) {}
 
   ~SavedStreamPosition() {
-    Cursor.JumpToBit(Offset);
+    if (llvm::Error Err = Cursor.JumpToBit(Offset))
+      llvm::report_fatal_error(
+          "Cursor should always be able to go back, failed: " +
+          toString(std::move(Err)));
   }
 
 private:

clang/include/clang/Serialization/GlobalModuleIndex.h

@@ -20,6 +20,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include <memory>
 #include <utility>
 
@@ -122,27 +123,14 @@ class GlobalModuleIndex {
 public:
   ~GlobalModuleIndex();
 
-  /// An error code returned when trying to read an index.
-  enum ErrorCode {
-    /// No error occurred.
-    EC_None,
-    /// No index was found.
-    EC_NotFound,
-    /// Some other process is currently building the index; it is not
-    /// available yet.
-    EC_Building,
-    /// There was an unspecified I/O error reading or writing the index.
-    EC_IOError
-  };
-
   /// Read a global index file for the given directory.
   ///
   /// \param Path The path to the specific module cache where the module files
   /// for the intended configuration reside.
   ///
   /// \returns A pair containing the global module index (if it exists) and
-  /// the error code.
-  static std::pair<GlobalModuleIndex *, ErrorCode>
+  /// the error.
+  static std::pair<GlobalModuleIndex *, llvm::Error>
   readIndex(llvm::StringRef Path);
 
   /// Returns an iterator for identifiers stored in the index table.
@@ -194,9 +182,9 @@ class GlobalModuleIndex {
   /// creating modules.
   /// \param Path The path to the directory containing module files, into
   /// which the global index will be written.
-  static ErrorCode writeIndex(FileManager &FileMgr,
-                              const PCHContainerReader &PCHContainerRdr,
-                              llvm::StringRef Path);
+  static llvm::Error writeIndex(FileManager &FileMgr,
+                                const PCHContainerReader &PCHContainerRdr,
+                                llvm::StringRef Path);
 };
 }
 

clang/lib/Frontend/ASTUnit.cpp

@@ -1206,8 +1206,10 @@ bool ASTUnit::Parse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   else
     PreambleSrcLocCache.clear();
 
-  if (!Act->Execute())
+  if (llvm::Error Err = Act->Execute()) {
+    consumeError(std::move(Err)); // FIXME this drops errors on the floor.
     goto error;
+  }
 
   transferASTDataFromCompilerInstance(*Clang);
 
@@ -1632,7 +1634,8 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
     Clang->setASTConsumer(
         llvm::make_unique<MultiplexConsumer>(std::move(Consumers)));
   }
-  if (!Act->Execute()) {
+  if (llvm::Error Err = Act->Execute()) {
+    consumeError(std::move(Err)); // FIXME this drops errors on the floor.
     AST->transferASTDataFromCompilerInstance(*Clang);
     if (OwnAST && ErrAST)
       ErrAST->swap(OwnAST);
@@ -2280,7 +2283,9 @@ void ASTUnit::CodeComplete(
   std::unique_ptr<SyntaxOnlyAction> Act;
   Act.reset(new SyntaxOnlyAction);
   if (Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0])) {
-    Act->Execute();
+    if (llvm::Error Err = Act->Execute()) {
+      consumeError(std::move(Err)); // FIXME this drops errors on the floor.
+    }
     Act->EndSourceFile();
   }
 }