[PATCH] D63518: WIP BitStream reader: propagate errors

[Bruno Cardoso Lopes via Phabricator via llvm-commits]

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