[pdb] Change type visitor pattern to use dynamic polymorphism

[zturner]

Summary

This gets rid of the CRTP (curiously recurring template pattern) on the CTypeVisitor, and instead changes to a method of dynamic polymorphism to call back into the visitor implementation.

This has a number of advantages, including:

1. Ability to implement CTypeVisitor in a .cpp file instead of a .h file
2. Removal of a bunch of methods and code duplication since functionality of CTypeVisitorImpl can be merged with functionality of TypeDumper.
3. It's possible to implement a simple visitor with MUCH less code than before. Since the base class provides virtual methods with default implementations, if all you want to do is handle a few simple method types, you need only override the few methods you care about. This can lead to much less code when writing a new visitation handler. Previously you would have to do the #define magic and implement every method in the class, and if you only wanted 2 or 3 methods to do something, there would be a lot of unnecessary function definitions.
4. More robust error propagation. Since we don't rely on saving error state in a boolean anymore, we can simply propagate errors all the way out to the top level with actual llvm::Errors. This allows the implementations to propagate more information up the call stack.

[ruiu]

LGTM

lib/DebugInfo/CodeView/CVTypeVisitor.cpp

1

Remove -*- C++ -*- since this file has .cpp extnesion.

21

This is not directly related to this change, but should we write return {} instead of return Error::success() for the sake of brevity?

[zturner]

I can't check the code at the moment but somewhere in Error.h it
specifically recommends using the explicit Error::success syntax

[rnk]

I guess on the whole I'm not in favor of this.

1. Ability to implement CTypeVisitor in a .cpp file instead of a .h file

I don't see how this wasn't already possible. I could have merged CVTypeDumperImpl and CVTypeDumper if I wanted to, but I was trying to hide as many implementation details as possible and minimize the header.

2. Removal of a bunch of methods and code duplication since functionality of CTypeVisitorImpl can be merged with functionality of TypeDumper.

CVTypeDumperImpl and CVTypeDumper don't have that much code duplication IMO. The other nice thing about the way this was structured is that clients of TypeDumper.h didn't depend on the visitor, and all of the methods of the type dumper were in an anonymous namespace, giving them internal linkage, reducing binary size, etc.

3. It's possible to implement a simple visitor with MUCH less code than before. Since the base class provides virtual methods with default implementations, if all you want to do is handle a few simple method types, you need only override the few methods you care about. This can lead to much less code when writing a new visitation handler. Previously you would have to do the #define magic and implement every method in the class, and if you only wanted 2 or 3 methods to do something, there would be a lot of unnecessary function definitions.

This should have already been true. You don't have to use TypeRecord.def to declare every kind of visit method, you should be able to inherit the no-op visit##Kind method from the visitor.

4. More robust error propagation. Since we don't rely on saving error state in a boolean anymore, we can simply propagate errors all the way out to the top level with actual llvm::Errors. This allows the implementations to propagate more information up the call stack.

This is great, but it only requires changing the return types of the visitor to Error.

[zturner]

I can't respond inline from mobile for some reason, but here's my comments:

1. it wasn't possible before because CVTypeVisitor was a template class, so

it had to be implemented in the header. You could have merged them, yes,
but this would couple the visitation switch delegation with the handlers,
making it not reusable and defeating the whole purpose. This way, the
header is even more minimized than before, including the removal of many
#includes from the header, which is always a win.

I guess in general, I'm not in favor of templatizing anything unless the
advantages are very compelling (significantly less code, higher
performance, greater reusability, etc). If there's no practical difference,
i prefer dynamic polymorphism purely on readability and better compiler
errors.

2. clients of TypeDumper.h still don't depend on the visitor, but they do

have access to the visitor should they want to use it. Internal linkage is
fine and has some advantages, but obviously hurts reusability (albeit by
design)

[zturner]

Rebases on top of Rui's recent patches.

One added advantage of this approach is that, while rebasing, I actually caught a couple of bugs in Rui's patch that would not have been possible to catch at compile time otherwise. Mainly there were previously separate visitation functions for class, struct, and interface, but these are all implemented by a single record type, and thus have only a single visitation function. This was caught because when I added the override keyword to them, the compiler error'ed saying that there was no suitable function to override.

[rnk]

In D21410#460027, @zturner wrote:

One added advantage of this approach is that, while rebasing, I actually caught a couple of bugs in Rui's patch that would not have been possible to catch at compile time otherwise. Mainly there were previously separate visitation functions for class, struct, and interface, but these are all implemented by a single record type, and thus have only a single visitation function. This was caught because when I added the override keyword to them, the compiler error'ed saying that there was no suitable function to override.

Acking on the mailing list that I buy this reasoning, lgtm

include/llvm/DebugInfo/CodeView/CVTypeVisitor.h

 //===- CVTypeVisitor.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_CVTYPEVISITOR_H
 #define LLVM_DEBUGINFO_CODEVIEW_CVTYPEVISITOR_H
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
-#include "llvm/DebugInfo/CodeView/CodeView.h"
-#include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
-#include "llvm/Support/ErrorOr.h"
+#include "llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h"
+#include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace codeview {
 
