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

7 files changed, 72 insertions, 36 deletions

diff --git a/clang-tools-extra/clang-doc/BitcodeReader.cpp b/clang-tools-extra/clang-doc/BitcodeReader.cpp
index 4489daf3d92..77e67e896c2 100644
--- a/clang-tools-extra/clang-doc/BitcodeReader.cpp
+++ b/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);
diff --git a/clang-tools-extra/clang-doc/BitcodeWriter.cpp b/clang-tools-extra/clang-doc/BitcodeWriter.cpp
index ed235e09947..46ae5af99f8 100644
--- a/clang-tools-extra/clang-doc/BitcodeWriter.cpp
+++ b/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) {
diff --git a/clang-tools-extra/clang-doc/BitcodeWriter.h b/clang-tools-extra/clang-doc/BitcodeWriter.h
index 57c3b056699..c342d34f9f1 100644
--- a/clang-tools-extra/clang-doc/BitcodeWriter.h
+++ b/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;
 };
 
diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp
index e7b910f9fdd..4b272a90ba3 100644
--- a/clang-tools-extra/clangd/ClangdUnit.cpp
+++ b/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.
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 208376a80cd..5bb24941c11 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/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();
diff --git a/clang-tools-extra/clangd/index/Background.cpp b/clang-tools-extra/clangd/index/Background.cpp
index 5b7391b3775..77769ca0d05 100644
--- a/clang-tools-extra/clangd/index/Background.cpp
+++ b/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()) {
diff --git a/clang-tools-extra/clangd/unittests/HeadersTests.cpp b/clang-tools-extra/clangd/unittests/HeadersTests.cpp
index e1591abb11f..a014d780cd5 100644
--- a/clang-tools-extra/clangd/unittests/HeadersTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HeadersTests.cpp
@@ -68,7 +68,7 @@ protected:
     IncludeStructure Includes;
     Clang->getPreprocessor().addPPCallbacks(
         collectIncludeStructureCallback(Clang->getSourceManager(), &Includes));
-    EXPECT_TRUE(Action.Execute());
+    EXPECT_FALSE(Action.Execute());
     Action.EndSourceFile();
     return Includes;
   }