[Modules] Change result of reading AST block to llvm::Error instead

[bnbarham]

Summary

Reading the AST block can never fail with a recoverable error as modules
cannot be removed during this phase. Change the return type of these
functions to return an llvm::Error instead, ie. either success or
failure.

NFC other than the wording of some of the errors.

[vsapsai]

Not super happy with the complexity DiagnosedError introduces, want to consider alternatives first.

clang/lib/Serialization/ASTReader.cpp

3778

Good copy-paste catch.

[bnbarham]

In D108268#2958346, @vsapsai wrote:

Not super happy with the complexity DiagnosedError introduces, want to consider alternatives first.

Ah yes, I wasn't happy with that either but couldn't come up with a decent alternative. I also considered using DiagnosticError but we'd need to keep a DiagStorageAllocator around in that case.

Honestly I don't love Error in general. It'd be nice if DiagnosticsEngine handled multiple in-flight diagnostics.

EDIT: I just noticed that ASTContext has a getDiagAllocator, so maybe DiagnosticError is the way to go?

[vsapsai]

DiagnosticError looks like a good fit for the task at hand, so it is worth to try it. Though I don't know if it would end up in the end convoluted or OK.

[dexonsmith]

LGTM once @vsapsai is happy.

[bnbarham]

In D108268#2958568, @vsapsai wrote:

DiagnosticError looks like a good fit for the task at hand, so it is worth to try it. Though I don't know if it would end up in the end convoluted or OK.

Unless we also change DiagnosticEngine it doesn't look like this is a viable solution. The PartialDiagnostic can't be emitted straight to Diags, since there may already be a diagnostic in flight (see Error(unsigned DiagID, ...)). The args in PartialDiagnostic are currently protected and it seems weird to change that, but even if they weren't it sort of defeats the purpose of using DiagnosticError in the first place.

Any other ideas?

[vsapsai]

In D108268#2961547, @bnbarham wrote:

Unless we also change DiagnosticEngine it doesn't look like this is a viable solution. The PartialDiagnostic can't be emitted straight to Diags, since there may already be a diagnostic in flight (see Error(unsigned DiagID, ...)). The args in PartialDiagnostic are currently protected and it seems weird to change that, but even if they weren't it sort of defeats the purpose of using DiagnosticError in the first place.

Any other ideas?

This might be a stupid idea and a bridge too far but what if delayed diagnostic was storing PartialDiagnostic and not three strings? This looks like a better API but I haven't tried it myself and concerned we might not have diag allocator in all required places.

Another idea is to replace DiagnosedError with something like ThreeStringError (please don't use this name). I can be wrong but I find it easier to understand an error representing diagnostics compared to a marker that diagnostic was emitted-or-scheduled earlier.

[bnbarham]

! In D108268#2961735, @vsapsai wrote:
This might be a stupid idea and a bridge too far but what if delayed diagnostic was storing PartialDiagnostic and not three strings? This looks like a better API but I haven't tried it myself and concerned we might not have diag allocator in all required places.

It seems like it should be possible to not have the "only one in-flight diagnostic" restriction at all. Removing that would be nice since 1. we could just use DiagnosticError and 2. there would be no need to check for an existing diagnostic.

@yaxunl I see you did a bunch of work extracting out the DiagnosticStorage and StreamingDiagnostic classes. Any thoughts on this? I feel like I'm missing something obvious here.

Another idea is to replace DiagnosedError with something like ThreeStringError (please don't use this name). I can be wrong but I find it easier to understand an error representing diagnostics compared to a marker that diagnostic was emitted-or-scheduled earlier.

There's only a few places that call SetDelayedDiagnostic, but none of them would have an allocator (ie. DiagnosticIDs and ContentCache). I may fallback to this instead (could just call it ASTReadError/something similar).

[bnbarham]

Ended up just using DiagnosticError and reading the args from PartialDiagnostic. Still not really happy with this, but I think the alternative of allowing multiple in-flight diagnostics should be a separate PR regardless (if done at all).

[vsapsai]

Allowing multiple PartialDiagnostic to be in-flight/delayed is a nice goal but it's not clear if it's worth it. Also I'm not sure it is worth investing into making delayed diagnostic easier to use. But that is a separate change and the current one looks fine.

clang/lib/Serialization/ASTReader.cpp

1268

Don't have a strong opinion but an alternative to preserve more of the old code and avoid manual SmallVector manipulations can be

llvm::Error RemainingErr = handleErrors(std::move(Err), /*DiagnosticError lambda*/);
if (RemainingErr)
  Error(toString(std::move(RemainingErr)));

[bnbarham]

Updated error handling and added the clang-format #include changes in.

clang/include/clang/Serialization/ASTReader.h

private:
 
   static ASTReadResult
   readUnhashedControlBlockImpl(ModuleFile *F, llvm::StringRef StreamData,
                                unsigned ClientLoadCapabilities,
                                bool AllowCompatibleConfigurationMismatch,
                                ASTReaderListener *Listener,
                                bool ValidateDiagnosticOptions);
 
-  ASTReadResult ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities);
-  ASTReadResult ReadExtensionBlock(ModuleFile &F);
+  llvm::Error ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities);
+  llvm::Error ReadExtensionBlock(ModuleFile &F);
   void ReadModuleOffsetMap(ModuleFile &F) const;
-  bool ParseLineTable(ModuleFile &F, const RecordData &Record);
-  bool ReadSourceManagerBlock(ModuleFile &F);
+  void ParseLineTable(ModuleFile &F, const RecordData &Record);
+  llvm::Error ReadSourceManagerBlock(ModuleFile &F);
   llvm::BitstreamCursor &SLocCursorForID(int ID);
   SourceLocation getImportLocation(ModuleFile *F);
   ASTReadResult ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
                                        const ModuleFile *ImportedBy,
                                        unsigned ClientLoadCapabilities);
