Move CodeViewTypeStream and iterators to DebugInfo/CodeView

[zturner]

Summary

These classes should be reusable outside of the CV dumper, so moving them into the library.

[zturner]

I also changed the iterator to work with ranged based for syntax as well.

[dblaikie]

include/llvm/DebugInfo/CodeView/TypeStream.h

34

static->inline as this code is in a header (otherwise you get ODR violations, code duplication, etc) - same goes for any other non-member static things here (consumeObject above)

123–129

'inline' now that these are in a header, otherwise you'll get duplicate symbols when two object files using that include this header are linked together.

Moving these definitions into their friend declarations would make them implicitly inline and they're quite small anyway, so that's probably not a bad idea.

131

I'm not sure about this type, with regard to our previous discussions on "where to put the magic" and how to fail the ctor.

If this type is just for range-for convenience, should probably just use llvm::iterator_range to put the two TypeIterators together.

But if we want to use it to make the magic access a bit more natural/remove the extra memory footprint from the iterators, then I'd expect to see the knowledge of magic removed from the iterator and more of the initialization work to be done in TypeStream's ctor instead, which seems OK. Then maybe the iterators would be a private implementation detail of TypeStream, or at least have some private API that (via friendship) only TypeStream can use (for construction, etc).

(still missing the error handling here - I'd imagine TypeStream would have a named ctor that returns ErrorOr<TypeStream> - not sure about errors during iteration (beyond the initial construction). That gets trickier)

133–136

Prefer to use the initializer list to initialize members (& you shouldn't need to initialize 'End' at all in that case, since it's default constructed)

141

Difficult question about style: while begin/end need to be spelled that way, magic here could match the LLVM coding conventions and be called 'getMagic' or similar.

[amccarth]

I agree with dblaikie's suggestions, but I don't see any issues beyond those.

[zturner]

Regarding the magic, I honestly don't even think this class should handle the magic. It should be handled at a higher level. The type stream in a PDB file doesn't have this magic anyway, it's just a list of records.

[amccarth]

This class started out as the higher-level thing, and it grew into an
iterator (and the input got lower level: StringRef instead of SectionRef).

[zturner]

Various changes as suggested by previous reviews.

The TypeStream is no longer its own class, you can just create an iterator pair. The magic is not part of a PDB's type stream, and it seems to be specific to parsing a .debug$T section of an object file, so that logic is extracted into the dumper while the codeview library assumes that the first byte of the stream is the first byte of the first type record. Updated DebugInfoPDB to use this new iterator logic from DebugInfoCodeview rather than hand parsing the codeview type stream.

[amccarth]

I'm not thrilled with the backsliding with respect to the parsing of the magic number. The magic number is part of the format of the type stream, and the original intent was to abstract the parsing of the typestream so that multiple users didn't have to know about the details. Now we're exposing this detail again and making the callers know that they have to pass a subset of the typestream to construct the iterators.

Consider what happens in the future if there's a newer typestream format, and the indicator is in the magic number field. This would completely break the abstraction. The createCodeViewTypeStream function (which you removed) would probably have been a better place to handle the magic number).

That said, I won't object because I know this is still a work in progress, so I'm confident we'll find the right level of abstraction eventually.

[zturner]

In D19887#420489, @amccarth wrote:

I'm not thrilled with the backsliding with respect to the parsing of the magic number. The magic number is part of the format of the type stream, and the original intent was to abstract the parsing of the typestream so that multiple users didn't have to know about the details. Now we're exposing this detail again and making the callers know that they have to pass a subset of the typestream to construct the iterators.

Consider what happens in the future if there's a newer typestream format, and the indicator is in the magic number field. This would completely break the abstraction. The createCodeViewTypeStream function (which you removed) would probably have been a better place to handle the magic number).

That said, I won't object because I know this is still a work in progress, so I'm confident we'll find the right level of abstraction eventually.

Well I guess what I'm finding is that the magic number is not part of the format of a codeview type stream, but rather part of the format of a .debug$T section of an object file. I'm open to putting it somewhere else, but I think we need some kind of abstraction that simply iterates a raw sequence of types, since that's the lowest common denominator that all type stream containers hold.

[zturner]

BTW, I found out that the magic is actually equal to COFF::DEBUG_SECTION_MAGIC, and printCodeViewSymbolSection has the same check inside of it. So I added a safety check to the code view type dumper that errors out if the magic is not equal to COFF::DEBUG_SECTION_MAGIC.

include/llvm/DebugInfo/CodeView/TypeStream.h

+//===- TypeStream.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_TYPESTREAM_H
+#define LLVM_DEBUGINFO_CODEVIEW_TYPESTREAM_H
+
+#include "CodeView.h"
+
+#include "llvm/ADT/StringRef.h"
+
+#include <stdint.h>
+
+namespace llvm {
+namespace codeview {
+
+/// Consumes sizeof(T) bytes from the given byte sequence. Returns an error if
+/// there are not enough bytes remaining. Reinterprets the consumed bytes as a
+/// T object and points 'Res' at them.
+template <typename T>
+static std::error_code consumeObject(StringRef &Data, const T *&Res) {
+  if (Data.size() < sizeof(*Res))
+    return object_error::parse_failed;
+  Res = reinterpret_cast<const T *>(Data.data());
+  Data = Data.drop_front(sizeof(*Res));
+  return std::error_code();
+}
+
+static std::error_code consumeUInt32(StringRef &Data, uint32_t &Res) {
+  const ulittle32_t *IntPtr;
+  if (auto EC = consumeObject(Data, IntPtr))
+    return EC;
+  Res = *IntPtr;
+  return std::error_code();
+}
+
+// A const input iterator interface to the CodeView type stream.
+class TypeIterator {
+public:
+  struct TypeRecord {
+    std::size_t Length;
+    TypeLeafKind Leaf;
+    StringRef LeafData;
+  };
+
+  explicit TypeIterator(const StringRef &SectionData)
+      : Data(SectionData), AtEnd(false) {
+    if (Data.size() >= 4) {
+      Magic = *reinterpret_cast<const ulittle32_t *>(Data.data());
+      Data = Data.drop_front(4);
+    }
+    next(); // Prime the pump
+  }
+
+  TypeIterator() : AtEnd(true) {}
+
+  // For iterators to compare equal, they must both point at the same record
+  // in the same data stream, or they must both be at the end of a stream.
+  friend bool operator==(const TypeIterator &lhs, const TypeIterator &rhs);
+
+  friend bool operator!=(const TypeIterator &lhs, const TypeIterator &rhs);
+
+  unsigned getMagic() const { return Magic; }
+
+  const TypeRecord &operator*() const {
+    assert(!AtEnd);
+    return Current;
+  }
+
+  const TypeRecord *operator->() const {
+    assert(!AtEnd);
+    return &Current;
+  }
+
+  TypeIterator operator++() {
+    next();
+    return *this;
+  }
+
+  TypeIterator operator++(int) {
+    TypeIterator Original = *this;
+    ++*this;
+    return Original;
+  }
+
+private:
+  void next() {
+    assert(!AtEnd && "Attempted to advance more than one past the last rec");
+    if (Data.empty()) {
+      // We've advanced past the last record.
+      AtEnd = true;
+      return;
+    }
+
+    const TypeRecordPrefix *Rec;
+    if (consumeObject(Data, Rec))
+      return;
+    Current.Length = Rec->Len;
+    Current.Leaf = static_cast<TypeLeafKind>(uint16_t(Rec->Leaf));
+    Current.LeafData = Data.substr(0, Current.Length - 2);
+
+    // The next record starts immediately after this one.
+    Data = Data.drop_front(Current.LeafData.size());
+
+    // FIXME: The stream contains LF_PAD bytes that we need to ignore, but those
+    // are typically included in LeafData. We may need to call skipPadding() if
+    // we ever find a record that doesn't count those bytes.
+
+    return;
+  }
+
+  StringRef Data;
+  unsigned Magic = 0;
+  TypeRecord Current;
+  bool AtEnd;
+};
+
+bool operator==(const TypeIterator &lhs, const TypeIterator &rhs) {
+  return (lhs.Data.begin() == rhs.Data.begin()) || (lhs.AtEnd && rhs.AtEnd);
+}
+
+bool operator!=(const TypeIterator &lhs, const TypeIterator &rhs) {
+  return !(lhs == rhs);
+}
+
+class TypeStream {
+public:
+  TypeStream(const StringRef &Data) {
+    Begin = TypeIterator(Data);
+    End = TypeIterator();
+    Magic = Begin.getMagic();
+  }
+
+  TypeIterator begin() const { return Begin; }
+  TypeIterator end() const { return End; }
+  unsigned magic() const { return Magic; }
+
+private:
+  TypeIterator Begin;
+  TypeIterator End;
+  unsigned Magic;
+};
+}
+}
+
+#endif

tools/llvm-readobj/COFFDumper.cpp

 #include "llvm/DebugInfo/CodeView/SymbolRecord.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
+#include "llvm/DebugInfo/CodeView/TypeStream.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/COFF.h"

   }
 }
 
