[PATCH] D63518: BitStream reader: propagate errors
Lang Hames via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 25 15:03:50 PDT 2019
lhames added a comment.
I haven't had a chance to audit the whole patch yet, but in general the error suppression idioms are unsafe (though maybe no more so than the existing code?).
I would be inclined to audit all those FIXMEs and replace them with cantFails or consumeErrors. consumeError will generally match existing behavior in places where you were ignoring errors. cantFail will get you aggressive crashes, which can be handy for debugging but not so fun in release code.
Also, if this patch passes the regression tests we need more failure tests. :)
================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+ if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+ if (llvm::Error Skipped = Stream.SkipBlock())
+ return Skipped;
----------------
Bigcheese wrote:
> This is a pretty big behavior change. Is clang-doc expecting to get files with unknown blocks?
The inner test here is unsafe, as it will discard the outer error. You need:
if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
consumeError(std::move(Err));
return Skipped;
}
return Err;
}
or:
if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
return joinErrors(std::move(Err), std::move(Skipped));
return Err;
}
================
Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+ Expected<unsigned> MaybeCode = Stream.ReadCode();
+ if (!MaybeCode) {
+ // FIXME this drops the error on the floor.
+ consumeError(MaybeCode.takeError());
+ }
+
+ // FIXME check that the enum is in range.
----------------
Bigcheese wrote:
> Does `consumeError` return? `MaybeCode.get()` will crash if `!MaybeCode`.
Yes: consumeError returns. This should probably be:
if (!MaybeCode)
return consumeError(MaybeCode.takeError()), Cursor::Badness;
================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+ // 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).
----------------
This will crash if writeIndex ever generates an error. For that reason I would suggest writing this as:
// FIXME: Can actually fail! <fixme goes here>
cantFail(GlobalModuleIndex::writeIndex(...));
It has the same effect, but without the control flow.
================
Comment at: clang/lib/Frontend/CompilerInstance.cpp:2082
+ getPreprocessor().getHeaderSearchInfo().getModuleCachePath())) {
+ // FIXME As above, this drops the error on the floor.
+ }
----------------
Another cantFail candidate.
================
Comment at: clang/lib/Frontend/FrontendAction.cpp:945-948
+ // 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));
----------------
I'd suggest cantFail here, if not for that fixme comment. Yikes.
================
Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:132-133
CI.getLangOpts(), FixItOpts.get());
- FixAction->Execute();
+ if (llvm::Error Err = FixAction->Execute())
+ err = true; // FIXME this drops the error on the floor.
----------------
You need a consumeError here or you'll get a runtime crash if an error is generated.
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:89-91
+ if (llvm::Error Err = Stream.SkipBlock())
+ return SDError::MalformedTopLevelBlock; // FIXME propagate the error
+ // details.
----------------
This needs a consumeError or it will crash if an error is generated. How about:
if (llvm::Error Err = Stream.SkipBlock())
return consumeError(std::move(Err)), SDError::MalformedTopLevelBlock;
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:149-151
+ if (llvm::Error Err =
+ Stream.EnterSubBlock(clang::serialized_diags::BLOCK_META))
+ return SDError::MalformedMetadataBlock; // FIXME propagate the error
----------------
Needs a consumeError.
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:166-167
case Cursor::BlockBegin:
- if (Stream.SkipBlock())
- return SDError::MalformedMetadataBlock;
+ if (llvm::Error Err = Stream.SkipBlock())
+ return SDError::MalformedMetadataBlock; // FIXME propagate the error
+ // details.
----------------
Needs a consumeError.
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:194-196
+ if (llvm::Error Err =
+ Stream.EnterSubBlock(clang::serialized_diags::BLOCK_DIAG))
+ return SDError::MalformedDiagnosticBlock; // FIXME propagate the error
----------------
Needs a consumeError.
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:216-217
return EC;
- } else if (!Stream.SkipBlock())
- return SDError::MalformedSubBlock;
+ } else if (llvm::Error Err = Stream.SkipBlock())
+ return SDError::MalformedSubBlock; // FIXME propagate the error details.
continue;
----------------
Needs a consumeError.
================
Comment at: clang/lib/Frontend/TestModuleFileExtension.cpp:53-54
+ Stream.advanceSkippingSubblocks();
+ if (!MaybeEntry)
+ (void)MaybeEntry.takeError();
+ llvm::BitstreamEntry Entry = MaybeEntry.get();
----------------
Needs a consumeError or cantFail.
Fun fact: cantFail will auto-unwrap Expecteds:
Expected<T> foo();
T v = cantFail(foo());
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1620
+ if (llvm::Error Err = Cursor.EnterSubBlock(BlockID)) {
+ // FIXME this drops errors on the floor.
return true;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1628
+ if (!MaybeCode) {
+ // FIXME this drops errors on the floor.
+ return true;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1636
+ if (llvm::Error Err = Cursor.JumpToBit(Offset)) {
+ // FIXME this drops errors on the floor.
+ return true;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1642
+ if (llvm::Error Err = Cursor.ReadAbbrevRecord()) {
+ // FIXME this drops errors on the floor.
+ return true;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1669
+ if (llvm::Error Err = Stream.JumpToBit(Offset)) {
+ // FIXME this drops errors on the floor.
+ return nullptr;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:2208
+ if (!MaybeCode) {
+ // FIXME this drops errors on the floor.
+ }
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:2218
+ else {
+ // FIXME this drops errors on the floor.
+ }
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:2250
+ if (llvm::Error Err = Cursor.JumpToBit(F.InputFileOffsets[ID - 1])) {
+ // FIXME this drops errors on the floor.
+ }
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:2401
+ if (llvm::Error Err = Stream.EnterSubBlock(OPTIONS_BLOCK_ID)) {
+ // FIXME this drops errors on the floor.
return Failure;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:2411
+ if (!MaybeEntry) {
+ // FIXME this drops errors on the floor.
+ return Failure;
----------------
Unsafe on error. Needs cantFail or consumeError.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:2433
+ if (!MaybeRecordType) {
+ // FIXME this drops errors on the floor.
+ return Failure;
----------------
Unsafe on error. Needs cantFail or consumeError.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63518/new/
https://reviews.llvm.org/D63518
More information about the llvm-commits
mailing list