-template <typename Derived>
 class CVTypeVisitor {
 public:
-  CVTypeVisitor() {}
+  explicit CVTypeVisitor(TypeVisitorCallbacks &Callbacks);
 
-  bool hadError() const { return HadError; }
-
-  template <typename T>
-  bool consumeObject(ArrayRef<uint8_t> &Data, const T *&Res) {
-    if (Data.size() < sizeof(*Res)) {
-      HadError = true;
-      return false;
-    }
-    Res = reinterpret_cast<const T *>(Data.data());
-    Data = Data.drop_front(sizeof(*Res));
-    return true;
-  }
-
-  /// Actions to take on known types. By default, they do nothing. Visit methods
-  /// for member records take the FieldData by non-const reference and are
-  /// expected to consume the trailing bytes used by the field.
-  /// FIXME: Make the visitor interpret the trailing bytes so that clients don't
-  /// need to.
-#define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
-  void visit##Name(TypeLeafKind LeafType, Name##Record &Record) {}
-#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
-#define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
-  void visit##Name(TypeLeafKind LeafType, Name##Record &Record) {}
-#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
-#include "TypeRecords.def"
-
-  void visitTypeRecord(const CVRecord<TypeLeafKind> &Record) {
-    ArrayRef<uint8_t> LeafData = Record.Data;
-    ArrayRef<uint8_t> RecordData = LeafData;
-    auto *DerivedThis = static_cast<Derived *>(this);
-    DerivedThis->visitTypeBegin(Record.Type, RecordData);
-    switch (Record.Type) {
-    default:
-      DerivedThis->visitUnknownType(Record.Type, RecordData);
-      break;
-    case LF_FIELDLIST:
-      DerivedThis->visitFieldList(Record.Type, LeafData);
-      break;
-#define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
-  case EnumName: {                                                             \
-    TypeRecordKind RK = static_cast<TypeRecordKind>(EnumName);                 \
-    auto Result = Name##Record::deserialize(RK, LeafData);                     \
-    if (Result.getError())                                                     \
-      return parseError();                                                     \
-    DerivedThis->visit##Name(Record.Type, *Result);                            \
-    break;                                                                     \
-  }
-#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)                  \
-  TYPE_RECORD(EnumVal, EnumVal, AliasName)
-#define MEMBER_RECORD(EnumName, EnumVal, Name)
-#include "TypeRecords.def"
-      }
-      DerivedThis->visitTypeEnd(Record.Type, RecordData);
-  }
+  Error visitTypeRecord(const CVRecord<TypeLeafKind> &Record);
 
   /// Visits the type records in Data. Sets the error flag on parse failures.
-  void visitTypeStream(const CVTypeArray &Types) {
-    for (const auto &I : Types) {
-      visitTypeRecord(I);
-      if (hadError())
-        break;
-    }
-  }
-
-  /// Action to take on unknown types. By default, they are ignored.
-  void visitUnknownType(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData) {}
-
-  /// Paired begin/end actions for all types. Receives all record data,
-  /// including the fixed-length record prefix.
-  void visitTypeBegin(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData) {}
-  void visitTypeEnd(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData) {}
-
-  ArrayRef<uint8_t> skipPadding(ArrayRef<uint8_t> Data) {
-    if (Data.empty())
-      return Data;
-    uint8_t Leaf = Data.front();
-    if (Leaf < LF_PAD0)
-      return Data;
-    // Leaf is greater than 0xf0. We should advance by the number of bytes in
-    // the low 4 bits.
-    unsigned BytesToAdvance = Leaf & 0x0F;
-    if (Data.size() < BytesToAdvance) {
-      parseError();
-      return None;
-    }
-    return Data.drop_front(BytesToAdvance);
-  }
+  Error visitTypeStream(const CVTypeArray &Types);
+
+  Error skipPadding(ArrayRef<uint8_t> &Data);
 
   /// Visits individual member records of a field list record. Member records do
   /// not describe their own length, and need special handling.
-  void visitFieldList(TypeLeafKind Leaf, ArrayRef<uint8_t> FieldData) {
-    auto *DerivedThis = static_cast<Derived *>(this);
-    while (!FieldData.empty()) {
-      const ulittle16_t *LeafPtr;
-      if (!CVTypeVisitor::consumeObject(FieldData, LeafPtr))
-        return;
-      TypeLeafKind Leaf = TypeLeafKind(unsigned(*LeafPtr));
-      switch (Leaf) {
-      default:
-        // Field list records do not describe their own length, so we cannot
-        // continue parsing past an unknown member type.
-        DerivedThis->visitUnknownMember(Leaf);
-        return parseError();
-#define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
-  case EnumName: {                                                             \
-    TypeRecordKind RK = static_cast<TypeRecordKind>(EnumName);                 \
-    auto Result = Name##Record::deserialize(RK, FieldData);                    \
-    if (Result.getError())                                                     \
-      return parseError();                                                     \
-    DerivedThis->visit##Name(Leaf, *Result);                                   \
-    break;                                                                     \
-  }
-#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)                \
-  MEMBER_RECORD(EnumVal, EnumVal, AliasName)
-#include "TypeRecords.def"
-      }
-      FieldData = skipPadding(FieldData);
-      if (hadError())
-        break;
-    }
-  }
-
-  /// Action to take on unknown members. By default, they are ignored. Member
-  /// record parsing cannot recover from an unknown member record, so this
-  /// method is only called at most once per field list record.
-  void visitUnknownMember(TypeLeafKind Leaf) {}
-
-  /// Helper for returning from a void function when the stream is corrupted.
-  void parseError() { HadError = true; }
+  Error visitFieldList(TypeLeafKind Leaf, ArrayRef<uint8_t> FieldData);
 
 private:
-  /// Whether a type stream parsing error was encountered.
-  bool HadError = false;
+  /// The interface to the class that gets notified of each visitation.
+  TypeVisitorCallbacks &Callbacks;
 };
 
 } // end namespace codeview
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_CODEVIEW_CVTYPEVISITOR_H

include/llvm/DebugInfo/CodeView/TypeDumper.h

 //===-- TypeDumper.h - CodeView type info dumper ----------------*- 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_TYPEDUMPER_H
 #define LLVM_DEBUGINFO_CODEVIEW_TYPEDUMPER_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
