bruno added a comment.
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. I have omitted any format comments but I noticed several of 80
cols violations. More specific reviews inline.
================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:356
+ if (llvm::Error Err = Act->Execute())
+ return errorToErrorCode(std::move(Err));
----------------
Changes like this are so much better!
================
Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:59
+ } else
+ return SDError::InvalidDiagnostics; // FIXME propagate the error
details.
----------------
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.
```
================
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) {
----------------
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.
================
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)) {
----------------
Very similar to SerializedDiagnosticReader.cpp:44, does it deserve a common
helper?
================
Comment at: clang/lib/Serialization/ASTReader.cpp:4559
+ // FIXME this drops the error.
+ return Failure;
+ }
----------------
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()) {
----------------
typo on `readung`
================
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)));
----------------
Why? What's a better alternative?
================
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
----------------
Does this builds fine without assertions?
================
Comment at: clang/lib/Serialization/GlobalModuleIndex.cpp:133
+ report_fatal_error("Module index '" + Buffer->getBufferIdentifier() + "'
failed: " + toString(std::move(Err)));
+ };
+
----------------
Maybe use a similar helper for error checking added in ASTReaderDecl.cpp?
================
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)) {
----------------
Very similar to SerializedDiagnosticReader.cpp:44, does it deserve a common
helper?
================
Comment at: clang/lib/Serialization/GlobalModuleIndex.cpp:535
// Sniff for the signature.
- if (InStream.Read(8) != 'C' ||
- InStream.Read(8) != 'P' ||
- InStream.Read(8) != 'C' ||
- InStream.Read(8) != 'H') {
- return true;
- }
+ for (unsigned char C : {'C','P','C','H'})
+ if (Expected<llvm::SimpleBitstreamCursor::word_t> Res = InStream.Read(8)) {
----------------
Very similar to SerializedDiagnosticReader.cpp:44, does it deserve a common
helper?
================
Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:230
+ uint32_t Piece;
+ if (Expected<unsigned> Res = Read(NumBits))
+ Piece = Res.get();
----------------
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();
```
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
