[PATCH] D63518: WIP BitStream reader: propagate errors

[JF Bastien via Phabricator via llvm-commits]

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