-/// Consumes sizeof(T) bytes from the given byte sequence. Returns an error if
-/// there are not enough bytes remaining. Reinterprets the consumed bytes as a
-/// T object and points 'Res' at them.
-template <typename T>
-static std::error_code consumeObject(StringRef &Data, const T *&Res) {
-  if (Data.size() < sizeof(*Res))
-    return object_error::parse_failed;
-  Res = reinterpret_cast<const T *>(Data.data());
-  Data = Data.drop_front(sizeof(*Res));
-  return std::error_code();
-}
-
-static std::error_code consumeUInt32(StringRef &Data, uint32_t &Res) {
-  const ulittle32_t *IntPtr;
-  if (auto EC = consumeObject(Data, IntPtr))
-    return EC;
-  Res = *IntPtr;
-  return std::error_code();
-}
-
 void COFFDumper::initializeFileAndStringTables(StringRef Data) {
   while (!Data.empty() && (CVFileChecksumTable.data() == nullptr ||
                            CVStringTable.data() == nullptr)) {

   return "UnknownLeaf";
 }
 
-// A const input iterator interface to the CodeView type stream.
-class CodeViewTypeIterator {
-public:
-  struct TypeRecord {
-    std::size_t Length;
-    TypeLeafKind Leaf;
-    StringRef LeafData;
-  };
-
-  explicit CodeViewTypeIterator(const StringRef &SectionData)
-      : Data(SectionData), AtEnd(false) {
-    if (Data.size() >= 4) {
-      Magic = *reinterpret_cast<const ulittle32_t *>(Data.data());
-      Data = Data.drop_front(4);
-    }
-    next(); // Prime the pump
-  }
-
-  CodeViewTypeIterator() : AtEnd(true) {}
-
-  // For iterators to compare equal, they must both point at the same record
-  // in the same data stream, or they must both be at the end of a stream.
-  friend bool operator==(const CodeViewTypeIterator &lhs,
-                         const CodeViewTypeIterator &rhs);
-
-  friend bool operator!=(const CodeViewTypeIterator &lhs,
-                         const CodeViewTypeIterator &rhs);
-
-  unsigned getMagic() const { return Magic; }
-
-  const TypeRecord &operator*() const {
-    assert(!AtEnd);
-    return Current;
-  }
-
-  const TypeRecord *operator->() const {
-    assert(!AtEnd);
-    return &Current;
-  }
-
-  CodeViewTypeIterator operator++() {
-    next();
-    return *this;
-  }
-
-  CodeViewTypeIterator operator++(int) {
-    CodeViewTypeIterator Original = *this;
-    ++*this;
-    return Original;
-  }
-
-private:
-  void next() {
-    assert(!AtEnd && "Attempted to advance more than one past the last rec");
-    if (Data.empty()) {
-      // We've advanced past the last record.
-      AtEnd = true;
-      return;
-    }
-
-    const TypeRecordPrefix *Rec;
-    if (consumeObject(Data, Rec))
-      return;
-    Current.Length = Rec->Len;
-    Current.Leaf = static_cast<TypeLeafKind>(uint16_t(Rec->Leaf));
-    Current.LeafData = Data.substr(0, Current.Length - 2);
-
-    // The next record starts immediately after this one.
-    Data = Data.drop_front(Current.LeafData.size());
-
-    // FIXME: The stream contains LF_PAD bytes that we need to ignore, but those
-    // are typically included in LeafData. We may need to call skipPadding() if
-    // we ever find a record that doesn't count those bytes.
-
-    return;
-  }
-
-  StringRef Data;
-  unsigned Magic = 0;
-  TypeRecord Current;
-  bool AtEnd;
-};
-
-bool operator==(const CodeViewTypeIterator &lhs,
-                const CodeViewTypeIterator &rhs) {
-  return (lhs.Data.begin() == rhs.Data.begin()) || (lhs.AtEnd && rhs.AtEnd);
-}
-
-bool operator!=(const CodeViewTypeIterator &lhs,
-                const CodeViewTypeIterator &rhs) {
-  return !(lhs == rhs);
-}
-
-struct CodeViewTypeStream {
-  CodeViewTypeIterator begin;
-  CodeViewTypeIterator end;
-  unsigned Magic;
-};
-
-CodeViewTypeStream CreateCodeViewTypeIter(const StringRef &Data) {
-  CodeViewTypeStream Stream;
-  Stream.begin = CodeViewTypeIterator(Data);
-  Stream.end   = CodeViewTypeIterator();
-  Stream.Magic = Stream.begin.getMagic();
-
-  return Stream;
-}
 
 void COFFDumper::printCodeViewTypeSection(StringRef SectionName,
                                           const SectionRef &Section) {

 }
 
 void CVTypeDumper::dump(StringRef Data) {
-  CodeViewTypeStream Stream = CreateCodeViewTypeIter(Data);
-  W.printHex("Magic", Stream.Magic);
+  TypeStream Stream(Data);
+  W.printHex("Magic", Stream.magic());
 
-  for (auto Iter = Stream.begin; Iter != Stream.end; ++Iter) {
-    StringRef LeafData = Iter->LeafData;
+  for (const auto &Record : Stream) {
+    StringRef LeafData = Record.LeafData;
 
     // Find the name of this leaf type.
-    StringRef LeafName = getLeafTypeName(Iter->Leaf);
+    StringRef LeafName = getLeafTypeName(Record.Leaf);
     DictScope S(W, LeafName);
     unsigned NextTypeIndex = 0x1000 + CVUDTNames.size();
-    W.printEnum("TypeLeafKind", unsigned(Iter->Leaf),
+    W.printEnum("TypeLeafKind", unsigned(Record.Leaf),
                 makeArrayRef(LeafTypeNames));
     W.printHex("TypeIndex", NextTypeIndex);
 
     // Fill this in inside the switch to get something in CVUDTNames.
     StringRef Name;
 
-    switch (Iter->Leaf) {
+    switch (Record.Leaf) {
     default: {
-      W.printHex("Size", Iter->Length);
+      W.printHex("Size", Record.Length);
       break;
     }
 

     }
 
     case LF_FIELDLIST: {
-      W.printHex("Size", Iter->Length);
+      W.printHex("Size", Record.Length);
       // FieldList has no fixed prefix that can be described with a struct. All
       // the bytes must be interpreted as more records.
       printCodeViewFieldList(LeafData);