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
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index 66e2c2b..4e261d9 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -1206,8 +1206,10 @@
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 @@
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 @@
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();
}
}
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 9cbc9ed..cf02675 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -941,7 +941,9 @@
getSourceManager().clearIDTables();
if (Act.BeginSourceFile(*this, FIF)) {
- Act.Execute();
+ if (llvm::Error Err = Act.Execute()) {
+ consumeError(std::move(Err)); // FIXME this drops errors on the floor.
+ }
Act.EndSourceFile();
}
}
@@ -2043,9 +2045,16 @@
hasPreprocessor()) {
llvm::sys::fs::create_directories(
getPreprocessor().getHeaderSearchInfo().getModuleCachePath());
- GlobalModuleIndex::writeIndex(
- getFileManager(), getPCHContainerReader(),
- getPreprocessor().getHeaderSearchInfo().getModuleCachePath());
+ if (llvm::Error Err = GlobalModuleIndex::writeIndex(
+ getFileManager(), getPCHContainerReader(),
+ getPreprocessor().getHeaderSearchInfo().getModuleCachePath())) {
+ // FIXME this drops the error on the floor. This code is only used for
+ // typo correction and drops more than just this one source of errors
+ // (such as the directory creation failure above). It should handle the
+ // error.
+ consumeError(std::move(Err));
+ return nullptr;
+ }
ModuleManager->resetForReload();
ModuleManager->loadGlobalIndex();
GlobalIndex = ModuleManager->getGlobalIndex();
@@ -2070,9 +2079,13 @@
}
}
if (RecreateIndex) {
- GlobalModuleIndex::writeIndex(
- getFileManager(), getPCHContainerReader(),
- getPreprocessor().getHeaderSearchInfo().getModuleCachePath());
+ if (llvm::Error Err = GlobalModuleIndex::writeIndex(
+ getFileManager(), getPCHContainerReader(),
+ getPreprocessor().getHeaderSearchInfo().getModuleCachePath())) {
+ // FIXME As above, this drops the error on the floor.
+ consumeError(std::move(Err));
+ return nullptr;
+ }
ModuleManager->resetForReload();
ModuleManager->loadGlobalIndex();
GlobalIndex = ModuleManager->getGlobalIndex();
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 2f4f5ef..d724bbc 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -924,7 +924,7 @@
return false;
}
-bool FrontendAction::Execute() {
+llvm::Error FrontendAction::Execute() {
CompilerInstance &CI = getCompilerInstance();
if (CI.hasFrontendTimer()) {
@@ -939,12 +939,18 @@
CI.hasPreprocessor()) {
StringRef Cache =
CI.getPreprocessor().getHeaderSearchInfo().getModuleCachePath();
- if (!Cache.empty())
- GlobalModuleIndex::writeIndex(CI.getFileManager(),
- CI.getPCHContainerReader(), Cache);
+ if (!Cache.empty()) {
+ if (llvm::Error Err = GlobalModuleIndex::writeIndex(
+ CI.getFileManager(), CI.getPCHContainerReader(), Cache)) {
+ // FIXME this drops the error on the floor, but
+ // Index/pch-from-libclang.c seems to rely on dropping at least some of
+ // the error conditions!
+ consumeError(std::move(Err));
+ }
+ }
}
- return true;
+ return llvm::Error::success();
}
void FrontendAction::EndSourceFile() {
diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp
index 1fe8bfc..276a967 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -352,7 +352,8 @@
if (auto CommentHandler = Callbacks.getCommentHandler())
Clang->getPreprocessor().addCommentHandler(CommentHandler);
- Act->Execute();
+ if (llvm::Error Err = Act->Execute())
+ return errorToErrorCode(std::move(Err));
// Run the callbacks.
Callbacks.AfterExecute(*Clang);
diff --git a/clang/lib/Frontend/Rewrite/FrontendActions.cpp b/clang/lib/Frontend/Rewrite/FrontendActions.cpp
index aaef44b..0f1a058 100644
--- a/clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ b/clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -129,7 +129,11 @@
FixItOpts->FixOnlyWarnings = FEOpts.FixOnlyWarnings;
FixItRewriter Rewriter(CI.getDiagnostics(), CI.getSourceManager(),
CI.getLangOpts(), FixItOpts.get());
- FixAction->Execute();
+ if (llvm::Error Err = FixAction->Execute()) {
+ // FIXME this drops the error on the floor.
+ consumeError(std::move(Err));
+ return false;
+ }
err = Rewriter.WriteFixedFiles(&RewrittenFiles);
diff --git a/clang/lib/Frontend/SerializedDiagnosticReader.cpp b/clang/lib/Frontend/SerializedDiagnosticReader.cpp
index bb82e9a..c60dd6c 100644
--- a/clang/lib/Frontend/SerializedDiagnosticReader.cpp
+++ b/clang/lib/Frontend/SerializedDiagnosticReader.cpp
@@ -41,21 +41,47 @@
return SDError::InvalidSignature;
// Sniff for the signature.
- if (Stream.Read(8) != 'D' ||
- Stream.Read(8) != 'I' ||
- Stream.Read(8) != 'A' ||
- Stream.Read(8) != 'G')
+ for (unsigned char C : {'D', 'I', 'A', 'G'}) {
+ if (Expected<llvm::SimpleBitstreamCursor::word_t> Res = Stream.Read(8)) {
+ if (Res.get() == C)
+ continue;
+ } else {
+ // FIXME this drops the error on the floor.
+ consumeError(Res.takeError());
+ }
return SDError::InvalidSignature;
+ }
// Read the top level blocks.
while (!Stream.AtEndOfStream()) {
- if (Stream.ReadCode() != llvm::bitc::ENTER_SUBBLOCK)
+ if (Expected<unsigned> Res = Stream.ReadCode()) {
+ if (Res.get() != llvm::bitc::ENTER_SUBBLOCK)
+ return SDError::InvalidDiagnostics;
+ } else {
+ // FIXME this drops the error on the floor.
+ consumeError(Res.takeError());
return SDError::InvalidDiagnostics;
+ }
std::error_code EC;
- switch (Stream.ReadSubBlockID()) {
- case llvm::bitc::BLOCKINFO_BLOCK_ID:
- BlockInfo = Stream.ReadBlockInfoBlock();
+ Expected<unsigned> MaybeSubBlockID = Stream.ReadSubBlockID();
+ if (!MaybeSubBlockID) {
+ // FIXME this drops the error on the floor.
+ consumeError(MaybeSubBlockID.takeError());
+ return SDError::InvalidDiagnostics;
+ }
+
+ switch (MaybeSubBlockID.get()) {
+ case llvm::bitc::BLOCKINFO_BLOCK_ID: {
+ Expected<Optional<llvm::BitstreamBlockInfo>> MaybeBlockInfo =
+ Stream.ReadBlockInfoBlock();
+ if (!MaybeBlockInfo) {
+ // FIXME this drops the error on the floor.
+ consumeError(MaybeBlockInfo.takeError());
+ return SDError::InvalidDiagnostics;
+ }
+ BlockInfo = std::move(MaybeBlockInfo.get());
+ }
if (!BlockInfo)
return SDError::MalformedBlockInfoBlock;
Stream.setBlockInfo(&*BlockInfo);
@@ -69,8 +95,11 @@
return EC;
continue;
default:
- if (!Stream.SkipBlock())
+ if (llvm::Error Err = Stream.SkipBlock()) {
+ // FIXME this drops the error on the floor.
+ consumeError(std::move(Err));
return SDError::MalformedTopLevelBlock;
+ }
continue;
}
}
@@ -89,11 +118,18 @@
BlockOrRecordID = 0;
while (!Stream.AtEndOfStream()) {
- unsigned Code = Stream.ReadCode();
+ unsigned Code;
+ if (Expected<unsigned> Res = Stream.ReadCode())
+ Code = Res.get();
+ else
+ return llvm::errorToErrorCode(Res.takeError());
switch ((llvm::bitc::FixedAbbrevIDs)Code) {
case llvm::bitc::ENTER_SUBBLOCK:
- BlockOrRecordID = Stream.ReadSubBlockID();
+ if (Expected<unsigned> Res = Stream.ReadSubBlockID())
+ BlockOrRecordID = Res.get();
+ else
+ return llvm::errorToErrorCode(Res.takeError());
return Cursor::BlockBegin;
case llvm::bitc::END_BLOCK:
@@ -102,7 +138,8 @@
return Cursor::BlockEnd;
case llvm::bitc::DEFINE_ABBREV:
- Stream.ReadAbbrevRecord();
+ if (llvm::Error Err = Stream.ReadAbbrevRecord())
+ return llvm::errorToErrorCode(std::move(Err));
continue;
case llvm::bitc::UNABBREV_RECORD:
@@ -120,8 +157,12 @@
std::error_code
SerializedDiagnosticReader::readMetaBlock(llvm::BitstreamCursor &Stream) {
- if (Stream.EnterSubBlock(clang::serialized_diags::BLOCK_META))
+ if (llvm::Error Err =
+ Stream.EnterSubBlock(clang::serialized_diags::BLOCK_META)) {
+ // FIXME this drops the error on the floor.
+ consumeError(std::move(Err));
return SDError::MalformedMetadataBlock;
+ }
bool VersionChecked = false;
@@ -135,8 +176,11 @@
case Cursor::Record:
break;
case Cursor::BlockBegin:
- if (Stream.SkipBlock())
+ if (llvm::Error Err = Stream.SkipBlock()) {
+ // FIXME this drops the error on the floor.
+ consumeError(std::move(Err));
return SDError::MalformedMetadataBlock;
+ }
LLVM_FALLTHROUGH;
case Cursor::BlockEnd:
if (!VersionChecked)
@@ -145,7 +189,10 @@
}
SmallVector<uint64_t, 1> Record;
- unsigned RecordID = Stream.readRecord(BlockOrCode, Record);
+ Expected<unsigned> MaybeRecordID = Stream.readRecord(BlockOrCode, Record);
+ if (!MaybeRecordID)
+ return errorToErrorCode(MaybeRecordID.takeError());
+ unsigned RecordID = MaybeRecordID.get();
if (RecordID == RECORD_VERSION) {
if (Record.size() < 1)
@@ -159,8 +206,12 @@
std::error_code
SerializedDiagnosticReader::readDiagnosticBlock(llvm::BitstreamCursor &Stream) {
- if (Stream.EnterSubBlock(clang::serialized_diags::BLOCK_DIAG))
+ if (llvm::Error Err =
+ Stream.EnterSubBlock(clang::serialized_diags::BLOCK_DIAG)) {
+ // FIXME this drops the error on the floor.
+ consumeError(std::move(Err));
return SDError::MalformedDiagnosticBlock;
+ }
std::error_code EC;
if ((EC = visitStartOfDiagnostic()))
@@ -179,8 +230,11 @@
if (BlockOrCode == serialized_diags::BLOCK_DIAG) {
if ((EC = readDiagnosticBlock(Stream)))
return EC;
- } else if (!Stream.SkipBlock())
+ } else if (llvm::Error Err = Stream.SkipBlock()) {
+ // FIXME this drops the error on the floor.
+ consumeError(std::move(Err));
return SDError::MalformedSubBlock;
+ }
continue;
case Cursor::BlockEnd:
if ((EC = visitEndOfDiagnostic()))
@@ -193,7 +247,11 @@
// Read the record.
Record.clear();
StringRef Blob;
- unsigned RecID = Stream.readRecord(BlockOrCode, Record, &Blob);
+ Expected<unsigned> MaybeRecID =
+ Stream.readRecord(BlockOrCode, Record, &Blob);
+ if (!MaybeRecID)
+ return errorToErrorCode(MaybeRecID.takeError());
+ unsigned RecID = MaybeRecID.get();
if (RecID < serialized_diags::RECORD_FIRST ||
RecID > serialized_diags::RECORD_LAST)
diff --git a/clang/lib/Frontend/TestModuleFileExtension.cpp b/clang/lib/Frontend/TestModuleFileExtension.cpp
index 561fe10..f8ed341 100644
--- a/clang/lib/Frontend/TestModuleFileExtension.cpp
+++ b/clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -48,7 +48,12 @@
// Read the extension block.
SmallVector<uint64_t, 4> Record;
while (true) {
- llvm::BitstreamEntry Entry = Stream.advanceSkippingSubblocks();
+ llvm::Expected<llvm::BitstreamEntry> MaybeEntry =
+ Stream.advanceSkippingSubblocks();
+ if (!MaybeEntry)
+ (void)MaybeEntry.takeError();
+ llvm::BitstreamEntry Entry = MaybeEntry.get();
+
switch (Entry.Kind) {
case llvm::BitstreamEntry::SubBlock:
case llvm::BitstreamEntry::EndBlock:
@@ -61,8 +66,12 @@
Record.clear();
StringRef Blob;
- unsigned RecCode = Stream.readRecord(Entry.ID, Record, &Blob);
- switch (RecCode) {
+ Expected<unsigned> MaybeRecCode =
+ Stream.readRecord(Entry.ID, Record, &Blob);
+ if (!MaybeRecCode)
+ fprintf(stderr, "Failed reading rec code: %s\n",
+ toString(MaybeRecCode.takeError()).c_str());
+ switch (MaybeRecCode.get()) {
case FIRST_EXTENSION_RECORD_ID: {
StringRef Message = Blob.substr(0, Record[0]);
fprintf(stderr, "Read extension block message: %s\n",