Return Error instead of bool from mergeTypeStreams().

[ruiu]

Summary

Previously, mergeTypeStreams returns only true or false, so it was
impossible to know the reason if it failed. This patch changes the
function signature so that it returns an Error object.

[zturner]

llvm/include/llvm/DebugInfo/CodeView/TypeStreamMerger.h

16

Should probably #include "llvm/Support/Error.h" here.

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

104–105

If there is more than one error, this will trigger an exception because you will have not checked the previous value of the error. Instead of storing an Optional<Error> maybe try storing just an Error, and then saying

LastError = llvm::join_errors(std::move(LastError), llvm::make_error<CodeViewError>(cv_error_code::corrupt_record));

188

If you do the above, you can simply return LastError here.

[ruiu]

• Updated as per Zach's review comments.

[zturner]

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

62

Shouldn't we do this in the destructor instead?

[ruiu]

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

176

No, because we overwrite it unconditionally here.

[zturner]

Hmm, that's a bummer. In that case I guess I like using the Optional<> better. More comments inline.

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

176

Before this, add assert(!LastError.hasValue());

188

Then this can change to

Error Result = std::move(*LastError);
LastError.reset();
return std::move(Result);

[ruiu]

• Use llvm::Optional<Error> instead of Error.

lld/COFF/PDB.cpp

for (ObjectFile *File : Symtab->ObjectFiles) {
     if (Data.empty())
       continue;
 
     msf::ByteStream Stream(Data);
     codeview::CVTypeArray Types;
     msf::StreamReader Reader(Stream);
     if (auto EC = Reader.readArray(Types, Reader.getLength()))
       fatal(EC, "Reader::readArray failed");
-    if (!codeview::mergeTypeStreams(Builder, Types))
-      fatal("codeview::mergeTypeStreams failed");
+    if (auto Err = codeview::mergeTypeStreams(Builder, Types))
+      fatal(Err, "codeview::mergeTypeStreams failed");
   }
 
   // Construct section contents.
   std::vector<uint8_t> V;
   Builder.ForEachRecord([&](TypeIndex TI, ArrayRef<uint8_t> Rec) {
     V.insert(V.end(), Rec.begin(), Rec.end());
   });
   return V;

llvm/include/llvm/DebugInfo/CodeView/TypeStreamMerger.h

 //===- TypeStreamMerger.h ---------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_CODEVIEW_TYPESTREAMMERGER_H
 #define LLVM_DEBUGINFO_CODEVIEW_TYPESTREAMMERGER_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/DebugInfo/CodeView/TypeTableBuilder.h"
+#include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace codeview {
 
 /// Merges one type stream into another. Returns true on success.
-bool mergeTypeStreams(TypeTableBuilder &DestStream, const CVTypeArray &Types);
+Error mergeTypeStreams(TypeTableBuilder &DestStream, const CVTypeArray &Types);
 
 } // end namespace codeview
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_CODEVIEW_TYPESTREAMMERGER_H

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

 ///   type index, and update the two hash tables.
 /// - If the type record already exists in the destination stream, discard it
 ///   and update the type index map to forward the source type index to the
 ///   existing destination type index.
 class TypeStreamMerger : public TypeVisitorCallbacks {
 public:
   TypeStreamMerger(TypeTableBuilder &DestStream)
       : DestStream(DestStream), FieldListBuilder(DestStream) {
-    assert(!hadError());
+    // The initial value of LastError have no significance,
+    // but we still need to set its internal "isChecked" flag.
+    // Otherwise it will raise a runtime error. So we do that here.
+    (void)static_cast<bool>(LastError);
   }
 
 /// TypeVisitorCallbacks overrides.
 #define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
   Error visitKnownRecord(CVType &CVR, Name##Record &Record) override;
 #define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
   Error visitKnownMember(CVMemberRecord &CVR, Name##Record &Record) override;
 #define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #include "llvm/DebugInfo/CodeView/TypeRecords.def"
 
   Error visitUnknownType(CVType &Record) override;
 
   Error visitTypeBegin(CVType &Record) override;
   Error visitTypeEnd(CVType &Record) override;
   Error visitMemberEnd(CVMemberRecord &Record) override;
 
-  bool mergeStream(const CVTypeArray &Types);
+  Error mergeStream(const CVTypeArray &Types);
 
 private:
   template <typename RecordType>
   Error visitKnownRecordImpl(RecordType &Record) {
-    FoundBadTypeIndex |= !Record.remapTypeIndices(IndexMap);
+    if (!Record.remapTypeIndices(IndexMap))
+      LastError = joinErrors(
+          std::move(LastError),
+          llvm::make_error<CodeViewError>(cv_error_code::corrupt_record));
     IndexMap.push_back(DestStream.writeKnownType(Record));
     return Error::success();
   }
 
   Error visitKnownRecordImpl(FieldListRecord &Record) {
     CVTypeVisitor Visitor(*this);
 
     if (auto EC = Visitor.visitFieldListMemberStream(Record.Data))
       return EC;
     return Error::success();
   }
 
   template <typename RecordType>
   Error visitKnownMemberRecordImpl(RecordType &Record) {
-    FoundBadTypeIndex |= !Record.remapTypeIndices(IndexMap);
+    if (!Record.remapTypeIndices(IndexMap))
+      LastError = joinErrors(
+          std::move(LastError),
+          llvm::make_error<CodeViewError>(cv_error_code::corrupt_record));
     FieldListBuilder.writeMemberType(Record);
     return Error::success();
   }
 
-  bool hadError() { return FoundBadTypeIndex; }
-
-  bool FoundBadTypeIndex = false;
+  Error LastError = Error::success();
 
   BumpPtrAllocator Allocator;
 
   TypeTableBuilder &DestStream;
   FieldListRecordBuilder FieldListBuilder;
 
   bool IsInFieldList{false};
   size_t BeginIndexMapSize = 0;

 
 Error TypeStreamMerger::visitUnknownType(CVType &Rec) {
   // We failed to translate a type. Translate this index as "not translated".
   IndexMap.push_back(
       TypeIndex(SimpleTypeKind::NotTranslated, SimpleTypeMode::Direct));
   return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
 }
 
-bool TypeStreamMerger::mergeStream(const CVTypeArray &Types) {
+Error TypeStreamMerger::mergeStream(const CVTypeArray &Types) {
   assert(IndexMap.empty());
   TypeVisitorCallbackPipeline Pipeline;
+  LastError = Error::success();
 
   TypeDeserializer Deserializer;
   Pipeline.addCallbackToPipeline(Deserializer);
   Pipeline.addCallbackToPipeline(*this);
 
   CVTypeVisitor Visitor(Pipeline);
 
-  if (auto EC = Visitor.visitTypeStream(Types)) {
-    consumeError(std::move(EC));
-    return false;
-  }
+  if (auto EC = Visitor.visitTypeStream(Types))
+    return EC;
   IndexMap.clear();
-  return !hadError();
+
+  return std::move(LastError);
 }
 
-bool llvm::codeview::mergeTypeStreams(TypeTableBuilder &DestStream,
+Error llvm::codeview::mergeTypeStreams(TypeTableBuilder &DestStream,
                                        const CVTypeArray &Types) {
   return TypeStreamMerger(DestStream).mergeStream(Types);
 }