-  ASTReadResult ReadSubmoduleBlock(ModuleFile &F,
-                                   unsigned ClientLoadCapabilities);
+  llvm::Error ReadSubmoduleBlock(ModuleFile &F,
   static bool ParseLanguageOptions(const RecordData &Record, bool Complain,
                                    ASTReaderListener &Listener,
                                    bool AllowCompatibleDifferences);
   static bool ParseTargetOptions(const RecordData &Record, bool Complain,
                                  ASTReaderListener &Listener,
                                  bool AllowCompatibleDifferences);
   static bool ParseDiagnosticOptions(const RecordData &Record, bool Complain,
                                      ASTReaderListener &Listener);
public:
   /// This operation will read a new statement from the external
   /// source each time it is called, and is meant to be used via a
   /// LazyOffsetPtr (which is used by Decls for the body of functions, etc).
   Stmt *GetExternalDeclStmt(uint64_t Offset) override;
 
   /// ReadBlockAbbrevs - Enter a subblock of the specified BlockID with the
   /// specified cursor.  Read the abbreviations that are at the top of the block
   /// and then leave the cursor pointing into the block.
-  static bool ReadBlockAbbrevs(llvm::BitstreamCursor &Cursor, unsigned BlockID,
+  static llvm::Error ReadBlockAbbrevs(llvm::BitstreamCursor &Cursor,
+                                      unsigned BlockID,
                                       uint64_t *StartOfBlockOffset = nullptr);
 
   /// Finds all the visible declarations with a given name.
   /// The current implementation of this method just loads the entire
   /// lookup table as unmaterialized references.
   bool FindExternalVisibleDeclsByName(const DeclContext *DC,
                                       DeclarationName Name) override;
 
   /// Read all of the declarations lexically stored in a

clang/lib/Serialization/ASTReader.cpp

 //===- ASTReader.cpp - AST File Reader ------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ASTReader class, which reads AST files.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Basic/OpenMPKinds.h"
 #include "clang/Serialization/ASTRecordReader.h"
 #include "ASTCommon.h"
 #include "ASTReaderInternals.h"
 #include "clang/AST/AbstractTypeReader.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTMutationListener.h"
 #include "clang/AST/ASTUnresolvedSet.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclGroup.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/DeclarationName.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExternalASTSource.h"
 #include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/OpenMPClause.h"
 #include "clang/AST/ODRHash.h"
 #include "clang/AST/RawCommentList.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/TypeLocVisitor.h"
 #include "clang/AST/UnresolvedSet.h"
 #include "clang/Basic/CommentOptions.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/ExceptionSpecificationType.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/Module.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/OperatorKinds.h"
 #include "clang/Basic/PragmaKinds.h"
 #include "clang/Basic/Sanitizers.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/SourceManagerInternals.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Basic/Version.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/ModuleMap.h"
 #include "clang/Lex/PreprocessingRecord.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Lex/Token.h"
 #include "clang/Sema/ObjCMethodList.h"
 #include "clang/Sema/Scope.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Sema/Weak.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTDeserializationListener.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
 #include "clang/Serialization/InMemoryModuleCache.h"
 #include "clang/Serialization/ModuleFile.h"
 #include "clang/Serialization/ModuleFileExtension.h"
 #include "clang/Serialization/ModuleManager.h"
 #include "clang/Serialization/PCHContainerOperations.h"
 #include "clang/Serialization/SerializationDiagnostic.h"
void ASTReader::Error(unsigned DiagID, StringRef Arg1, StringRef Arg2,
                       StringRef Arg3) const {
   if (Diags.isDiagnosticInFlight())
     Diags.SetDelayedDiagnostic(DiagID, Arg1, Arg2, Arg3);
   else
     Diag(DiagID) << Arg1 << Arg2 << Arg3;
 }
 
 void ASTReader::Error(llvm::Error &&Err) const {
-  Error(toString(std::move(Err)));
+  SmallVector<std::string, 2> Errors;
+  handleAllErrors(
+      std::move(Err),
+      [this](const DiagnosticError &E) {
+        auto Diag = E.getDiagnostic().second;
+
+        // Ideally we'd just emit it, but have to handle a possible in-flight
+        // diagnostic. Note that the location is currently ignored as well.
+        auto NumArgs = Diag.getStorage()->NumDiagArgs;
+        assert(NumArgs <= 3 && "Can only have up to 3 arguments");
+        StringRef Arg1, Arg2, Arg3;
+        switch (NumArgs) {
+        case 3:
+          Arg3 = Diag.getStringArg(2);
+          LLVM_FALLTHROUGH;
+        case 2:
+          Arg2 = Diag.getStringArg(1);
+          LLVM_FALLTHROUGH;
+        case 1:
+          Arg1 = Diag.getStringArg(0);
+        }
+        Error(Diag.getDiagID(), Arg1, Arg2, Arg3);
+      },
+      [&Errors](const llvm::ErrorInfoBase &EI) {
+        Errors.push_back(EI.message());
+      });
+  if (!Errors.empty())
+    Error(llvm::join(Errors.begin(), Errors.end(), "\n"));
 }
 
 //===----------------------------------------------------------------------===//
 // Source Manager Deserialization
 //===----------------------------------------------------------------------===//
 
 /// Read the line table in the source manager block.
-/// \returns true if there was an error.
-bool ASTReader::ParseLineTable(ModuleFile &F,
-                               const RecordData &Record) {
+void ASTReader::ParseLineTable(ModuleFile &F, const RecordData &Record) {
   unsigned Idx = 0;
   LineTableInfo &LineTable = SourceMgr.getLineTable();
 
   // Parse the file names
   std::map<int, int> FileIDs;
   FileIDs[-1] = -1; // For unspecified filenames.
   for (unsigned I = 0; Record[Idx]; ++I) {
     // Extract the file name
for (unsigned I = 0; I != NumEntries; ++I) {
       SrcMgr::CharacteristicKind FileKind
         = (SrcMgr::CharacteristicKind)Record[Idx++];
       unsigned IncludeOffset = Record[Idx++];
       Entries.push_back(LineEntry::get(FileOffset, LineNo, FilenameID,
                                        FileKind, IncludeOffset));
     }
     LineTable.AddEntry(FileID::get(FID), Entries);
   }
-
-  return false;
 }
 
 /// Read a source manager block
-bool ASTReader::ReadSourceManagerBlock(ModuleFile &F) {
+llvm::Error ASTReader::ReadSourceManagerBlock(ModuleFile &F) {
   using namespace SrcMgr;
 
   BitstreamCursor &SLocEntryCursor = F.SLocEntryCursor;
 
   // Set the source-location entry cursor to the current position in
   // the stream. This cursor will be used to read the contents of the
   // source manager block initially, and then lazily read
   // source-location entries as needed.
   SLocEntryCursor = F.Stream;
 
   // The stream itself is going to skip over the source manager block.
-  if (llvm::Error Err = F.Stream.SkipBlock()) {
-    Error(std::move(Err));
-    return true;
-  }
+  if (llvm::Error Err = F.Stream.SkipBlock())
+    return Err;
 
   // Enter the source manager block.
-  if (llvm::Error Err =
-          SLocEntryCursor.EnterSubBlock(SOURCE_MANAGER_BLOCK_ID)) {
-    Error(std::move(Err));
-    return true;
-  }
+  if (llvm::Error Err = SLocEntryCursor.EnterSubBlock(SOURCE_MANAGER_BLOCK_ID))
+    return Err;
   F.SourceManagerBlockStartOffset = SLocEntryCursor.GetCurrentBitNo();
 
   RecordData Record;
   while (true) {
     Expected<llvm::BitstreamEntry> MaybeE =
         SLocEntryCursor.advanceSkippingSubblocks();
-    if (!MaybeE) {
-      Error(MaybeE.takeError());
-      return true;
-    }
+    if (!MaybeE)
+      return MaybeE.takeError();
     llvm::BitstreamEntry E = MaybeE.get();
 
     switch (E.Kind) {
     case llvm::BitstreamEntry::SubBlock: // Handled for us already.
     case llvm::BitstreamEntry::Error:
-      Error("malformed block record in AST file");
-      return true;
+      return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                     "malformed block record in AST file");
     case llvm::BitstreamEntry::EndBlock:
-      return false;
+      return llvm::Error::success();
     case llvm::BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read a record.
     Record.clear();
     StringRef Blob;
     Expected<unsigned> MaybeRecord =
         SLocEntryCursor.readRecord(E.ID, Record, &Blob);
-    if (!MaybeRecord) {
-      Error(MaybeRecord.takeError());
-      return true;
-    }
+    if (!MaybeRecord)
+      return MaybeRecord.takeError();
     switch (MaybeRecord.get()) {
     default:  // Default behavior: ignore.
       break;
 
     case SM_SLOC_FILE_ENTRY:
     case SM_SLOC_BUFFER_ENTRY:
     case SM_SLOC_EXPANSION_ENTRY:
       // Once we hit one of the source location entries, we're done.
-      return false;
+      return llvm::Error::success();
     }
   }
 }
 
 /// If a header file is not found at the path that we expect it to be
 /// and the PCH file was moved from its original location, try to resolve the
 /// file by assuming that header+PCH were moved together and the header is in
 /// the same place relative to the PCH.
if (F->ImportedBy.empty() || !F->ImportedBy[0]) {
     return SourceMgr.getLocForStartOfFile(SourceMgr.getMainFileID());
   }
   return F->ImportedBy[0]->FirstLoc;
 }
 
 /// Enter a subblock of the specified BlockID with the specified cursor. Read
 /// the abbreviations that are at the top of the block and then leave the cursor
 /// pointing into the block.
-bool ASTReader::ReadBlockAbbrevs(BitstreamCursor &Cursor, unsigned BlockID,
+llvm::Error ASTReader::ReadBlockAbbrevs(BitstreamCursor &Cursor,
+                                        unsigned BlockID,
                                         uint64_t *StartOfBlockOffset) {
-  if (llvm::Error Err = Cursor.EnterSubBlock(BlockID)) {
-    // FIXME this drops errors on the floor.
-    consumeError(std::move(Err));
-    return true;
-  }
+  if (llvm::Error Err = Cursor.EnterSubBlock(BlockID))
+    return Err;
 
   if (StartOfBlockOffset)
     *StartOfBlockOffset = Cursor.GetCurrentBitNo();
 
   while (true) {
     uint64_t Offset = Cursor.GetCurrentBitNo();
     Expected<unsigned> MaybeCode = Cursor.ReadCode();
-    if (!MaybeCode) {
-      // FIXME this drops errors on the floor.
-      consumeError(MaybeCode.takeError());
-      return true;
-    }
+    if (!MaybeCode)
+      return MaybeCode.takeError();
     unsigned Code = MaybeCode.get();
 
     // We expect all abbrevs to be at the start of the block.
     if (Code != llvm::bitc::DEFINE_ABBREV) {
-      if (llvm::Error Err = Cursor.JumpToBit(Offset)) {
-        // FIXME this drops errors on the floor.
-        consumeError(std::move(Err));
-        return true;
-      }
-      return false;
-    }
-    if (llvm::Error Err = Cursor.ReadAbbrevRecord()) {
-      // FIXME this drops errors on the floor.
-      consumeError(std::move(Err));
-      return true;
+      if (llvm::Error Err = Cursor.JumpToBit(Offset))
+        return Err;
+      return llvm::Error::success();
     }
+    if (llvm::Error Err = Cursor.ReadAbbrevRecord())
+      return Err;
   }
 }
 
 Token ASTReader::ReadToken(ModuleFile &F, const RecordDataImpl &Record,
                            unsigned &Idx) {
   Token Tok;
   Tok.startToken();
   Tok.setLocation(ReadSourceLocation(F, Record, Idx));
case INPUT_FILE_OFFSETS:
           (const llvm::support::unaligned_uint64_t *)Blob.data();
       F.InputFilesLoaded.resize(NumInputs);
       F.NumUserInputFiles = NumUserInputs;
       break;
     }
   }
 }
 
-ASTReader::ASTReadResult
-ASTReader::ReadASTBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
+llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
+                                    unsigned ClientLoadCapabilities) {
   BitstreamCursor &Stream = F.Stream;
 
-  if (llvm::Error Err = Stream.EnterSubBlock(AST_BLOCK_ID)) {
-    Error(std::move(Err));
-    return Failure;
-  }
+  if (llvm::Error Err = Stream.EnterSubBlock(AST_BLOCK_ID))
+    return Err;
   F.ASTBlockStartOffset = Stream.GetCurrentBitNo();
 
   // Read all of the records and blocks for the AST file.
   RecordData Record;
   while (true) {
     Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
-    if (!MaybeEntry) {
-      Error(MaybeEntry.takeError());
-      return Failure;
-    }
+    if (!MaybeEntry)
+      return MaybeEntry.takeError();
     llvm::BitstreamEntry Entry = MaybeEntry.get();
 
     switch (Entry.Kind) {
     case llvm::BitstreamEntry::Error:
-      Error("error at end of module block in AST file");
-      return Failure;
+      return llvm::createStringError(
+          std::errc::illegal_byte_sequence,
+          "error at end of module block in AST file");
     case llvm::BitstreamEntry::EndBlock:
       // Outside of C++, we do not store a lookup map for the translation unit.
       // Instead, mark it as needing a lookup map to be built if this module
       // contains any declarations lexically within it (which it always does!).
       // This usually has no cost, since we very rarely need the lookup map for
       // the translation unit outside C++.
       if (ASTContext *Ctx = ContextObj) {
         DeclContext *DC = Ctx->getTranslationUnitDecl();
         if (DC->hasExternalLexicalStorage() && !Ctx->getLangOpts().CPlusPlus)
           DC->setMustBuildLookupTable();
       }
 
-      return Success;
+      return llvm::Error::success();
     case llvm::BitstreamEntry::SubBlock:
       switch (Entry.ID) {
       case DECLTYPES_BLOCK_ID:
         // We lazily load the decls block, but we want to set up the
         // DeclsCursor cursor to point into it.  Clone our current bitcode
         // cursor to it, enter the block and read the abbrevs in that block.
         // With the main cursor, we just skip over it.
         F.DeclsCursor = Stream;
-        if (llvm::Error Err = Stream.SkipBlock()) {
-          Error(std::move(Err));
-          return Failure;
-        }
-        if (ReadBlockAbbrevs(F.DeclsCursor, DECLTYPES_BLOCK_ID,
-                             &F.DeclsBlockStartOffset)) {
-          Error("malformed block record in AST file");
-          return Failure;
-        }
+        if (llvm::Error Err = Stream.SkipBlock())
+          return Err;
+        if (llvm::Error Err = ReadBlockAbbrevs(
+                F.DeclsCursor, DECLTYPES_BLOCK_ID, &F.DeclsBlockStartOffset))
+          return Err;
         break;
 
       case PREPROCESSOR_BLOCK_ID:
         F.MacroCursor = Stream;
         if (!PP.getExternalSource())
           PP.setExternalSource(this);
 
-        if (llvm::Error Err = Stream.SkipBlock()) {
-          Error(std::move(Err));
-          return Failure;
-        }
-        if (ReadBlockAbbrevs(F.MacroCursor, PREPROCESSOR_BLOCK_ID)) {
-          Error("malformed block record in AST file");
-          return Failure;
-        }
+        if (llvm::Error Err = Stream.SkipBlock())
+          return Err;
+        if (llvm::Error Err =
+                ReadBlockAbbrevs(F.MacroCursor, PREPROCESSOR_BLOCK_ID))
+          return Err;
         F.MacroStartOffset = F.MacroCursor.GetCurrentBitNo();
         break;
 
       case PREPROCESSOR_DETAIL_BLOCK_ID:
         F.PreprocessorDetailCursor = Stream;
 
         if (llvm::Error Err = Stream.SkipBlock()) {
-          Error(std::move(Err));
-          return Failure;
-        }
-        if (ReadBlockAbbrevs(F.PreprocessorDetailCursor,
-                             PREPROCESSOR_DETAIL_BLOCK_ID)) {
-          Error("malformed preprocessor detail record in AST file");
-          return Failure;
+          return Err;
         }
+        if (llvm::Error Err = ReadBlockAbbrevs(F.PreprocessorDetailCursor,
+                                               PREPROCESSOR_DETAIL_BLOCK_ID))
+          return Err;
         F.PreprocessorDetailStartOffset
         = F.PreprocessorDetailCursor.GetCurrentBitNo();
 
         if (!PP.getPreprocessingRecord())
           PP.createPreprocessingRecord();
         if (!PP.getPreprocessingRecord()->getExternalSource())
           PP.getPreprocessingRecord()->SetExternalSource(*this);
         break;
 
       case SOURCE_MANAGER_BLOCK_ID:
-        if (ReadSourceManagerBlock(F))
-          return Failure;
+        if (llvm::Error Err = ReadSourceManagerBlock(F))
+          return Err;
         break;
 
       case SUBMODULE_BLOCK_ID:
-        if (ASTReadResult Result =
-                ReadSubmoduleBlock(F, ClientLoadCapabilities))
-          return Result;
+        if (llvm::Error Err = ReadSubmoduleBlock(F, ClientLoadCapabilities))
+          return Err;
         break;
 
       case COMMENTS_BLOCK_ID: {
         BitstreamCursor C = Stream;
 
-        if (llvm::Error Err = Stream.SkipBlock()) {
-          Error(std::move(Err));
-          return Failure;
-        }
-        if (ReadBlockAbbrevs(C, COMMENTS_BLOCK_ID)) {
-          Error("malformed comments block in AST file");
-          return Failure;
-        }
+        if (llvm::Error Err = Stream.SkipBlock())
+          return Err;
+        if (llvm::Error Err = ReadBlockAbbrevs(C, COMMENTS_BLOCK_ID))
+          return Err;
         CommentsCursors.push_back(std::make_pair(C, &F));
         break;
       }
 
       default:
-        if (llvm::Error Err = Stream.SkipBlock()) {
-          Error(std::move(Err));
-          return Failure;
-        }
+        if (llvm::Error Err = Stream.SkipBlock())
+          return Err;
         break;
       }
       continue;
 
     case llvm::BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read and process a record.
     Record.clear();
     StringRef Blob;
     Expected<unsigned> MaybeRecordType =
         Stream.readRecord(Entry.ID, Record, &Blob);
-    if (!MaybeRecordType) {
-      Error(MaybeRecordType.takeError());
-      return Failure;
-    }
+    if (!MaybeRecordType)
+      return MaybeRecordType.takeError();
     ASTRecordTypes RecordType = (ASTRecordTypes)MaybeRecordType.get();
 
     // If we're not loading an AST context, we don't care about most records.
     if (!ContextObj) {
       switch (RecordType) {
       case IDENTIFIER_TABLE:
       case IDENTIFIER_OFFSET:
       case INTERESTING_IDENTIFIERS:
if (!ContextObj) {
       }
     }
 
     switch (RecordType) {
     default:  // Default behavior: ignore.
       break;
 
     case TYPE_OFFSET: {
-      if (F.LocalNumTypes != 0) {
-        Error("duplicate TYPE_OFFSET record in AST file");
-        return Failure;
-      }
+      if (F.LocalNumTypes != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "duplicate TYPE_OFFSET record in AST file");
       F.TypeOffsets = reinterpret_cast<const UnderalignedInt64 *>(Blob.data());
       F.LocalNumTypes = Record[0];
       unsigned LocalBaseTypeIndex = Record[1];
       F.BaseTypeIndex = getTotalNumTypes();
 
       if (F.LocalNumTypes > 0) {
         // Introduce the global -> local mapping for types within this module.
         GlobalTypeMap.insert(std::make_pair(getTotalNumTypes(), &F));
 
         // Introduce the local -> global mapping for types within this module.
         F.TypeRemap.insertOrReplace(
           std::make_pair(LocalBaseTypeIndex,
                          F.BaseTypeIndex - LocalBaseTypeIndex));
 
         TypesLoaded.resize(TypesLoaded.size() + F.LocalNumTypes);
       }
       break;
     }
 
     case DECL_OFFSET: {
-      if (F.LocalNumDecls != 0) {
-        Error("duplicate DECL_OFFSET record in AST file");
-        return Failure;
-      }
+      if (F.LocalNumDecls != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "duplicate DECL_OFFSET record in AST file");
       F.DeclOffsets = (const DeclOffset *)Blob.data();
       F.LocalNumDecls = Record[0];
       unsigned LocalBaseDeclID = Record[1];
       F.BaseDeclID = getTotalNumDecls();
 
       if (F.LocalNumDecls > 0) {
         // Introduce the global -> local mapping for declarations within this
         // module.
case IDENTIFIER_TABLE:
             F.IdentifierTableData,
             ASTIdentifierLookupTrait(*this, F));
 
         PP.getIdentifierTable().setExternalIdentifierLookup(this);
       }
       break;
 
     case IDENTIFIER_OFFSET: {
-      if (F.LocalNumIdentifiers != 0) {
-        Error("duplicate IDENTIFIER_OFFSET record in AST file");
-        return Failure;
-      }
+      if (F.LocalNumIdentifiers != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "duplicate IDENTIFIER_OFFSET record in AST file");
       F.IdentifierOffsets = (const uint32_t *)Blob.data();
       F.LocalNumIdentifiers = Record[0];
       unsigned LocalBaseIdentifierID = Record[1];
       F.BaseIdentifierID = getTotalNumIdentifiers();
 
       if (F.LocalNumIdentifiers > 0) {
         // Introduce the global -> local mapping for identifiers within this
         // module.
while (true) {
 
     case SPECIAL_TYPES:
       if (SpecialTypes.empty()) {
         for (unsigned I = 0, N = Record.size(); I != N; ++I)
           SpecialTypes.push_back(getGlobalTypeID(F, Record[I]));
         break;
       }
 
-      if (SpecialTypes.size() != Record.size()) {
-        Error("invalid special-types record");
-        return Failure;
-      }
+      if (SpecialTypes.size() != Record.size())
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid special-types record");
 
       for (unsigned I = 0, N = Record.size(); I != N; ++I) {
         serialization::TypeID ID = getGlobalTypeID(F, Record[I]);
         if (!SpecialTypes[I])
           SpecialTypes[I] = ID;
         // FIXME: If ID && SpecialTypes[I] != ID, do we need a separate
         // merge step?
       }
case UNUSED_FILESCOPED_DECLS:
       break;
 
     case DELEGATING_CTORS:
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         DelegatingCtorDecls.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case WEAK_UNDECLARED_IDENTIFIERS:
-      if (Record.size() % 4 != 0) {
-        Error("invalid weak identifiers record");
-        return Failure;
-      }
+      if (Record.size() % 4 != 0)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid weak identifiers record");
 
       // FIXME: Ignore weak undeclared identifiers from non-original PCH
       // files. This isn't the way to do it :)
       WeakUndeclaredIdentifiers.clear();
 
       // Translate the weak, undeclared identifiers into global IDs.
       for (unsigned I = 0, N = Record.size(); I < N; /* in loop */) {
         WeakUndeclaredIdentifiers.push_back(
while (true) {
     case SOURCE_LOCATION_OFFSETS: {
       F.SLocEntryOffsets = (const uint32_t *)Blob.data();
       F.LocalNumSLocEntries = Record[0];
       SourceLocation::UIntTy SLocSpaceSize = Record[1];
       F.SLocEntryOffsetsBase = Record[2] + F.SourceManagerBlockStartOffset;
       std::tie(F.SLocEntryBaseID, F.SLocEntryBaseOffset) =
           SourceMgr.AllocateLoadedSLocEntries(F.LocalNumSLocEntries,
                                               SLocSpaceSize);
-      if (!F.SLocEntryBaseID) {
-        Error("ran out of source locations");
-        break;
-      }
+      if (!F.SLocEntryBaseID)
+        return llvm::createStringError(std::errc::invalid_argument,
+                                       "ran out of source locations");
       // Make our entry in the range map. BaseID is negative and growing, so
       // we invert it. Because we invert it, though, we need the other end of
       // the range.
       unsigned RangeStart =
           unsigned(-F.SLocEntryBaseID) - F.LocalNumSLocEntries + 1;
       GlobalSLocEntryMap.insert(std::make_pair(RangeStart, &F));
       F.FirstLoc = SourceLocation::getFromRawEncoding(F.SLocEntryBaseOffset);
 
case SOURCE_LOCATION_OFFSETS: {
       break;
     }
 
     case MODULE_OFFSET_MAP:
       F.ModuleOffsetMap = Blob;
       break;
 
     case SOURCE_MANAGER_LINE_TABLE:
-      if (ParseLineTable(F, Record)) {
-        Error("malformed SOURCE_MANAGER_LINE_TABLE in AST file");
-        return Failure;
-      }
+      ParseLineTable(F, Record);
       break;
 
     case SOURCE_LOCATION_PRELOADS: {
       // Need to transform from the local view (1-based IDs) to the global view,
       // which is based off F.SLocEntryBaseID.
-      if (!F.PreloadSLocEntries.empty()) {
-        Error("Multiple SOURCE_LOCATION_PRELOADS records in AST file");
-        return Failure;
-      }
+      if (!F.PreloadSLocEntries.empty())
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "Multiple SOURCE_LOCATION_PRELOADS records in AST file");
 
       F.PreloadSLocEntries.swap(Record);
       break;
     }
 
     case EXT_VECTOR_DECLS:
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         ExtVectorDecls.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case VTABLE_USES:
-      if (Record.size() % 3 != 0) {
-        Error("Invalid VTABLE_USES record");
-        return Failure;
-      }
+      if (Record.size() % 3 != 0)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "Invalid VTABLE_USES record");
 
       // Later tables overwrite earlier ones.
       // FIXME: Modules will have some trouble with this. This is clearly not
       // the right way to do this.
       VTableUses.clear();
 
       for (unsigned Idx = 0, N = Record.size(); Idx != N; /* In loop */) {
         VTableUses.push_back(getGlobalDeclID(F, Record[Idx++]));
         VTableUses.push_back(
           ReadSourceLocation(F, Record, Idx).getRawEncoding());
         VTableUses.push_back(Record[Idx++]);
       }
       break;
 
     case PENDING_IMPLICIT_INSTANTIATIONS:
-      if (PendingInstantiations.size() % 2 != 0) {
-        Error("Invalid existing PendingInstantiations");
-        return Failure;
-      }
+      if (PendingInstantiations.size() % 2 != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "Invalid existing PendingInstantiations");
 
-      if (Record.size() % 2 != 0) {
-        Error("Invalid PENDING_IMPLICIT_INSTANTIATIONS block");
-        return Failure;
-      }
+      if (Record.size() % 2 != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "Invalid PENDING_IMPLICIT_INSTANTIATIONS block");
 
       for (unsigned I = 0, N = Record.size(); I != N; /* in loop */) {
         PendingInstantiations.push_back(getGlobalDeclID(F, Record[I++]));
         PendingInstantiations.push_back(
           ReadSourceLocation(F, Record, I).getRawEncoding());
       }
       break;
 
     case SEMA_DECL_REFS:
-      if (Record.size() != 3) {
-        Error("Invalid SEMA_DECL_REFS block");
-        return Failure;
-      }
+      if (Record.size() != 3)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "Invalid SEMA_DECL_REFS block");
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         SemaDeclRefs.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case PPD_ENTITIES_OFFSETS: {
       F.PreprocessedEntityOffsets = (const PPEntityOffset *)Blob.data();
       assert(Blob.size() % sizeof(PPEntityOffset) == 0);
       F.NumPreprocessedEntities = Blob.size() / sizeof(PPEntityOffset);
case PPD_SKIPPED_RANGES: {
 
       if (F.NumPreprocessedSkippedRanges > 0)
         GlobalSkippedRangeMap.insert(
             std::make_pair(F.BasePreprocessedSkippedRangeID, &F));
       break;
     }
 
     case DECL_UPDATE_OFFSETS:
-      if (Record.size() % 2 != 0) {
-        Error("invalid DECL_UPDATE_OFFSETS block in AST file");
-        return Failure;
-      }
+      if (Record.size() % 2 != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "invalid DECL_UPDATE_OFFSETS block in AST file");
       for (unsigned I = 0, N = Record.size(); I != N; I += 2) {
         GlobalDeclID ID = getGlobalDeclID(F, Record[I]);
         DeclUpdateOffsets[ID].push_back(std::make_pair(&F, Record[I + 1]));
 
         // If we've already loaded the decl, perform the updates when we finish
         // loading this block.
         if (Decl *D = GetExistingDecl(ID))
           PendingUpdateRecords.push_back(
               PendingUpdateRecord(ID, D, /*JustLoaded=*/false));
       }
       break;
 
     case OBJC_CATEGORIES_MAP:
-      if (F.LocalNumObjCCategoriesInMap != 0) {
-        Error("duplicate OBJC_CATEGORIES_MAP record in AST file");
-        return Failure;
-      }
+      if (F.LocalNumObjCCategoriesInMap != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "duplicate OBJC_CATEGORIES_MAP record in AST file");
 
       F.LocalNumObjCCategoriesInMap = Record[0];
       F.ObjCCategoriesMap = (const ObjCCategoriesInfo *)Blob.data();
       break;
 
     case OBJC_CATEGORIES:
       F.ObjCCategories.swap(Record);
       break;
case TENTATIVE_DEFINITIONS:
       break;
 
     case KNOWN_NAMESPACES:
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         KnownNamespaces.push_back(getGlobalDeclID(F, Record[I]));
       break;
 
     case UNDEFINED_BUT_USED:
-      if (UndefinedButUsed.size() % 2 != 0) {
-        Error("Invalid existing UndefinedButUsed");
-        return Failure;
-      }
+      if (UndefinedButUsed.size() % 2 != 0)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "Invalid existing UndefinedButUsed");
 
-      if (Record.size() % 2 != 0) {
-        Error("invalid undefined-but-used record");
-        return Failure;
-      }
+      if (Record.size() % 2 != 0)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid undefined-but-used record");
       for (unsigned I = 0, N = Record.size(); I != N; /* in loop */) {
         UndefinedButUsed.push_back(getGlobalDeclID(F, Record[I++]));
         UndefinedButUsed.push_back(
             ReadSourceLocation(F, Record, I).getRawEncoding());
       }
       break;
 
     case DELETE_EXPRS_TO_ANALYZE:
case IMPORTED_MODULES:
             if (DeserializationListener)
               DeserializationListener->ModuleImportRead(GlobalID, Loc);
           }
         }
       }
       break;
 
     case MACRO_OFFSET: {
-      if (F.LocalNumMacros != 0) {
-        Error("duplicate MACRO_OFFSET record in AST file");
-        return Failure;
-      }
+      if (F.LocalNumMacros != 0)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "duplicate MACRO_OFFSET record in AST file");
       F.MacroOffsets = (const uint32_t *)Blob.data();
       F.LocalNumMacros = Record[0];
       unsigned LocalBaseMacroID = Record[1];
       F.MacroOffsetsBase = Record[2] + F.ASTBlockStartOffset;
       F.BaseMacroID = getTotalNumMacros();
 
       if (F.LocalNumMacros > 0) {
         // Introduce the global -> local mapping for macros within this module.
while (true) {
 
     case LATE_PARSED_TEMPLATE:
       LateParsedTemplates.emplace_back(
           std::piecewise_construct, std::forward_as_tuple(&F),
           std::forward_as_tuple(Record.begin(), Record.end()));
       break;
 
     case OPTIMIZE_PRAGMA_OPTIONS:
-      if (Record.size() != 1) {
-        Error("invalid pragma optimize record");
-        return Failure;
-      }
+      if (Record.size() != 1)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid pragma optimize record");
       OptimizeOffPragmaLocation = ReadSourceLocation(F, Record[0]);
       break;
 
     case MSSTRUCT_PRAGMA_OPTIONS:
-      if (Record.size() != 1) {
-        Error("invalid pragma ms_struct record");
-        return Failure;
-      }
+      if (Record.size() != 1)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid pragma ms_struct record");
       PragmaMSStructState = Record[0];
       break;
 
     case POINTERS_TO_MEMBERS_PRAGMA_OPTIONS:
-      if (Record.size() != 2) {
-        Error("invalid pragma ms_struct record");
-        return Failure;
-      }
+      if (Record.size() != 2)
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "invalid pragma pointers to members record");
       PragmaMSPointersToMembersState = Record[0];
       PointersToMembersPragmaLocation = ReadSourceLocation(F, Record[1]);
       break;
 
     case UNUSED_LOCAL_TYPEDEF_NAME_CANDIDATES:
       for (unsigned I = 0, N = Record.size(); I != N; ++I)
         UnusedLocalTypedefNameCandidates.push_back(
             getGlobalDeclID(F, Record[I]));
       break;
 
     case CUDA_PRAGMA_FORCE_HOST_DEVICE_DEPTH:
-      if (Record.size() != 1) {
-        Error("invalid cuda pragma options record");
-        return Failure;
-      }
+      if (Record.size() != 1)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid cuda pragma options record");
       ForceCUDAHostDeviceDepth = Record[0];
       break;
 
     case ALIGN_PACK_PRAGMA_OPTIONS: {
-      if (Record.size() < 3) {
-        Error("invalid pragma pack record");
-        return Failure;
-      }
+      if (Record.size() < 3)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid pragma pack record");
       PragmaAlignPackCurrentValue = ReadAlignPackInfo(Record[0]);
       PragmaAlignPackCurrentLocation = ReadSourceLocation(F, Record[1]);
       unsigned NumStackEntries = Record[2];
       unsigned Idx = 3;
       // Reset the stack when importing a new module.
       PragmaAlignPackStack.clear();
       for (unsigned I = 0; I < NumStackEntries; ++I) {
         PragmaAlignPackStackEntry Entry;
         Entry.Value = ReadAlignPackInfo(Record[Idx++]);
         Entry.Location = ReadSourceLocation(F, Record[Idx++]);
         Entry.PushLocation = ReadSourceLocation(F, Record[Idx++]);
         PragmaAlignPackStrings.push_back(ReadString(Record, Idx));
         Entry.SlotLabel = PragmaAlignPackStrings.back();
         PragmaAlignPackStack.push_back(Entry);
       }
       break;
     }
 
     case FLOAT_CONTROL_PRAGMA_OPTIONS: {
-      if (Record.size() < 3) {
-        Error("invalid pragma pack record");
-        return Failure;
-      }
+      if (Record.size() < 3)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "invalid pragma float control record");
       FpPragmaCurrentValue = FPOptionsOverride::getFromOpaqueInt(Record[0]);
       FpPragmaCurrentLocation = ReadSourceLocation(F, Record[1]);
       unsigned NumStackEntries = Record[2];
       unsigned Idx = 3;
       // Reset the stack when importing a new module.
       FpPragmaStack.clear();
       for (unsigned I = 0; I < NumStackEntries; ++I) {
         FpPragmaStackEntry Entry;
if (ASTReadResult ReadResult =
 
     // If we find that any modules are unusable, the global index is going
     // to be out-of-date. Just remove it.
     GlobalIndex.reset();
     ModuleMgr.setGlobalIndex(nullptr);
     return ReadResult;
   }
 
-  // Here comes stuff that we only do once the entire chain is loaded.
+  // Here comes stuff that we only do once the entire chain is loaded. Do *not*
+  // remove modules from this point. Various fields are updated during reading
+  // the AST block and removing the modules would result in dangling pointers.
+  // They are generally only incidentally dereferenced, ie. a binary search
+  // runs over `GlobalSLocEntryMap`, which could cause an invalid module to
+  // be dereferenced but it wouldn't actually be used.
 
-  // Load the AST blocks of all of the modules that we loaded.  We can still
+  // Load the AST blocks of all of the modules that we loaded. We can still
   // hit errors parsing the ASTs at this point.
   for (ImportedModule &M : Loaded) {
     ModuleFile &F = *M.Mod;
 
     // Read the AST block.
-    if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
+    if (llvm::Error Err = ReadASTBlock(F, ClientLoadCapabilities)) {
+      Error(std::move(Err));
       return Failure;
+    }
 
     // The AST block should always have a definition for the main module.
     if (F.isModule() && !F.DidReadTopLevelSubmodule) {
       Error(diag::err_module_file_missing_top_level_submodule, F.FileName);
       return Failure;
     }
 
     // Read the extension blocks.
     while (!SkipCursorToBlock(F.Stream, EXTENSION_BLOCK_ID)) {
-      if (ASTReadResult Result = ReadExtensionBlock(F))
+      if (llvm::Error Err = ReadExtensionBlock(F)) {
+        Error(std::move(Err));
         return Failure;
       }
+    }
 
     // Once read, set the ModuleFile bit base offset and update the size in
     // bits of all files we've seen.
     F.GlobalBitOffset = TotalModulesSizeInBits;
     TotalModulesSizeInBits += F.SizeInBits;
     GlobalBitOffsetsMap.insert(std::make_pair(F.GlobalBitOffset, &F));
   }
 
static bool parseModuleFileExtensionMetadata(
   if (BlockNameLen + UserInfoLen > Blob.size()) return true;
 
   Metadata.BlockName = std::string(Blob.data(), Blob.data() + BlockNameLen);
   Metadata.UserInfo = std::string(Blob.data() + BlockNameLen,
                                   Blob.data() + BlockNameLen + UserInfoLen);
   return false;
 }
 
-ASTReader::ASTReadResult ASTReader::ReadExtensionBlock(ModuleFile &F) {
+llvm::Error ASTReader::ReadExtensionBlock(ModuleFile &F) {
   BitstreamCursor &Stream = F.Stream;
 
   RecordData Record;
   while (true) {
     Expected<llvm::BitstreamEntry> MaybeEntry = Stream.advance();
-    if (!MaybeEntry) {
-      Error(MaybeEntry.takeError());
-      return Failure;
-    }
+    if (!MaybeEntry)
+      return MaybeEntry.takeError();
     llvm::BitstreamEntry Entry = MaybeEntry.get();
 
     switch (Entry.Kind) {
     case llvm::BitstreamEntry::SubBlock:
-      if (llvm::Error Err = Stream.SkipBlock()) {
-        Error(std::move(Err));
-        return Failure;
-      }
+      if (llvm::Error Err = Stream.SkipBlock())
+        return Err;
       continue;
-
     case llvm::BitstreamEntry::EndBlock:
-      return Success;
-
+      return llvm::Error::success();
     case llvm::BitstreamEntry::Error:
-      return HadErrors;
-
+      return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                     "malformed block record in AST file");
     case llvm::BitstreamEntry::Record:
       break;
     }
 
     Record.clear();
     StringRef Blob;
     Expected<unsigned> MaybeRecCode =
         Stream.readRecord(Entry.ID, Record, &Blob);
-    if (!MaybeRecCode) {
-      Error(MaybeRecCode.takeError());
-      return Failure;
-    }
+    if (!MaybeRecCode)
+      return MaybeRecCode.takeError();
     switch (MaybeRecCode.get()) {
     case EXTENSION_METADATA: {
       ModuleFileExtensionMetadata Metadata;
-      if (parseModuleFileExtensionMetadata(Record, Blob, Metadata)) {
-        Error("malformed EXTENSION_METADATA in AST file");
-        return Failure;
-      }
+      if (parseModuleFileExtensionMetadata(Record, Blob, Metadata))
+        return llvm::createStringError(
+            std::errc::illegal_byte_sequence,
+            "malformed EXTENSION_METADATA in AST file");
 
       // Find a module file extension with this block name.
       auto Known = ModuleFileExtensions.find(Metadata.BlockName);
       if (Known == ModuleFileExtensions.end()) break;
 
       // Form a reader.
       if (auto Reader = Known->second->createExtensionReader(Metadata, *this,
                                                              F, Stream)) {
         F.ExtensionReaders.push_back(std::move(Reader));
       }
 
       break;
     }
     }
   }
 
-  return Success;
+  return llvm::Error::success();
 }
 
 void ASTReader::InitializeContext() {
   assert(ContextObj && "no context to initialize");
   ASTContext &Context = *ContextObj;
 
   // If there's a listener, notify them that we "read" the translation unit.
   if (DeserializationListener)
bool ASTReader::isAcceptableASTFile(StringRef Filename, FileManager &FileMgr,
   SimplePCHValidator validator(LangOpts, TargetOpts, PPOpts,
                                ExistingModuleCachePath, FileMgr);
   return !readASTFileControlBlock(Filename, FileMgr, PCHContainerRdr,
                                   /*FindModuleFileExtensions=*/false,
                                   validator,
                                   /*ValidateDiagnosticOptions=*/true);
 }
 
-ASTReader::ASTReadResult
-ASTReader::ReadSubmoduleBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
+llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
+                                          unsigned ClientLoadCapabilities) {
   // Enter the submodule block.
-  if (llvm::Error Err = F.Stream.EnterSubBlock(SUBMODULE_BLOCK_ID)) {
-    Error(std::move(Err));
-    return Failure;
-  }
+  if (llvm::Error Err = F.Stream.EnterSubBlock(SUBMODULE_BLOCK_ID))
+    return Err;
 
   ModuleMap &ModMap = PP.getHeaderSearchInfo().getModuleMap();
   bool First = true;
   Module *CurrentModule = nullptr;
   RecordData Record;
   while (true) {
     Expected<llvm::BitstreamEntry> MaybeEntry =
         F.Stream.advanceSkippingSubblocks();
-    if (!MaybeEntry) {
-      Error(MaybeEntry.takeError());
-      return Failure;
-    }
+    if (!MaybeEntry)
+      return MaybeEntry.takeError();
     llvm::BitstreamEntry Entry = MaybeEntry.get();
 
     switch (Entry.Kind) {
     case llvm::BitstreamEntry::SubBlock: // Handled for us already.
     case llvm::BitstreamEntry::Error:
-      Error("malformed block record in AST file");
-      return Failure;
+      return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                     "malformed block record in AST file");
     case llvm::BitstreamEntry::EndBlock:
-      return Success;
+      return llvm::Error::success();
     case llvm::BitstreamEntry::Record:
       // The interesting case.
       break;
     }
 
     // Read a record.
     StringRef Blob;
     Record.clear();
     Expected<unsigned> MaybeKind = F.Stream.readRecord(Entry.ID, Record, &Blob);
-    if (!MaybeKind) {
-      Error(MaybeKind.takeError());
-      return Failure;
-    }
+    if (!MaybeKind)
+      return MaybeKind.takeError();
     unsigned Kind = MaybeKind.get();
 
-    if ((Kind == SUBMODULE_METADATA) != First) {
-      Error("submodule metadata record should be at beginning of block");
-      return Failure;
-    }
+    if ((Kind == SUBMODULE_METADATA) != First)
+      return llvm::createStringError(
+          std::errc::illegal_byte_sequence,
+          "submodule metadata record should be at beginning of block");
     First = false;
 
     // Submodule information is only valid if we have a current module.
     // FIXME: Should we error on these cases?
     if (!CurrentModule && Kind != SUBMODULE_METADATA &&
         Kind != SUBMODULE_DEFINITION)
       continue;
 
     switch (Kind) {
     default:  // Default behavior: ignore.
       break;
 
     case SUBMODULE_DEFINITION: {
-      if (Record.size() < 12) {
-        Error("malformed module definition");
-        return Failure;
-      }
+      if (Record.size() < 12)
+        return llvm::createStringError(std::errc::illegal_byte_sequence,
+                                       "malformed module definition");
 
       StringRef Name = Blob;
       unsigned Idx = 0;
       SubmoduleID GlobalID = getGlobalSubmoduleID(F, Record[Idx++]);
       SubmoduleID Parent = getGlobalSubmoduleID(F, Record[Idx++]);
       Module::ModuleKind Kind = (Module::ModuleKind)Record[Idx++];
       bool IsFramework = Record[Idx++];
       bool IsExplicit = Record[Idx++];
case SUBMODULE_DEFINITION: {
           ModMap.findOrCreateModule(Name, ParentModule, IsFramework, IsExplicit)
               .first;
 
       // FIXME: set the definition loc for CurrentModule, or call
       // ModMap.setInferredModuleAllowedBy()
 
       SubmoduleID GlobalIndex = GlobalID - NUM_PREDEF_SUBMODULE_IDS;
       if (GlobalIndex >= SubmodulesLoaded.size() ||
-          SubmodulesLoaded[GlobalIndex]) {
-        Error("too many submodules");
-        return Failure;
-      }
+          SubmodulesLoaded[GlobalIndex])
+        return llvm::createStringError(std::errc::invalid_argument,
+                                       "too many submodules");
 
       if (!ParentModule) {
         if (const FileEntry *CurFile = CurrentModule->getASTFile()) {
           // Don't emit module relocation error if we have -fno-validate-pch
           if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
                     DisableValidationForModuleKind::Module) &&
               CurFile != F.File) {
-            Error(diag::err_module_file_conflict,
-                  CurrentModule->getTopLevelModuleName(), CurFile->getName(),
-                  F.File->getName());
-            return Failure;
+            auto ConflictError =
+                PartialDiagnostic(diag::err_module_file_conflict,
+                                  ContextObj->DiagAllocator)
+                << CurrentModule->getTopLevelModuleName() << CurFile->getName()
+                << F.File->getName();
+            return DiagnosticError::create(CurrentImportLoc, ConflictError);
           }
         }
 
         F.DidReadTopLevelSubmodule = true;
         CurrentModule->setASTFile(F.File);
         CurrentModule->PresumedModuleMapFile = F.ModuleMapPath;
       }