NFC: An iterator for stepping through CodeView type stream in llvm-readobj

[amccarth]

Summary

This is a small refactoring step toward moving CodeView type stream logic from llvm-readobj to a library. It abstracts the logic of stepping through the stream into an iterator class and updates llvm-readobj to use that iterator. This has no functional change; llvm-readobj produces identical output.

The next step is to abstract the parsing of the different leaf types and then move that and the iterator into a library.

Since this is my first contrib outside LLDB, please let me know if I'm messing up on any of the LLVM style guidelines, idioms, or patterns.

[dblaikie]

tools/llvm-readobj/COFFDumper.cpp

1855

You can use std::ignore if you don't want some of the results (eg: "std::tie(Leading, std::ignore)")

Or you could just write "return LeafData.split('\0').first;"

But this code was in the original, so not really your responsibility to refactor.

1979

You can remove "Data()" from the init list, it doesn't do anything, presumably "Current()" too.

& you can use a non-static data member initializer for Magic. Then you just need to initialize AtEnd in the init list of the two ctors.

1980

LLVM doesn't generally use const on local values (you can, just saying - it's a bit uncommon/atypical)

1981–1982

You could roll the variable declaration into the conditional to reduce its scope to just as much as is needed:

if (std::error_code Error = ...)
  throw Error;

Also - throw? LLVM doesn't use exceptions...

You may need a named ctor, or another way of handling construction failure.

1994

Generally any operator overloads that can be non-members, should be (ensures that any implicit conversions are equally viable to the LHS and RHS of an expression using that operator)

You can just stick "friend" in front of these, and add the other parameter to the parameter list rather than using 'this'.

2002

Having a custom function on an iterator like this might be a bit awkward (eg: you wouldn't be able to access this value in a range-based for loop). Is that OK?

Perhaps this could be addressed along with the "how to fail in the ctor" issue - or, maybe if the ctor has to be fail-able anyway, you wouldn't just build one in a range-based-for anyway, so calling a member function on this iterator wouldn't be too surprising.

On the 3rd hand - Magic doesn't seem to be used by this iterator, it's just computed during construction - so once there's a named ctor thing that can produce an error, it can also produce the Magic without it being part of the iterator (& thus reducing the size of the iterator making it more suitable to do iterator-y things with (pass by value, copy cheaply, etc))

2020

Prefer copy init over direct init when the two are equivalent. This avoids any accidental explicit conversions, making the code easier to read:

CodeViewTypeIterator original = *this;

Also, naming convention - variables start with upper case ( http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly )

2021

You could write this as ++*this;

2035–2037

Roll the variable declaration into the condition:

if (std::error_code Error = ...)
  return;

& in fact, you'll get an unused variable warning then, so it'll just be:

if (consumeObject(...))
  return;

[rnk]

tools/llvm-readobj/COFFDumper.cpp

1978–1982

I think the best way to handle this error case is to have the caller do the getContents call. Then we aren't in the awkward position handling errors in the constructor.

Alternatively, we could store the error code in the iterator (or some parent iterator state object) and check for it after the loop exit.

2026

In LLVM, methods are leading lower-case:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

We are famously inconsistent here and the rules don't make a lot of sense, but we're trying to improve.

[amccarth]

Thanks for all the detailed feedback. I'll take a stab at a named constructor to fix the awkwardness of handling the magic number before I update the diff.

tools/llvm-readobj/COFFDumper.cpp

1981–1982

Sorry about that. I meant to circle back and take care of the throw.

1994

Generally any operator overloads that can be non-members, should be

Yes, but I generally interpret "can be" as "implementable via the rest of its public interface." With friend you can always make it a non-member, right?

(ensures that any implicit conversions are equally viable to the LHS and RHS of an expression using that operator)

Right, but this class has no implicit conversions.

I'm happy to make the change if you think it's worthwhile. I'm just explaining why I didn't do it that way the first time.

2002

I agree it's awkward. This wrapper didn't start out with an iterator-like interface. The intent was to hide the details of parsing the "stream." The "stream" consists of a magic number followed by a series of types. Eventually the iterator interface began to make sense for everything after the magic number.

The caller may or may not care about the magic number. The llvm-readobj does, but another client might not, and it would be perfectly happy to use a range-based for loop to just get the types. This allows both approaches.

Let me consider the named constructor.

2020

[Copy initialization] avoids any accidental explicit conversions

Explicit conversions? How so? Both ways are prone to implicit conversions, which seems like more of a problem. Fortunately, this class has no conversions.

(I've made the change to be consistent with LLVM style. I'm just trying to understand how direct init could allow an accidental explicit conversion.)

variables start with upper case

Oops, I forgot. That's going to be crazy hard to get used to.

2026

It's like living in Opposite Land. (-:

2035–2037

Sorry, this was a leftover from when Next (now next) was returning the error.

[amccarth]

Addressed the comments from the first round. Rebased on top of rnk's recent changes to this file.

[rnk]

lgtm

tools/llvm-readobj/COFFDumper.cpp

private:
   typedef DenseMap<const coff_section*, std::vector<RelocationRef> > RelocMapTy;
 
   const llvm::object::COFFObjectFile *Obj;
   bool RelocCached = false;
   RelocMapTy RelocMap;
   StringRef CVFileChecksumTable;
   StringRef CVStringTable;
 
-  /// 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
+  /// 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;
 };
 
 } // namespace
 
