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

98–99

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));

180

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

60

Shouldn't we do this in the destructor instead?

[ruiu]

llvm/lib/DebugInfo/CodeView/TypeStreamMerger.cpp

169

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

169

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

180

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 EC = codeview::mergeTypeStreams(Builder, Types))
+      fatal(EC, "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"
 
 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

 ///   hash table, append it to the destination type stream, assign it the next
 ///   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());
-  }
+      : DestStream(DestStream), FieldListBuilder(DestStream) {}
 
 /// 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 =
+          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 =
+          llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
     FieldListBuilder.writeMemberType(Record);
     return Error::success();
   }
 
-  bool hadError() { return FoundBadTypeIndex; }
-
-  bool FoundBadTypeIndex = false;
+  Optional<Error> LastError;
 
   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;
 
   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();
+
+  if (LastError.hasValue()) {
+    Error E = std::move(*LastError);
+    LastError.reset();
+    return E;
+  }
+  return Error::success();
 }
 
-bool llvm::codeview::mergeTypeStreams(TypeTableBuilder &DestStream,
+Error llvm::codeview::mergeTypeStreams(TypeTableBuilder &DestStream,
                                        const CVTypeArray &Types) {
   return TypeStreamMerger(DestStream).mergeStream(Types);
 }