[PATCH] D63518: BitStream reader: propagate errors

bjope added inline comments. Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 +// FIXME check that the enum is in range. +auto Code = static_cast(MaybeCode.get()); + jfb wrote: > jfb wrote: > > bjope wrote: > > > This has caused problem

[PATCH] D63518: BitStream reader: propagate errors

jfb marked an inline comment as done. jfb added inline comments. Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 +// FIXME check that the enum is in range. +auto Code = static_cast(MaybeCode.get()); + jfb wrote: > bjope wrote: > > This

[PATCH] D63518: BitStream reader: propagate errors

jfb marked 2 inline comments as done. jfb added inline comments. Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 +// FIXME check that the enum is in range. +auto Code = static_cast(MaybeCode.get()); + bjope wrote: > This has caused pro

[PATCH] D63518: BitStream reader: propagate errors

bjope added inline comments. Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619 +// FIXME check that the enum is in range. +auto Code = static_cast(MaybeCode.get()); + This has caused problem for our sanitizer tests the last couple of day

[PATCH] D63518: BitStream reader: propagate errors

jfb marked 2 inline comments as done. jfb added a subscriber: hans. jfb added inline comments. Comment at: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp:205 + return MaybeBitCode.takeError(); +switch (unsigned BitCode = MaybeBitCode.get()) { default: // Default be

[PATCH] D63518: BitStream reader: propagate errors

abrachet added inline comments. Comment at: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp:205 + return MaybeBitCode.takeError(); +switch (unsigned BitCode = MaybeBitCode.get()) { default: // Default behavior: reject This and an identical switch on

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. I think this is ready to go! I rebased and ran `check-all` for LLVM / clang / clang-tools-extras and everything passes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/D63518 __

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. I've addressed Lang's comments and the re-audited all the new FIXME instances. This patch now actually drops errors on the floor instead of implicitly erroring out unless the error path is tested (which is what I had before). This hides bugs, but I left FIXMEs everywhere an

[PATCH] D63518: BitStream reader: propagate errors

jfb added inline comments. Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618 +Expected MaybeCode = Stream.ReadCode(); +if (!MaybeCode) { + // FIXME this drops the error on the floor. + consumeError(MaybeCode.takeError()); +} + +// FIX

[PATCH] D63518: BitStream reader: propagate errors

thegameg added inline comments. Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489 bool ReadBlockEnd() { if (BlockScope.empty()) return true; jfb wrote: > thegameg wrote: > > Any reason why this doesn't return `Error`? > I'm not sure it's reall

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. In D63518#1558197 , @lhames wrote: > 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

[PATCH] D63518: BitStream reader: propagate errors

jfb marked 8 inline comments as done. jfb added inline comments. 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; --

[PATCH] D63518: BitStream reader: propagate errors

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 wil

[PATCH] D63518: BitStream reader: propagate errors

Bigcheese added inline comments. 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; This is a pretty big

[PATCH] D63518: BitStream reader: propagate errors

thegameg added inline comments. Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:441 // If we found a sub-block, just skip over it and check the next entry. - if (SkipBlock()) -return BitstreamEntry::getError(); + if (llvm::Error Err = SkipBlock())

[PATCH] D63518: BitStream reader: propagate errors

bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63518/new/ https://reviews.llvm.org/D63518 ___ c

[PATCH] D63518: BitStream reader: propagate errors

jfb added a comment. I did a build of all LLVM monorepo projects, and only LLVM / clang / clang-tools-extra are impacted. I therefore ran tests as follows, and they all pass: rm -rf debug ; mkdir debug && (cd debug && cmake -G Ninja ../llvm -DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_BUILD_TYPE=Debu