+#include "llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h"
 
 namespace llvm {
 class ScopedPrinter;
 
 namespace codeview {
 
 /// Dumper for CodeView type streams found in COFF object files and PDB files.
-class CVTypeDumper {
+class CVTypeDumper : public TypeVisitorCallbacks {
 public:
   CVTypeDumper(ScopedPrinter *W, bool PrintRecordBytes)
       : W(W), PrintRecordBytes(PrintRecordBytes) {}
 
   StringRef getTypeName(TypeIndex TI);
   void printTypeIndex(StringRef FieldName, TypeIndex TI);
 
   /// Dumps one type record.  Returns false if there was a type parsing error,
   /// and true otherwise.  This should be called in order, since the dumper
   /// maintains state about previous records which are necessary for cross
   /// type references.
-  bool dump(const CVRecord<TypeLeafKind> &Record);
+  Error dump(const CVRecord<TypeLeafKind> &Record);
 
   /// Dumps the type records in Types. Returns false if there was a type stream
   /// parse error, and true otherwise.
-  bool dump(const CVTypeArray &Types);
+  Error dump(const CVTypeArray &Types);
 
   /// Dumps the type records in Data. Returns false if there was a type stream
   /// parse error, and true otherwise. Use this method instead of the
   /// CVTypeArray overload when type records are laid out contiguously in
   /// memory.
-  bool dump(ArrayRef<uint8_t> Data);
+  Error dump(ArrayRef<uint8_t> Data);
 
   /// Gets the type index for the next type record.
   unsigned getNextTypeIndex() const {
     return 0x1000 + CVUDTNames.size();
   }
 
   /// Records the name of a type, and reserves its type index.
   void recordType(StringRef Name) { CVUDTNames.push_back(Name); }
 
   /// Saves the name in a StringSet and creates a stable StringRef.
   StringRef saveName(StringRef TypeName) {
     return TypeNames.insert(TypeName).first->getKey();
   }
 
   void setPrinter(ScopedPrinter *P);
   ScopedPrinter *getPrinter() { return W; }
 
+  /// Action to take on unknown types. By default, they are ignored.
+  Error visitUnknownType(TypeLeafKind Leaf,
+                         ArrayRef<uint8_t> RecordData) override;
+  Error visitUnknownMember(TypeLeafKind Leaf) override;
+
+  /// Paired begin/end actions for all types. Receives all record data,
+  /// including the fixed-length record prefix.
+  Error visitTypeBegin(TypeLeafKind Leaf,
+                       ArrayRef<uint8_t> RecordData) override;
+  Error visitTypeEnd(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData) override;
+
+#define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
+  Error visit##Name(TypeLeafKind LeafType, Name##Record &Record) override;
+#define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
+  TYPE_RECORD(EnumName, EnumVal, Name)
+#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
+#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
+#include "TypeRecords.def"
+
 private:
+  void printMemberAttributes(MemberAttributes Attrs);
+  void printMemberAttributes(MemberAccess Access, MethodKind Kind,
+                             MethodOptions Options);
+
   ScopedPrinter *W;
 
   bool PrintRecordBytes = false;
 
+  /// Name of the current type. Only valid before visitTypeEnd.
+  StringRef Name;
+
   /// All user defined type records in .debug$T live in here. Type indices
   /// greater than 0x1000 are user defined. Subtract 0x1000 from the index to
   /// index into this vector.
   SmallVector<StringRef, 10> CVUDTNames;
 
   StringSet<> TypeNames;
 };
 
 } // end namespace codeview
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_CODEVIEW_TYPEDUMPER_H

include/llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h

+//===- TypeVisitorCallbacks.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_TYPEVISITORCALLBACKS_H
+#define LLVM_DEBUGINFO_CODEVIEW_TYPEVISITORCALLBACKS_H
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/DebugInfo/CodeView/CodeView.h"
+#include "llvm/Support/Error.h"
+
+namespace llvm {
+namespace codeview {
+class TypeVisitorCallbacks {
+  friend class CVTypeVisitor;
+
+public:
+  virtual ~TypeVisitorCallbacks() {}
+
+  /// Action to take on unknown types. By default, they are ignored.
+  virtual Error visitUnknownType(TypeLeafKind Leaf,
+                                 ArrayRef<uint8_t> RecordData) {
+    return Error::success();
+  }
+  virtual Error visitUnknownMember(TypeLeafKind Leaf) {
+    return Error::success();
+  }
+
+  /// Paired begin/end actions for all types. Receives all record data,
+  /// including the fixed-length record prefix.
+  virtual Error visitTypeBegin(TypeLeafKind Leaf,
+                               ArrayRef<uint8_t> RecordData) {
+    return Error::success();
+  }
+  virtual Error visitTypeEnd(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData) {
+    return Error::success();
+  }
+
+#define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
+  virtual Error visit##Name(TypeLeafKind LeafType, Name##Record &Record) {     \
+    return Error::success();                                                   \
+  }
+#define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
+  TYPE_RECORD(EnumName, EnumVal, Name)
+#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
+#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
+#include "TypeRecords.def"
+};
+}
+}
+
+#endif
 No newline at end of file

lib/DebugInfo/CodeView/CMakeLists.txt

 add_llvm_library(LLVMDebugInfoCodeView
   ByteStream.cpp
   CodeViewError.cpp
+  CVTypeVisitor.cpp
   EnumTables.cpp
   FieldListRecordBuilder.cpp
   Line.cpp
   ListRecordBuilder.cpp
   MemoryTypeTableBuilder.cpp
   MethodListRecordBuilder.cpp
   ModuleSubstream.cpp
   ModuleSubstreamVisitor.cpp

lib/DebugInfo/CodeView/CVTypeVisitor.cpp

+//===- CVTypeVisitor.cpp ----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
+
+using namespace llvm;
+using namespace llvm::codeview;
+
+template <typename T>
+static Error takeObject(ArrayRef<uint8_t> &Data, const T *&Res) {
+  if (Data.size() < sizeof(*Res))
+    return llvm::make_error<CodeViewError>(cv_error_code::insufficient_buffer);
+  Res = reinterpret_cast<const T *>(Data.data());
+  Data = Data.drop_front(sizeof(*Res));
+  return Error::success();
+}
+
+CVTypeVisitor::CVTypeVisitor(TypeVisitorCallbacks &Callbacks)
+    : Callbacks(Callbacks) {}
+
+Error CVTypeVisitor::visitTypeRecord(const CVRecord<TypeLeafKind> &Record) {
+  ArrayRef<uint8_t> LeafData = Record.Data;
+  ArrayRef<uint8_t> RecordData = LeafData;
+  if (auto EC = Callbacks.visitTypeBegin(Record.Type, RecordData))
+    return EC;
+  switch (Record.Type) {
+  default:
+    if (auto EC = Callbacks.visitUnknownType(Record.Type, RecordData))
+      return EC;
+    break;
+  case LF_FIELDLIST:
+    if (auto EC = visitFieldList(Record.Type, LeafData))
+      return EC;
+    break;
+#define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
+  case EnumName: {                                                             \
+    TypeRecordKind RK = static_cast<TypeRecordKind>(EnumName);                 \
+    auto Result = Name##Record::deserialize(RK, LeafData);                     \
+    if (Result.getError())                                                     \
+      return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);   \
+    if (auto EC = Callbacks.visit##Name(Record.Type, *Result))                 \
+      return EC;                                                               \
+    break;                                                                     \
+  }
+#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)                  \
+  TYPE_RECORD(EnumVal, EnumVal, AliasName)
+#define MEMBER_RECORD(EnumName, EnumVal, Name)
+#include "llvm/DebugInfo/CodeView/TypeRecords.def"
+  }
+  if (auto EC = Callbacks.visitTypeEnd(Record.Type, RecordData))
+    return EC;
+  return Error::success();
+}
+
+/// Visits the type records in Data. Sets the error flag on parse failures.
+Error CVTypeVisitor::visitTypeStream(const CVTypeArray &Types) {
+  for (const auto &I : Types) {
+    if (auto EC = visitTypeRecord(I))
+      return EC;
+  }
+  return Error::success();
+}
+
+Error CVTypeVisitor::skipPadding(ArrayRef<uint8_t> &Data) {
+  if (Data.empty())
+    return Error::success();
+  uint8_t Leaf = Data.front();
+  if (Leaf < LF_PAD0)
+    return Error::success();
+  // Leaf is greater than 0xf0. We should advance by the number of bytes in
+  // the low 4 bits.
+  unsigned BytesToAdvance = Leaf & 0x0F;
+  if (Data.size() < BytesToAdvance) {
+    return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record,
+                                           "Invalid padding bytes!");
+  }
+  Data = Data.drop_front(BytesToAdvance);
+  return Error::success();
+}
+
+/// Visits individual member records of a field list record. Member records do
+/// not describe their own length, and need special handling.
+Error CVTypeVisitor::visitFieldList(TypeLeafKind Leaf,
+                                    ArrayRef<uint8_t> FieldData) {
+  while (!FieldData.empty()) {
+    const ulittle16_t *LeafPtr;
+    if (auto EC = takeObject(FieldData, LeafPtr))
+      return EC;
+    TypeLeafKind Leaf = TypeLeafKind(unsigned(*LeafPtr));
+    switch (Leaf) {
+    default:
+      // Field list records do not describe their own length, so we cannot
+      // continue parsing past an unknown member type.
+      if (auto EC = Callbacks.visitUnknownMember(Leaf))
+        return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
+#define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
+  case EnumName: {                                                             \
+    TypeRecordKind RK = static_cast<TypeRecordKind>(EnumName);                 \
+    auto Result = Name##Record::deserialize(RK, FieldData);                    \
+    if (Result.getError())                                                     \
+      return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);   \
+    if (auto EC = Callbacks.visit##Name(Leaf, *Result))                        \
+      return EC;                                                               \
+    break;                                                                     \
+  }
+#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)                \
+  MEMBER_RECORD(EnumVal, EnumVal, AliasName)
+#include "llvm/DebugInfo/CodeView/TypeRecords.def"
+    }
+    if (auto EC = skipPadding(FieldData))
+      return EC;
+  }
+  return Error::success();
+}

lib/DebugInfo/CodeView/TypeDumper.cpp

 static const EnumEntry<uint8_t> FunctionOptionEnum[] = {
     ENUM_ENTRY(FunctionOptions, CxxReturnUdt),
     ENUM_ENTRY(FunctionOptions, Constructor),
     ENUM_ENTRY(FunctionOptions, ConstructorWithVirtualBases),
 };
 
 #undef ENUM_ENTRY
 
-
-namespace {
-
-/// Use this private dumper implementation to keep implementation details about
-/// the visitor out of TypeDumper.h.
-class CVTypeDumperImpl : public CVTypeVisitor<CVTypeDumperImpl> {
-public:
-  CVTypeDumperImpl(CVTypeDumper &CVTD, ScopedPrinter &W, bool PrintRecordBytes)
-      : CVTD(CVTD), W(W), PrintRecordBytes(PrintRecordBytes) {}
-
-  /// CVTypeVisitor overrides.
-#define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
-  void visit##Name(TypeLeafKind LeafType, Name##Record &Record);
-#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
-#define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
-  void visit##Name(TypeLeafKind LeafType, Name##Record &Record);
-#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
-#include "llvm/DebugInfo/CodeView/TypeRecords.def"
-
-  void visitUnknownMember(TypeLeafKind Leaf);
-  void visitUnknownType(TypeLeafKind Leaf, ArrayRef<uint8_t> LeafData);
-
-  void visitTypeBegin(TypeLeafKind Leaf, ArrayRef<uint8_t> LeafData);
-  void visitTypeEnd(TypeLeafKind Leaf, ArrayRef<uint8_t> LeafData);
-
-  void printMemberAttributes(MemberAttributes Attrs);
-  void printMemberAttributes(MemberAccess Access, MethodKind Kind,
-                             MethodOptions Options);
-
-private:
-  /// Forwards to the dumper, which holds the persistent state from visitation.
-  StringRef getTypeName(TypeIndex TI) {
-    return CVTD.getTypeName(TI);
-  }
-
-  void printTypeIndex(StringRef FieldName, TypeIndex TI) {
-    CVTD.printTypeIndex(FieldName, TI);
-  }
-
-  CVTypeDumper &CVTD;
-  ScopedPrinter &W;
-  bool PrintRecordBytes = false;
-
-  /// Name of the current type. Only valid before visitTypeEnd.
-  StringRef Name;
-};
-
-} // end anonymous namespace
-
 static StringRef getLeafTypeName(TypeLeafKind LT) {
   switch (LT) {
 #define TYPE_RECORD(ename, value, name)                                        \
   case ename:                                                                  \
     return #name;
 #include "llvm/DebugInfo/CodeView/TypeRecords.def"
   default:
     break;
   }
   return "UnknownLeaf";
 }
 
-void CVTypeDumperImpl::visitTypeBegin(TypeLeafKind Leaf,
-                                      ArrayRef<uint8_t> LeafData) {
+Error CVTypeDumper::visitTypeBegin(TypeLeafKind Leaf,
   // Reset Name to the empty string. If the visitor sets it, we know it.
   Name = "";
 
-  W.startLine() << getLeafTypeName(Leaf) << " ("
-                << HexNumber(CVTD.getNextTypeIndex()) << ") {\n";
-  W.indent();
-  W.printEnum("TypeLeafKind", unsigned(Leaf), makeArrayRef(LeafTypeNames));
+  W->startLine() << getLeafTypeName(Leaf) << " ("
+                 << HexNumber(getNextTypeIndex()) << ") {\n";
+  W->indent();
+  W->printEnum("TypeLeafKind", unsigned(Leaf), makeArrayRef(LeafTypeNames));
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitTypeEnd(TypeLeafKind Leaf,
-                                    ArrayRef<uint8_t> LeafData) {
+Error CVTypeDumper::visitTypeEnd(TypeLeafKind Leaf,
   // Always record some name for every type, even if Name is empty. CVUDTNames
   // is indexed by type index, and must have one entry for every type.
-  CVTD.recordType(Name);
+  recordType(Name);
 
   if (PrintRecordBytes)
-    W.printBinaryBlock("LeafData", getBytesAsCharacters(LeafData));
+    W->printBinaryBlock("LeafData", getBytesAsCharacters(LeafData));
 
-  W.unindent();
-  W.startLine() << "}\n";
+  W->unindent();
+  W->startLine() << "}\n";
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitStringId(TypeLeafKind Leaf,
-                                           StringIdRecord &String) {
+Error CVTypeDumper::visitStringId(TypeLeafKind Leaf, StringIdRecord &String) {
   printTypeIndex("Id", String.getId());
-  W.printString("StringData", String.getString());
+  W->printString("StringData", String.getString());
   // Put this in CVUDTNames so it gets printed with LF_UDT_SRC_LINE.
   Name = String.getString();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitArgList(TypeLeafKind Leaf, ArgListRecord &Args) {
+Error CVTypeDumper::visitArgList(TypeLeafKind Leaf, ArgListRecord &Args) {
   auto Indices = Args.getIndices();
   uint32_t Size = Indices.size();
-  W.printNumber("NumArgs", Size);
-  ListScope Arguments(W, "Arguments");
+  W->printNumber("NumArgs", Size);
+  ListScope Arguments(*W, "Arguments");
   SmallString<256> TypeName("(");
   for (uint32_t I = 0; I < Size; ++I) {
     printTypeIndex("ArgType", Indices[I]);
     StringRef ArgTypeName = getTypeName(Indices[I]);
     TypeName.append(ArgTypeName);
     if (I + 1 != Size)
       TypeName.append(", ");
   }
   TypeName.push_back(')');
-  Name = CVTD.saveName(TypeName);
+  Name = saveName(TypeName);
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitClass(TypeLeafKind Leaf, ClassRecord &Class) {
+Error CVTypeDumper::visitClass(TypeLeafKind Leaf, ClassRecord &Class) {
   uint16_t Props = static_cast<uint16_t>(Class.getOptions());
-  W.printNumber("MemberCount", Class.getMemberCount());
-  W.printFlags("Properties", Props, makeArrayRef(ClassOptionNames));
+  W->printNumber("MemberCount", Class.getMemberCount());
+  W->printFlags("Properties", Props, makeArrayRef(ClassOptionNames));
   printTypeIndex("FieldList", Class.getFieldList());
   printTypeIndex("DerivedFrom", Class.getDerivationList());
   printTypeIndex("VShape", Class.getVTableShape());
-  W.printNumber("SizeOf", Class.getSize());
-  W.printString("Name", Class.getName());
+  W->printNumber("SizeOf", Class.getSize());
+  W->printString("Name", Class.getName());
   if (Props & uint16_t(ClassOptions::HasUniqueName))
-    W.printString("LinkageName", Class.getUniqueName());
+    W->printString("LinkageName", Class.getUniqueName());
   Name = Class.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitUnion(TypeLeafKind Leaf, UnionRecord &Union) {
+Error CVTypeDumper::visitUnion(TypeLeafKind Leaf, UnionRecord &Union) {
   uint16_t Props = static_cast<uint16_t>(Union.getOptions());
-  W.printNumber("MemberCount", Union.getMemberCount());
-  W.printFlags("Properties", Props, makeArrayRef(ClassOptionNames));
+  W->printNumber("MemberCount", Union.getMemberCount());
+  W->printFlags("Properties", Props, makeArrayRef(ClassOptionNames));
   printTypeIndex("FieldList", Union.getFieldList());
-  W.printNumber("SizeOf", Union.getSize());
-  W.printString("Name", Union.getName());
+  W->printNumber("SizeOf", Union.getSize());
+  W->printString("Name", Union.getName());
   if (Props & uint16_t(ClassOptions::HasUniqueName))
-    W.printString("LinkageName", Union.getUniqueName());
+    W->printString("LinkageName", Union.getUniqueName());
   Name = Union.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitEnum(TypeLeafKind Leaf, EnumRecord &Enum) {
-  W.printNumber("NumEnumerators", Enum.getMemberCount());
-  W.printFlags("Properties", uint16_t(Enum.getOptions()),
+Error CVTypeDumper::visitEnum(TypeLeafKind Leaf, EnumRecord &Enum) {
+  W->printNumber("NumEnumerators", Enum.getMemberCount());
+  W->printFlags("Properties", uint16_t(Enum.getOptions()),
                 makeArrayRef(ClassOptionNames));
   printTypeIndex("UnderlyingType", Enum.getUnderlyingType());
   printTypeIndex("FieldListType", Enum.getFieldList());
-  W.printString("Name", Enum.getName());
+  W->printString("Name", Enum.getName());
   Name = Enum.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitArray(TypeLeafKind Leaf, ArrayRecord &AT) {
+Error CVTypeDumper::visitArray(TypeLeafKind Leaf, ArrayRecord &AT) {
   printTypeIndex("ElementType", AT.getElementType());
   printTypeIndex("IndexType", AT.getIndexType());
-  W.printNumber("SizeOf", AT.getSize());
-  W.printString("Name", AT.getName());
+  W->printNumber("SizeOf", AT.getSize());
+  W->printString("Name", AT.getName());
   Name = AT.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitVFTable(TypeLeafKind Leaf, VFTableRecord &VFT) {
+Error CVTypeDumper::visitVFTable(TypeLeafKind Leaf, VFTableRecord &VFT) {
   printTypeIndex("CompleteClass", VFT.getCompleteClass());
   printTypeIndex("OverriddenVFTable", VFT.getOverriddenVTable());
-  W.printHex("VFPtrOffset", VFT.getVFPtrOffset());
-  W.printString("VFTableName", VFT.getName());
+  W->printHex("VFPtrOffset", VFT.getVFPtrOffset());
+  W->printString("VFTableName", VFT.getName());
   for (auto N : VFT.getMethodNames())
-    W.printString("MethodName", N);
+    W->printString("MethodName", N);
   Name = VFT.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitMemberFuncId(TypeLeafKind Leaf,
-                                               MemberFuncIdRecord &Id) {
+Error CVTypeDumper::visitMemberFuncId(TypeLeafKind Leaf,
   printTypeIndex("ClassType", Id.getClassType());
   printTypeIndex("FunctionType", Id.getFunctionType());
-  W.printString("Name", Id.getName());
+  W->printString("Name", Id.getName());
   Name = Id.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitProcedure(TypeLeafKind Leaf,
-                                      ProcedureRecord &Proc) {
+Error CVTypeDumper::visitProcedure(TypeLeafKind Leaf, ProcedureRecord &Proc) {
   printTypeIndex("ReturnType", Proc.getReturnType());
-  W.printEnum("CallingConvention", uint8_t(Proc.getCallConv()),
+  W->printEnum("CallingConvention", uint8_t(Proc.getCallConv()),
                makeArrayRef(CallingConventions));
-  W.printFlags("FunctionOptions", uint8_t(Proc.getOptions()),
+  W->printFlags("FunctionOptions", uint8_t(Proc.getOptions()),
                 makeArrayRef(FunctionOptionEnum));
-  W.printNumber("NumParameters", Proc.getParameterCount());
+  W->printNumber("NumParameters", Proc.getParameterCount());
   printTypeIndex("ArgListType", Proc.getArgumentList());
 
   StringRef ReturnTypeName = getTypeName(Proc.getReturnType());
   StringRef ArgListTypeName = getTypeName(Proc.getArgumentList());
   SmallString<256> TypeName(ReturnTypeName);
   TypeName.push_back(' ');
   TypeName.append(ArgListTypeName);
-  Name = CVTD.saveName(TypeName);
+  Name = saveName(TypeName);
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitMemberFunction(TypeLeafKind Leaf,
-                                           MemberFunctionRecord &MF) {
+Error CVTypeDumper::visitMemberFunction(TypeLeafKind Leaf,
   printTypeIndex("ReturnType", MF.getReturnType());
   printTypeIndex("ClassType", MF.getClassType());
   printTypeIndex("ThisType", MF.getThisType());
-  W.printEnum("CallingConvention", uint8_t(MF.getCallConv()),
+  W->printEnum("CallingConvention", uint8_t(MF.getCallConv()),
                makeArrayRef(CallingConventions));
-  W.printFlags("FunctionOptions", uint8_t(MF.getOptions()),
+  W->printFlags("FunctionOptions", uint8_t(MF.getOptions()),
                 makeArrayRef(FunctionOptionEnum));
-  W.printNumber("NumParameters", MF.getParameterCount());
+  W->printNumber("NumParameters", MF.getParameterCount());
   printTypeIndex("ArgListType", MF.getArgumentList());
-  W.printNumber("ThisAdjustment", MF.getThisPointerAdjustment());
+  W->printNumber("ThisAdjustment", MF.getThisPointerAdjustment());
 
   StringRef ReturnTypeName = getTypeName(MF.getReturnType());
   StringRef ClassTypeName = getTypeName(MF.getClassType());
   StringRef ArgListTypeName = getTypeName(MF.getArgumentList());
   SmallString<256> TypeName(ReturnTypeName);
   TypeName.push_back(' ');
   TypeName.append(ClassTypeName);
   TypeName.append("::");
   TypeName.append(ArgListTypeName);
-  Name = CVTD.saveName(TypeName);
+  Name = saveName(TypeName);
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitMethodOverloadList(
+Error CVTypeDumper::visitMethodOverloadList(
     TypeLeafKind Leaf, MethodOverloadListRecord &MethodList) {
   for (auto &M : MethodList.getMethods()) {
-    ListScope S(W, "Method");
+    ListScope S(*W, "Method");
     printMemberAttributes(M.getAccess(), M.getKind(), M.getOptions());
     printTypeIndex("Type", M.getType());
     if (M.isIntroducingVirtual())
-      W.printHex("VFTableOffset", M.getVFTableOffset());
+      W->printHex("VFTableOffset", M.getVFTableOffset());
   }
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitFuncId(TypeLeafKind Leaf, FuncIdRecord &Func) {
+Error CVTypeDumper::visitFuncId(TypeLeafKind Leaf, FuncIdRecord &Func) {
   printTypeIndex("ParentScope", Func.getParentScope());
   printTypeIndex("FunctionType", Func.getFunctionType());
-  W.printString("Name", Func.getName());
+  W->printString("Name", Func.getName());
   Name = Func.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitTypeServer2(TypeLeafKind Leaf,
-                                        TypeServer2Record &TS) {
-  W.printBinary("Signature", TS.getGuid());
-  W.printNumber("Age", TS.getAge());
-  W.printString("Name", TS.getName());
+Error CVTypeDumper::visitTypeServer2(TypeLeafKind Leaf, TypeServer2Record &TS) {
+  W->printBinary("Signature", TS.getGuid());
+  W->printNumber("Age", TS.getAge());
+  W->printString("Name", TS.getName());
   Name = TS.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitPointer(TypeLeafKind Leaf, PointerRecord &Ptr) {
+Error CVTypeDumper::visitPointer(TypeLeafKind Leaf, PointerRecord &Ptr) {
   printTypeIndex("PointeeType", Ptr.getReferentType());
-  W.printHex("PointerAttributes", uint32_t(Ptr.getOptions()));
-  W.printEnum("PtrType", unsigned(Ptr.getPointerKind()),
+  W->printHex("PointerAttributes", uint32_t(Ptr.getOptions()));
+  W->printEnum("PtrType", unsigned(Ptr.getPointerKind()),
                makeArrayRef(PtrKindNames));
-  W.printEnum("PtrMode", unsigned(Ptr.getMode()), makeArrayRef(PtrModeNames));
+  W->printEnum("PtrMode", unsigned(Ptr.getMode()), makeArrayRef(PtrModeNames));
 
-  W.printNumber("IsFlat", Ptr.isFlat());
-  W.printNumber("IsConst", Ptr.isConst());
-  W.printNumber("IsVolatile", Ptr.isVolatile());
-  W.printNumber("IsUnaligned", Ptr.isUnaligned());
+  W->printNumber("IsFlat", Ptr.isFlat());
+  W->printNumber("IsConst", Ptr.isConst());
+  W->printNumber("IsVolatile", Ptr.isVolatile());
+  W->printNumber("IsUnaligned", Ptr.isUnaligned());
 
   if (Ptr.isPointerToMember()) {
     const MemberPointerInfo &MI = Ptr.getMemberInfo();
 
     printTypeIndex("ClassType", MI.getContainingType());
-    W.printEnum("Representation", uint16_t(MI.getRepresentation()),
+    W->printEnum("Representation", uint16_t(MI.getRepresentation()),
                  makeArrayRef(PtrMemberRepNames));
 
     StringRef PointeeName = getTypeName(Ptr.getReferentType());
     StringRef ClassName = getTypeName(MI.getContainingType());
     SmallString<256> TypeName(PointeeName);
     TypeName.push_back(' ');
     TypeName.append(ClassName);
     TypeName.append("::*");
-    Name = CVTD.saveName(TypeName);
+    Name = saveName(TypeName);
   } else {
     SmallString<256> TypeName;
     if (Ptr.isConst())
       TypeName.append("const ");
     if (Ptr.isVolatile())
       TypeName.append("volatile ");
     if (Ptr.isUnaligned())
       TypeName.append("__unaligned ");
 
     TypeName.append(getTypeName(Ptr.getReferentType()));
 
     if (Ptr.getMode() == PointerMode::LValueReference)
       TypeName.append("&");
     else if (Ptr.getMode() == PointerMode::RValueReference)
       TypeName.append("&&");
     else if (Ptr.getMode() == PointerMode::Pointer)
       TypeName.append("*");
 
     if (!TypeName.empty())
-      Name = CVTD.saveName(TypeName);
+      Name = saveName(TypeName);
   }
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitModifier(TypeLeafKind Leaf, ModifierRecord &Mod) {
+Error CVTypeDumper::visitModifier(TypeLeafKind Leaf, ModifierRecord &Mod) {
   uint16_t Mods = static_cast<uint16_t>(Mod.getModifiers());
   printTypeIndex("ModifiedType", Mod.getModifiedType());
-  W.printFlags("Modifiers", Mods, makeArrayRef(TypeModifierNames));
+  W->printFlags("Modifiers", Mods, makeArrayRef(TypeModifierNames));
 
   StringRef ModifiedName = getTypeName(Mod.getModifiedType());
   SmallString<256> TypeName;
   if (Mods & uint16_t(ModifierOptions::Const))
     TypeName.append("const ");
   if (Mods & uint16_t(ModifierOptions::Volatile))
     TypeName.append("volatile ");
   if (Mods & uint16_t(ModifierOptions::Unaligned))
     TypeName.append("__unaligned ");
   TypeName.append(ModifiedName);
-  Name = CVTD.saveName(TypeName);
+  Name = saveName(TypeName);
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitBitField(TypeLeafKind Leaf,
-                                     BitFieldRecord &BitField) {
+Error CVTypeDumper::visitBitField(TypeLeafKind Leaf, BitFieldRecord &BitField) {
   printTypeIndex("Type", BitField.getType());
-  W.printNumber("BitSize", BitField.getBitSize());
-  W.printNumber("BitOffset", BitField.getBitOffset());
+  W->printNumber("BitSize", BitField.getBitSize());
+  W->printNumber("BitOffset", BitField.getBitOffset());
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitVFTableShape(TypeLeafKind Leaf,
-                                         VFTableShapeRecord &Shape) {
-  W.printNumber("VFEntryCount", Shape.getEntryCount());
+Error CVTypeDumper::visitVFTableShape(TypeLeafKind Leaf,
+  W->printNumber("VFEntryCount", Shape.getEntryCount());
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitUdtSourceLine(TypeLeafKind Leaf,
-                                          UdtSourceLineRecord &Line) {
+Error CVTypeDumper::visitUdtSourceLine(TypeLeafKind Leaf,
   printTypeIndex("UDT", Line.getUDT());
   printTypeIndex("SourceFile", Line.getSourceFile());
-  W.printNumber("LineNumber", Line.getLineNumber());
+  W->printNumber("LineNumber", Line.getLineNumber());
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitBuildInfo(TypeLeafKind Leaf,
-                                      BuildInfoRecord &Args) {
-  W.printNumber("NumArgs", static_cast<uint32_t>(Args.getArgs().size()));
+Error CVTypeDumper::visitBuildInfo(TypeLeafKind Leaf, BuildInfoRecord &Args) {
+  W->printNumber("NumArgs", static_cast<uint32_t>(Args.getArgs().size()));
 
-  ListScope Arguments(W, "Arguments");
+  ListScope Arguments(*W, "Arguments");
   for (auto Arg : Args.getArgs()) {
     printTypeIndex("ArgType", Arg);
   }
+  return Error::success();
 }
 
-void CVTypeDumperImpl::printMemberAttributes(MemberAttributes Attrs) {
+void CVTypeDumper::printMemberAttributes(MemberAttributes Attrs) {
   return printMemberAttributes(Attrs.getAccess(), Attrs.getMethodKind(),
                                Attrs.getFlags());
 }
 
-void CVTypeDumperImpl::printMemberAttributes(MemberAccess Access,
-                                             MethodKind Kind,
-                                             MethodOptions Options) {
-  W.printEnum("AccessSpecifier", uint8_t(Access),
+void CVTypeDumper::printMemberAttributes(MemberAccess Access, MethodKind Kind,
+  W->printEnum("AccessSpecifier", uint8_t(Access),
                makeArrayRef(MemberAccessNames));
   // Data members will be vanilla. Don't try to print a method kind for them.
   if (Kind != MethodKind::Vanilla)
-    W.printEnum("MethodKind", unsigned(Kind), makeArrayRef(MemberKindNames));
+    W->printEnum("MethodKind", unsigned(Kind), makeArrayRef(MemberKindNames));
   if (Options != MethodOptions::None) {
-    W.printFlags("MethodOptions", unsigned(Options),
+    W->printFlags("MethodOptions", unsigned(Options),
                   makeArrayRef(MethodOptionNames));
   }
 }
 
-void CVTypeDumperImpl::visitUnknownMember(TypeLeafKind Leaf) {
-  W.printHex("UnknownMember", unsigned(Leaf));
+Error CVTypeDumper::visitUnknownMember(TypeLeafKind Leaf) {
+  W->printHex("UnknownMember", unsigned(Leaf));
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitUnknownType(TypeLeafKind Leaf,
-                                        ArrayRef<uint8_t> RecordData) {
-  DictScope S(W, "UnknownType");
-  W.printEnum("Kind", uint16_t(Leaf), makeArrayRef(LeafTypeNames));
-  W.printNumber("Length", uint32_t(RecordData.size()));
+Error CVTypeDumper::visitUnknownType(TypeLeafKind Leaf,
+  DictScope S(*W, "UnknownType");
+  W->printEnum("Kind", uint16_t(Leaf), makeArrayRef(LeafTypeNames));
+  W->printNumber("Length", uint32_t(RecordData.size()));
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitNestedType(TypeLeafKind Leaf,
-                                       NestedTypeRecord &Nested) {
-  DictScope S(W, "NestedType");
+Error CVTypeDumper::visitNestedType(TypeLeafKind Leaf,
+  DictScope S(*W, "NestedType");
   printTypeIndex("Type", Nested.getNestedType());
-  W.printString("Name", Nested.getName());
+  W->printString("Name", Nested.getName());
   Name = Nested.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitOneMethod(TypeLeafKind Leaf,
-                                      OneMethodRecord &Method) {
-  DictScope S(W, "OneMethod");
+Error CVTypeDumper::visitOneMethod(TypeLeafKind Leaf, OneMethodRecord &Method) {
+  DictScope S(*W, "OneMethod");
   MethodKind K = Method.getKind();
   printMemberAttributes(Method.getAccess(), K, Method.getOptions());
   printTypeIndex("Type", Method.getType());
   // If virtual, then read the vftable offset.
   if (Method.isIntroducingVirtual())
-    W.printHex("VFTableOffset", Method.getVFTableOffset());
-  W.printString("Name", Method.getName());
+    W->printHex("VFTableOffset", Method.getVFTableOffset());
+  W->printString("Name", Method.getName());
   Name = Method.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitOverloadedMethod(TypeLeafKind Leaf,
-                                             OverloadedMethodRecord &Method) {
-  DictScope S(W, "OverloadedMethod");
-  W.printHex("MethodCount", Method.getNumOverloads());
+Error CVTypeDumper::visitOverloadedMethod(TypeLeafKind Leaf,
+  DictScope S(*W, "OverloadedMethod");
+  W->printHex("MethodCount", Method.getNumOverloads());
   printTypeIndex("MethodListIndex", Method.getMethodList());
-  W.printString("Name", Method.getName());
+  W->printString("Name", Method.getName());
   Name = Method.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitDataMember(TypeLeafKind Leaf,
-                                       DataMemberRecord &Field) {
-  DictScope S(W, "DataMember");
+Error CVTypeDumper::visitDataMember(TypeLeafKind Leaf,
+  DictScope S(*W, "DataMember");
   printMemberAttributes(Field.getAccess(), MethodKind::Vanilla,
                         MethodOptions::None);
   printTypeIndex("Type", Field.getType());
-  W.printHex("FieldOffset", Field.getFieldOffset());
-  W.printString("Name", Field.getName());
+  W->printHex("FieldOffset", Field.getFieldOffset());
+  W->printString("Name", Field.getName());
   Name = Field.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitStaticDataMember(TypeLeafKind Leaf,
-                                             StaticDataMemberRecord &Field) {
-  DictScope S(W, "StaticDataMember");
+Error CVTypeDumper::visitStaticDataMember(TypeLeafKind Leaf,
+  DictScope S(*W, "StaticDataMember");
   printMemberAttributes(Field.getAccess(), MethodKind::Vanilla,
                         MethodOptions::None);
   printTypeIndex("Type", Field.getType());
-  W.printString("Name", Field.getName());
+  W->printString("Name", Field.getName());
   Name = Field.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitVFPtr(TypeLeafKind Leaf, VFPtrRecord &VFTable) {
-  DictScope S(W, "VFPtr");
+Error CVTypeDumper::visitVFPtr(TypeLeafKind Leaf, VFPtrRecord &VFTable) {
+  DictScope S(*W, "VFPtr");
   printTypeIndex("Type", VFTable.getType());
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitEnumerator(TypeLeafKind Leaf,
-                                       EnumeratorRecord &Enum) {
-  DictScope S(W, "Enumerator");
+Error CVTypeDumper::visitEnumerator(TypeLeafKind Leaf, EnumeratorRecord &Enum) {
+  DictScope S(*W, "Enumerator");
   printMemberAttributes(Enum.getAccess(), MethodKind::Vanilla,
                         MethodOptions::None);
-  W.printNumber("EnumValue", Enum.getValue());
-  W.printString("Name", Enum.getName());
+  W->printNumber("EnumValue", Enum.getValue());
+  W->printString("Name", Enum.getName());
   Name = Enum.getName();
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitBaseClass(TypeLeafKind Leaf,
-                                      BaseClassRecord &Base) {
-  DictScope S(W, "BaseClass");
+Error CVTypeDumper::visitBaseClass(TypeLeafKind Leaf, BaseClassRecord &Base) {
+  DictScope S(*W, "BaseClass");
   printMemberAttributes(Base.getAccess(), MethodKind::Vanilla,
                         MethodOptions::None);
   printTypeIndex("BaseType", Base.getBaseType());
-  W.printHex("BaseOffset", Base.getBaseOffset());
+  W->printHex("BaseOffset", Base.getBaseOffset());
+  return Error::success();
 }
 
-void CVTypeDumperImpl::visitVirtualBaseClass(TypeLeafKind Leaf,
-                                             VirtualBaseClassRecord &Base) {
-  DictScope S(W, "VirtualBaseClass");
+Error CVTypeDumper::visitVirtualBaseClass(TypeLeafKind Leaf,
+  DictScope S(*W, "VirtualBaseClass");
   printMemberAttributes(Base.getAccess(), MethodKind::Vanilla,
                         MethodOptions::None);
   printTypeIndex("BaseType", Base.getBaseType());
   printTypeIndex("VBPtrType", Base.getVBPtrType());
-  W.printHex("VBPtrOffset", Base.getVBPtrOffset());
-  W.printHex("VBTableIndex", Base.getVTableIndex());
+  W->printHex("VBPtrOffset", Base.getVBPtrOffset());
+  W->printHex("VBTableIndex", Base.getVTableIndex());
+  return Error::success();
 }
 
 StringRef CVTypeDumper::getTypeName(TypeIndex TI) {
   if (TI.isNoneType())
     return "<no type>";
 
   if (TI.isSimple()) {
     // This is a simple type.
void CVTypeDumper::printTypeIndex(StringRef FieldName, TypeIndex TI) {
   if (!TI.isNoneType())
     TypeName = getTypeName(TI);
   if (!TypeName.empty())
     W->printHex(FieldName, TypeName, TI.getIndex());
   else
     W->printHex(FieldName, TI.getIndex());
 }
 
-bool CVTypeDumper::dump(const CVRecord<TypeLeafKind> &Record) {
+Error CVTypeDumper::dump(const CVRecord<TypeLeafKind> &Record) {
   assert(W && "printer should not be null");
-  CVTypeDumperImpl Dumper(*this, *W, PrintRecordBytes);
-  Dumper.visitTypeRecord(Record);
-  return !Dumper.hadError();
+  CVTypeVisitor Visitor(*this);
+
+  if (auto EC = Visitor.visitTypeRecord(Record))
+    return EC;
+  return Error::success();
 }
 
-bool CVTypeDumper::dump(const CVTypeArray &Types) {
+Error CVTypeDumper::dump(const CVTypeArray &Types) {
   assert(W && "printer should not be null");
-  CVTypeDumperImpl Dumper(*this, *W, PrintRecordBytes);
-  Dumper.visitTypeStream(Types);
-  return !Dumper.hadError();
+  CVTypeVisitor Visitor(*this);
+  if (auto EC = Visitor.visitTypeStream(Types))
+    return EC;
+  return Error::success();
 }
 
-bool CVTypeDumper::dump(ArrayRef<uint8_t> Data) {
+Error CVTypeDumper::dump(ArrayRef<uint8_t> Data) {
   ByteStream<> Stream(Data);
   CVTypeArray Types;
   StreamReader Reader(Stream);
-  if (auto EC = Reader.readArray(Types, Reader.getLength())) {
-    consumeError(std::move(EC));
-    return false;
-  }
+  if (auto EC = Reader.readArray(Types, Reader.getLength()))
+    return EC;
 
   return dump(Types);
 }
 
 void CVTypeDumper::setPrinter(ScopedPrinter *P) {
   static ScopedPrinter NullP(llvm::nulls());
   W = P ? P : &NullP;
 }

lib/DebugInfo/CodeView/TypeStreamMerger.cpp

 #include "llvm/DebugInfo/CodeView/TypeStreamMerger.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
 #include "llvm/DebugInfo/CodeView/FieldListRecordBuilder.h"
 #include "llvm/DebugInfo/CodeView/StreamRef.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
+#include "llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 using namespace llvm;
 using namespace llvm::codeview;
 
 namespace {
 
 /// Implementation of CodeView type stream merging.

 /// - For each record, copy it and rewrite its type indices to be valid in the
 ///   destination type stream.
 /// - If the new type record is not already present in the destination stream
 ///   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 CVTypeVisitor<TypeStreamMerger> {
+class TypeStreamMerger : public TypeVisitorCallbacks {
 public:
   TypeStreamMerger(TypeTableBuilder &DestStream) : DestStream(DestStream) {
     assert(!hadError());
   }
 
-  /// CVTypeVisitor overrides.
+/// TypeVisitorCallbacks overrides.
 #define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
-  void visit##Name(TypeLeafKind LeafType, Name##Record &Record);
-#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
+  Error visit##Name(TypeLeafKind LeafType, Name##Record &Record) override;
 #define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
-  void visit##Name(TypeLeafKind LeafType, Name##Record &Record);
+  TYPE_RECORD(EnumName, EnumVal, Name)
+#define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #include "llvm/DebugInfo/CodeView/TypeRecords.def"
 
-  void visitUnknownType(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData);
-
-  void visitTypeBegin(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData);
-  void visitTypeEnd(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData);
+  Error visitUnknownType(TypeLeafKind Leaf,
+                         ArrayRef<uint8_t> RecordData) override;
 
-  void visitFieldList(TypeLeafKind Leaf, ArrayRef<uint8_t> FieldData);
+  Error visitTypeBegin(TypeLeafKind Leaf,
+                       ArrayRef<uint8_t> RecordData) override;
+  Error visitTypeEnd(TypeLeafKind Leaf, ArrayRef<uint8_t> RecordData) override;
 
   bool mergeStream(const CVTypeArray &Types);
 
 private:
-  bool hadError() { return FoundBadTypeIndex || CVTypeVisitor::hadError(); }
+  bool hadError() { return FoundBadTypeIndex; }
 
   bool FoundBadTypeIndex = false;
 
   FieldListRecordBuilder FieldBuilder;
 
   TypeTableBuilder &DestStream;
 
   size_t BeginIndexMapSize = 0;
 
   /// Map from source type index to destination type index. Indexed by source
   /// type index minus 0x1000.
   SmallVector<TypeIndex, 0> IndexMap;
 };
 
 } // end anonymous namespace
 
-void TypeStreamMerger::visitTypeBegin(TypeLeafKind Leaf,
+Error TypeStreamMerger::visitTypeBegin(TypeLeafKind Leaf,
                                        ArrayRef<uint8_t> RecordData) {
   BeginIndexMapSize = IndexMap.size();
+  return Error::success();
 }
 
-void TypeStreamMerger::visitTypeEnd(TypeLeafKind Leaf,
+Error TypeStreamMerger::visitTypeEnd(TypeLeafKind Leaf,
                                      ArrayRef<uint8_t> RecordData) {
-  assert(IndexMap.size() == BeginIndexMapSize + 1);
-}
-
-void TypeStreamMerger::visitFieldList(TypeLeafKind Leaf,
-                                      ArrayRef<uint8_t> FieldData) {
-  CVTypeVisitor::visitFieldList(Leaf, FieldData);
+  if (Leaf == TypeLeafKind::LF_FIELDLIST) {
     IndexMap.push_back(DestStream.writeFieldList(FieldBuilder));
     FieldBuilder.reset();
   }
 
+  assert(IndexMap.size() == BeginIndexMapSize + 1);
+  return Error::success();
+}
+
 #define TYPE_RECORD(EnumName, EnumVal, Name)                                   \
-  void TypeStreamMerger::visit##Name(TypeLeafKind LeafType,                    \
-                                     Name##Record &Record) {                   \
+  Error TypeStreamMerger::visit##Name(TypeLeafKind LeafType,                   \
+                                      Name##Record &Record) {                  \
     FoundBadTypeIndex |= !Record.remapTypeIndices(IndexMap);                   \
     IndexMap.push_back(DestStream.write##Name(Record));                        \
+    return Error::success();                                                   \
   }
 #define TYPE_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #define MEMBER_RECORD(EnumName, EnumVal, Name)                                 \
-  void TypeStreamMerger::visit##Name(TypeLeafKind LeafType,                    \
-                                     Name##Record &Record) {                   \
+  Error TypeStreamMerger::visit##Name(TypeLeafKind LeafType,                   \
+                                      Name##Record &Record) {                  \
     FoundBadTypeIndex |= !Record.remapTypeIndices(IndexMap);                   \
     FieldBuilder.write##Name(Record);                                          \
+    return Error::success();                                                   \
   }
 #define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
 #include "llvm/DebugInfo/CodeView/TypeRecords.def"
 
-void TypeStreamMerger::visitUnknownType(TypeLeafKind Leaf,
+Error TypeStreamMerger::visitUnknownType(TypeLeafKind Leaf,
                                          ArrayRef<uint8_t> RecordData) {
   // We failed to translate a type. Translate this index as "not translated".
   IndexMap.push_back(
       TypeIndex(SimpleTypeKind::NotTranslated, SimpleTypeMode::Direct));
-  parseError();
+  return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
 }
 
 bool TypeStreamMerger::mergeStream(const CVTypeArray &Types) {
   assert(IndexMap.empty());
-  visitTypeStream(Types);
+  CVTypeVisitor Visitor(*this);
+  if (auto EC = Visitor.visitTypeStream(Types)) {
+    consumeError(std::move(EC));
+    return false;
+  }
   IndexMap.clear();
   return !hadError();
 }
 
 bool llvm::codeview::mergeTypeStreams(TypeTableBuilder &DestStream,
                                       const CVTypeArray &Types) {
   return TypeStreamMerger(DestStream).mergeStream(Types);
 }

tools/llvm-pdbdump/LLVMOutputStyle.cpp

if (DumpRecords || DumpRecordBytes) {
     P.printNumber("Record count", Tpi->NumTypeRecords());
 
     ListScope L(P, "Records");
 
     bool HadError = false;
     for (auto &Type : Tpi->types(&HadError)) {
       DictScope DD(P, "");
 
-      if (DumpRecords)
-        TD.dump(Type);
+      if (DumpRecords) {
+        if (auto EC = TD.dump(Type))
+          return EC;
+      }
 
       if (DumpRecordBytes)
         P.printBinaryBlock("Bytes", Type.Data);
     }
     dumpTpiHash(P, *Tpi);
     if (HadError)
       return make_error<RawError>(raw_error_code::corrupt_file,
                                   "TPI stream contained corrupt record");
   } else if (opts::DumpModuleSyms) {
     // Even if the user doesn't want to dump type records, we still need to
     // iterate them in order to build the list of types so that we can print
     // them when dumping module symbols. So when they want to dump symbols
     // but not types, use a null output stream.
     ScopedPrinter *OldP = TD.getPrinter();
     TD.setPrinter(nullptr);
 
     bool HadError = false;
-    for (auto &Type : Tpi->types(&HadError))
-      TD.dump(Type);
+    for (auto &Type : Tpi->types(&HadError)) {
+      if (auto EC = TD.dump(Type))
+        return EC;
+    }
 
     TD.setPrinter(OldP);
     dumpTpiHash(P, *Tpi);
     if (HadError)
       return make_error<RawError>(raw_error_code::corrupt_file,
                                   "TPI stream contained corrupt record");
   }
   P.flush();