Author: jfb Date: Wed Jun 26 12:50:12 2019 New Revision: 364464 URL: http://llvm.org/viewvc/llvm-project?rev=364464&view=rev Log: 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 Modified: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp clang-tools-extra/trunk/clang-doc/BitcodeWriter.h clang-tools-extra/trunk/clangd/ClangdUnit.cpp clang-tools-extra/trunk/clangd/CodeComplete.cpp clang-tools-extra/trunk/clangd/index/Background.cpp clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp Modified: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp (original) +++ clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp Wed Jun 26 12:50:12 2019 @@ -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::readB 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::skipUntilRecordOr 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::valid 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); Modified: clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp (original) +++ clang-tools-extra/trunk/clang-doc/BitcodeWriter.cpp Wed Jun 26 12:50:12 2019 @@ -214,7 +214,7 @@ static const std::vector<std::pair<Block // AbbreviationMap -constexpr char BitCodeConstants::Signature[]; +constexpr unsigned char BitCodeConstants::Signature[]; void ClangDocBitcodeWriter::AbbreviationMap::add(RecordId RID, unsigned AbbrevID) { Modified: clang-tools-extra/trunk/clang-doc/BitcodeWriter.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/BitcodeWriter.h?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-doc/BitcodeWriter.h (original) +++ clang-tools-extra/trunk/clang-doc/BitcodeWriter.h Wed Jun 26 12:50:12 2019 @@ -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; }; Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original) +++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Wed Jun 26 12:50:12 2019 @@ -422,8 +422,9 @@ ParsedAST::build(std::unique_ptr<Compile // 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. Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original) +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Wed Jun 26 12:50:12 2019 @@ -1105,8 +1105,9 @@ bool semaCodeComplete(std::unique_ptr<Co 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(); Modified: clang-tools-extra/trunk/clangd/index/Background.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/index/Background.cpp (original) +++ clang-tools-extra/trunk/clangd/index/Background.cpp Wed Jun 26 12:50:12 2019 @@ -456,9 +456,9 @@ llvm::Error BackgroundIndex::index(tooli 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()) { Modified: clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp?rev=364464&r1=364463&r2=364464&view=diff ============================================================================== --- clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp (original) +++ clang-tools-extra/trunk/clangd/unittests/HeadersTests.cpp Wed Jun 26 12:50:12 2019 @@ -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; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