if (HasExtraFiles) {
         uint32_t FileID;
         error(consumeUInt32(Data, FileID));
         printFileNameForOffset("FileID", FileID);
       }
     }
   }
 }
 
-StringRef getRemainingTypeBytes(const TypeRecordPrefix *Rec, const char *Start) {
-  ptrdiff_t StartOffset = Start - reinterpret_cast<const char *>(Rec);
-  size_t RecSize = Rec->Len + 2;
-  assert(StartOffset >= 0 && "negative start-offset!");
-  assert(static_cast<size_t>(StartOffset) <= RecSize &&
-         "Start beyond the end of Rec");
-  return StringRef(Start, RecSize - StartOffset);
-}
-
-StringRef getRemainingBytesAsString(const TypeRecordPrefix *Rec, const char *Start) {
-  StringRef Remaining = getRemainingTypeBytes(Rec, Start);
+StringRef getLeafDataBytesAsString(StringRef LeafData) {
   StringRef Leading, Trailing;
-  std::tie(Leading, Trailing) = Remaining.split('\0');
+  std::tie(Leading, Trailing) = LeafData.split('\0');
   return Leading;
 }
 
 StringRef COFFDumper::getTypeName(TypeIndex TI) {
   if (TI.isNoType())
     return "<no type>";
 
   if (TI.isSimple()) {
static StringRef getLeafTypeName(TypeLeafKind LT) {
   case LF_VTSHAPE: return "VTableShape";
   case LF_UDT_SRC_LINE: return "UDTSrcLine";
   case LF_BUILDINFO: return "BuildInfo";
   default: break;
   }
   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 SectionRef &Section)
+      : Data(), Magic(0), Current(), AtEnd(false) {
+    const std::error_code Error = Section.getContents(Data);
+    if (Error)
+      throw Error;
+    if (Data.size() >= 4) {
+      Magic = *reinterpret_cast<const ulittle32_t *>(Data.data());
+      Data = Data.drop_front(4);
+    }
+    Next(); // Prime the pump
+  }
+
+  CodeViewTypeIterator() : Data(), Magic(0), Current(), 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.
+  bool operator==(const CodeViewTypeIterator &other) const {
+    return (Data.begin() == other.Data.begin()) || (AtEnd && other.AtEnd);
+  }
+
+  bool operator!=(const CodeViewTypeIterator &other) const {
+    return !(*this == other);
+  }
+
+  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->operator++();
+    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;
+    std::error_code Error = consumeObject(Data, Rec);
+    if (Error)
+      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;
+  TypeRecord Current;
+  bool AtEnd;
+};
+
 void COFFDumper::printCodeViewTypeSection(StringRef SectionName,
                                           const SectionRef &Section) {
   ListScope D(W, "CodeViewTypes");
   W.printNumber("Section", SectionName, Obj->getSectionID(Section));
+
   StringRef Data;
   error(Section.getContents(Data));
   W.printBinaryBlock("Data", Data);
 
-  unsigned Magic = *reinterpret_cast<const ulittle32_t *>(Data.data());
-  W.printHex("Magic", Magic);
-
-  Data = Data.drop_front(4);
+  CodeViewTypeIterator Iter(Section);
+  W.printHex("Magic", Iter.getMagic());
 
-  while (!Data.empty()) {
-    const TypeRecordPrefix *Rec;
-    error(consumeObject(Data, Rec));
-    auto Leaf = static_cast<TypeLeafKind>(uint16_t(Rec->Leaf));
-
-    // This record is 'Len - 2' bytes, and the next one starts immediately
-    // afterwards.
-    StringRef LeafData = Data.substr(0, Rec->Len - 2);
-    StringRef RemainingData = Data.drop_front(LeafData.size());
+  CodeViewTypeIterator End;
+  for (; Iter != End; ++Iter) {
+    StringRef LeafData = Iter->LeafData;
 
     // Find the name of this leaf type.
-    StringRef LeafName = getLeafTypeName(Leaf);
+    StringRef LeafName = getLeafTypeName(Iter->Leaf);
     DictScope S(W, LeafName);
     unsigned NextTypeIndex = 0x1000 + CVUDTNames.size();
-    W.printEnum("TypeLeafKind", unsigned(Leaf), makeArrayRef(LeafTypeNames));
+    W.printEnum("TypeLeafKind", unsigned(Iter->Leaf),
+                makeArrayRef(LeafTypeNames));
     W.printHex("TypeIndex", NextTypeIndex);
 
     // Fill this in inside the switch to get something in CVUDTNames.
     StringRef Name;
 
-    switch (Leaf) {
+    switch (Iter->Leaf) {
     default: {
-      W.printHex("Size", Rec->Len);
+      W.printHex("Size", Iter->Length);
       break;
     }
 
     case LF_STRING_ID: {
       const StringId *String;
       error(consumeObject(LeafData, String));
       W.printHex("Id", String->id.getIndex());
-      StringRef StringData = getRemainingBytesAsString(Rec, LeafData.data());
+      StringRef StringData = getLeafDataBytesAsString(LeafData);
       W.printString("StringData", StringData);
       // Put this in CVUDTNames so it gets printed with LF_UDT_SRC_LINE.
       Name = StringData;
       break;
     }
 
     case LF_FIELDLIST: {
-      W.printHex("Size", Rec->Len);
+      W.printHex("Size", Iter->Length);
       // FieldList has no fixed prefix that can be described with a struct. All
       // the bytes must be interpreted as more records.
       printCodeViewFieldList(LeafData);
       break;
     }
 
     case LF_ARGLIST:
     case LF_SUBSTR_LIST: {
case LF_INTERFACE: {
       printTypeIndex("VShape", Class->VShape);
       uint64_t SizeOf;
       error(decodeUIntLeaf(LeafData, SizeOf));
       W.printNumber("SizeOf", SizeOf);
       StringRef LinkageName;
       std::tie(Name, LinkageName) = LeafData.split('\0');
       W.printString("Name", Name);
       if (Props & uint16_t(ClassOptions::HasUniqueName)) {
-        LinkageName = getRemainingBytesAsString(Rec, LinkageName.data());
+        LinkageName = getLeafDataBytesAsString(LinkageName);
         if (LinkageName.empty())
           return error(object_error::parse_failed);
         W.printString("LinkageName", LinkageName);
       }
       break;
     }
 
     case LF_UNION: {
       const UnionType *Union;
       error(consumeObject(LeafData, Union));
       W.printNumber("MemberCount", Union->MemberCount);
       uint16_t Props = Union->Properties;
       W.printFlags("Properties", Props, makeArrayRef(ClassOptionNames));
       printTypeIndex("FieldList", Union->FieldList);
       uint64_t SizeOf;
       error(decodeUIntLeaf(LeafData, SizeOf));
       W.printNumber("SizeOf", SizeOf);
       StringRef LinkageName;
       std::tie(Name, LinkageName) = LeafData.split('\0');
       W.printString("Name", Name);
       if (Props & uint16_t(ClassOptions::HasUniqueName)) {
-        LinkageName = getRemainingBytesAsString(Rec, LinkageName.data());
+        LinkageName = getLeafDataBytesAsString(LinkageName);
         if (LinkageName.empty())
           return error(object_error::parse_failed);
         W.printString("LinkageName", LinkageName);
       }
       break;
     }
 
     case LF_ENUM: {
case LF_BUILDINFO: {
       break;
     }
     }
 
     if (opts::CodeViewSubsectionBytes)
       W.printBinaryBlock("LeafData", LeafData);
 
     CVUDTNames.push_back(Name);
-
-    Data = RemainingData;
-    // 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.
   }
 }
 
 static StringRef skipPadding(StringRef Data) {
   if (Data.empty())
     return Data;
   uint8_t Leaf = Data.front();
   if (Leaf < LF_PAD0)