jfb added a comment.
In D63518#1550827 <https://reviews.llvm.org/D63518#1550827>, @bruno wrote:
> Hi JF. Thanks for working on this, nice improvement to error handling!
>
> The overall approach is pretty solid and should prevent a lot of red herring
> while investigating hard to reproduce crashes in clang, specially when
> implicit clang modules is involved. Dropping the errors on the floor for
> previous code that didn't handle errors at all is a fair tradeoff for
> introducing the functionality.
Thanks!
> I have omitted any format comments but I noticed several of 80 cols
> violations. More specific reviews inline.
Yeah I've been avoiding `clang-format` because it's sometimes easier to manage
merge conflicts without reformatting. I'll absolutely run format when the patch
is further along.
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:59
+ } else
+ return SDError::InvalidDiagnostics; // FIXME propagate the error
details.
----------------
bruno wrote:
> Can this be simplified as below?
>
> ```
> Expected<unsigned> Res = Stream.ReadCode();
> if (!Res || Res.get() != llvm::bitc::ENTER_SUBBLOCK)
> return SDError::InvalidDiagnostics; // FIXME propagate the error details.
> ```
Yeah, but I think we'll want to propagate errors in a follow-up so we'll end up
re-separating them. I'd rather have the right structure here.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1158
StringRef Blob;
- unsigned Code = Cursor.ReadCode();
- unsigned RecCode = Cursor.readRecord(Code, Record, &Blob);
+ Expected<unsigned> MaybeCode = Cursor.ReadCode();
+ if (!MaybeCode) {
----------------
bruno wrote:
> Not necessarily needed as part of this patch, but I wonder how many of
> repetitive access patterns (readCode + readRecord, and maybe other patterns)
> we have that would take advantage of refactoring all these checks out into
> their own methods.
Yeah I noticed that too, probably worth adding a helper for. I'll note it in a
bug: https://bugs.llvm.org/show_bug.cgi?id=42335
================
Comment at: clang/lib/Serialization/ASTReader.cpp:4283
+ return llvm::createStringError(std::errc::illegal_byte_sequence, "file too
small to contain AST file magic");
+ for (unsigned C : {'C','P','C','H'})
+ if (Expected<llvm::SimpleBitstreamCursor::word_t> Res = Stream.Read(8)) {
----------------
bruno wrote:
> Very similar to SerializedDiagnosticReader.cpp:44, does it deserve a common
> helper?
It would be nice, but they don't have the same error handling :(
I'll add a FIXME.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:4559
+ // FIXME this drops the error.
+ return Failure;
+ }
----------------
bruno wrote:
> This is a good example of a real issue this patch solves. Sometimes we get
> signature mismatch problems in implicit modules builds because we read
> garbage. Having this check and failure here prevents the misleading error
> message.
🎉
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3695
+ if (!MaybeDeclCode)
+ llvm::report_fatal_error("ASTReader::ReadDeclRecord failed readung decl
code: " + toString(MaybeDeclCode.takeError()));
+ switch ((DeclCode)MaybeDeclCode.get()) {
----------------
bruno wrote:
> typo on `readung`
lol dung
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:4036
+ if (llvm::Error JumpFailed = Cursor.JumpToBit(Offset))
+ // FIXME don't do a fatal error.
+ llvm::report_fatal_error("ASTReader::loadDeclUpdateRecords failed
jumping: " + toString(std::move(JumpFailed)));
----------------
bruno wrote:
> Why? What's a better alternative?
Anything that can be used as a clang API needs to return errors instead of
using `report_fatal_error`, so that the API can't just `abort` your process. We
should propagate this error, but it's getting tedious here and I think better
in a follow-up.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:4119
+ if (Expected<unsigned> MaybeRecCode = Cursor.readRecord(Code, Record))
+ assert(MaybeRecCode.get() == LOCAL_REDECLARATIONS && "expected
LOCAL_REDECLARATIONS record!");
+ else
----------------
bruno wrote:
> Does this builds fine without assertions?
It should, why? The variable is used on both sides of the `if`.
================
Comment at: clang/lib/Serialization/GlobalModuleIndex.cpp:266
// Sniff for the signature.
- if (Cursor.Read(8) != 'B' ||
- Cursor.Read(8) != 'C' ||
- Cursor.Read(8) != 'G' ||
- Cursor.Read(8) != 'I') {
- return std::make_pair(nullptr, EC_IOError);
+ for (unsigned char C : {'B', 'C', 'G', 'I'}) {
+ if (Expected<llvm::SimpleBitstreamCursor::word_t> Res = Cursor.Read(8)) {
----------------
bruno wrote:
> Very similar to SerializedDiagnosticReader.cpp:44, does it deserve a common
> helper?
Left a FIXME in ASTReader.cpp for this.
================
Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:230
+ uint32_t Piece;
+ if (Expected<unsigned> Res = Read(NumBits))
+ Piece = Res.get();
----------------
bruno wrote:
> Alternatively, you can go early return mode for this and other error checking
> in BitstreamReader.h
>
> ```
> Expected<unsigned> Res = Read(NumBits);
> if (!Res)
> return Res;
> Piece = Res.get();
> ```
Yeah that's the first code I fixed, and later moved to what you suggest.
Updated here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63518/new/
https://reviews.llvm.org/D63518
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
