Refactor 1 symbol record

[zturner]

Summary

There are many more, but this just touches 1 record so we can see what the code is going to look like.

There is one potential problem with this patch which exposes a limitation in the underlying design we've chosen for the symbol and type records. dumping symbol records relies on calling a function printRelocatedField which assumes that you have a pointer into the section data so that it can compute the section offset. This isn't true anymore once we're copying bytes out of the input stream and into an in memory structure, so it makes it so we can't use printRelocatedField. However, this particular symbol (ProcSym) does have another field which is the section offset. So maybe we can use that. But I haven't checked whether it's the same as what we think it is yet, nor have I confirmed that this will work for the general case of all other types of symbols.

[zturner]

Slightly changed the way we do this so that we can still use the relocated field to get the linkage name. This diverges slightly from how we're doing type records, but not by much, and it still allows us to use the generic deserialize methods. But we just store a pointer directly to the layout rather than copying the fields. Who knows, maybe we can eventually adopt this approach for types as well.

[zturner]

Yea originally PublicSymbolStreamIndex was called PSSyms too, so it must be the same correlation.

[zturner]

This refactors all the symbol records as discussed offline. The code got shorter in some places, but more verbose in others, as now every case of the switch statement has to repeat the same 5-6 lines of code to try the deserialization.

This is just a stepping stone to introducing a symbol visitor however, and once we have this will become much simpler. I didn't want to do too much in one patch though, so that will come in a followup

[rnk]

include/llvm/DebugInfo/CodeView/SymbolRecord.h

95

Should this really modify the Annotations member variable? Maybe it should be a static method that takes an ArrayRef by non-const reference.

Also, it should be in a .cpp file.

[zturner]

include/llvm/DebugInfo/CodeView/SymbolRecord.h

95

Probably not, but that's essentially what the old code did. I can make it so that it doesn't.

[zturner]

Updated based on rnk's previous comments. Probably overkill, but now you can iterate annotations using ranged base for syntax.

[rnk]

rubber stamp, looks good =)

tools/llvm-readobj/COFFDumper.cpp

1157–1166

Should this be consistent and use Annotation.Name?

[Eugene.Zelenko]

Committed in r270475.

include/llvm/DebugInfo/CodeView/RecordSerialization.h

   U Func;
 };
 
-template <typename T> struct serialize_array_tail_impl {
-  serialize_array_tail_impl(std::vector<T> &Item) : Item(Item) {}
+template <typename T> struct serialize_vector_tail_impl {
+  serialize_vector_tail_impl(std::vector<T> &Item) : Item(Item) {}
 
   std::error_code deserialize(ArrayRef<uint8_t> &Data) const {
     T Field;

   std::vector<T> &Item;
 };
 
+template <typename T> struct serialize_arrayref_tail_impl {
+  serialize_arrayref_tail_impl(ArrayRef<T> &Item) : Item(Item) {}
+
+  std::error_code deserialize(ArrayRef<uint8_t> &Data) const {
+    uint32_t Count = Data.size() / sizeof(T);
+    Item = ArrayRef<T>(reinterpret_cast<const T *>(Data.begin()), Count);
+    return std::error_code();
+  }
+
+  ArrayRef<T> &Item;
+};
+
 template <typename T> struct serialize_numeric_impl {
   serialize_numeric_impl(T &Item) : Item(Item) {}
 

 }
 
 template <typename T>
-serialize_array_tail_impl<T> serialize_array_tail(std::vector<T> &Item) {
-  return serialize_array_tail_impl<T>(Item);
+serialize_vector_tail_impl<T> serialize_array_tail(std::vector<T> &Item) {
+  return serialize_vector_tail_impl<T>(Item);
+}
+
+template <typename T>
+serialize_arrayref_tail_impl<T> serialize_array_tail(ArrayRef<T> &Item) {
+  return serialize_arrayref_tail_impl<T>(Item);
 }
 
 template <typename T> serialize_numeric_impl<T> serialize_numeric(T &Item) {

 
 template <typename T>
 std::error_code consume(ArrayRef<uint8_t> &Data,
-                        const serialize_array_tail_impl<T> &Item) {
+                        const serialize_vector_tail_impl<T> &Item) {
+  return Item.deserialize(Data);
+}
+
+template <typename T>
+std::error_code consume(ArrayRef<uint8_t> &Data,
+                        const serialize_arrayref_tail_impl<T> &Item) {
   return Item.deserialize(Data);
 }
 

include/llvm/DebugInfo/CodeView/SymbolRecord.h

 
 #include "llvm/DebugInfo/CodeView/CodeView.h"
 #include "llvm/DebugInfo/CodeView/RecordIterator.h"
+#include "llvm/DebugInfo/CodeView/RecordSerialization.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/Support/Endian.h"
+#include "llvm/Support/Error.h"
 
 namespace llvm {
 namespace codeview {

 using llvm::support::ulittle32_t;
 using llvm::support::little32_t;
 
+class SymbolRecord {
+protected:
+  explicit SymbolRecord(SymbolRecordKind Kind) : Kind(Kind) {}
+
+public:
+  SymbolRecordKind getKind() const { return Kind; }
+
+private:
+  SymbolRecordKind Kind;
+};
+
 // S_GPROC32, S_LPROC32, S_GPROC32_ID, S_LPROC32_ID, S_LPROC32_DPC or
 // S_LPROC32_DPC_ID
-struct ProcSym {
-  ulittle32_t PtrParent;
-  ulittle32_t PtrEnd;
-  ulittle32_t PtrNext;
-  ulittle32_t CodeSize;
-  ulittle32_t DbgStart;
-  ulittle32_t DbgEnd;
-  TypeIndex FunctionType;
-  ulittle32_t CodeOffset;
-  ulittle16_t Segment;
-  uint8_t Flags; // ProcSymFlags enum
-  // Name: The null-terminated name follows.
+class ProcSym : public SymbolRecord {
+public:
+  struct Hdr {
+    ulittle32_t PtrParent;
+    ulittle32_t PtrEnd;
+    ulittle32_t PtrNext;
+    ulittle32_t CodeSize;
+    ulittle32_t DbgStart;
+    ulittle32_t DbgEnd;
+    TypeIndex FunctionType;
+    ulittle32_t CodeOffset;
+    ulittle16_t Segment;
+    uint8_t Flags; // ProcSymFlags enum
+                   // Name: The null-terminated name follows.
+  };
+
+  ProcSym(SymbolRecordKind Kind, const Hdr *H, StringRef Name)
+      : SymbolRecord(Kind), Header(H), Name(Name) {}
+
+  static ErrorOr<ProcSym> deserialize(SymbolRecordKind Kind,
+                                      ArrayRef<uint8_t> &Data) {
+    const Hdr *H = nullptr;
+    StringRef Name;
+    CV_DESERIALIZE(Data, H, Name);
+
+    return ProcSym(Kind, H, Name);
+  }
+
+  const Hdr *Header;
+  StringRef Name;
 };
 
 // S_INLINESITE
-struct InlineSiteSym {
-  ulittle32_t PtrParent;
-  ulittle32_t PtrEnd;
-  TypeIndex Inlinee;
-  // BinaryAnnotations
+class InlineSiteSym : public SymbolRecord {
+public:
+  struct Hdr {
+    ulittle32_t PtrParent;
+    ulittle32_t PtrEnd;
+    TypeIndex Inlinee;
+    // BinaryAnnotations
+  };
+
+  InlineSiteSym(const Hdr *H, ArrayRef<uint8_t> Annotations)
+      : SymbolRecord(SymbolRecordKind::InlineSiteSym), Header(H),
+        Annotations(Annotations) {}
+
+  static ErrorOr<InlineSiteSym> deserialize(SymbolRecordKind Kind,
+                                            ArrayRef<uint8_t> &Data) {
+    const Hdr *H = nullptr;
+    ArrayRef<uint8_t> Annotations;
+    CV_DESERIALIZE(Data, H, CV_ARRAY_FIELD_TAIL(Annotations));
+
+    return InlineSiteSym(H, Annotations);
+  }
+
+  uint32_t GetCompressedAnnotation() {
+    if (Annotations.empty())
+      return -1;
+
+    uint8_t FirstByte = Annotations.front();
+    Annotations = Annotations.drop_front();
+
+    if ((FirstByte & 0x80) == 0x00)
+      return FirstByte;
+
+    if (Annotations.empty())
+      return -1;
+
+    uint8_t SecondByte = Annotations.front();
+    Annotations = Annotations.drop_front();
+
+    if ((FirstByte & 0xC0) == 0x80)
+      return ((FirstByte & 0x3F) << 8) | SecondByte;
+
+    if (Annotations.empty())
+      return -1;
+
+    uint8_t ThirdByte = Annotations.front();
+    Annotations = Annotations.drop_front();
+
+    if (Annotations.empty())
+      return -1;
+
+    uint8_t FourthByte = Annotations.front();
+    Annotations = Annotations.drop_front();
+
+    if ((FirstByte & 0xE0) == 0xC0)
+      return ((FirstByte & 0x1F) << 24) | (SecondByte << 16) |
+             (ThirdByte << 8) | FourthByte;
+
+    return -1;
+  };
+
+  int32_t DecodeSignedOperand(uint32_t Operand) const {
+    if (Operand & 1)
+      return -(Operand >> 1);
+    return Operand >> 1;
+  };
+
+  int32_t DecodeSignedOperand() {
+    return DecodeSignedOperand(GetCompressedAnnotation());
+  };
+
+  const Hdr *Header;
+  ArrayRef<uint8_t> Annotations;
 };
 
 // S_LOCAL
-struct LocalSym {
-  TypeIndex Type;
-  ulittle16_t Flags; // LocalSymFlags enum
-  // Name: The null-terminated name follows.
+class LocalSym : public SymbolRecord {
+public:
+  struct Hdr {
+    TypeIndex Type;
+    ulittle16_t Flags; // LocalSymFlags enum
+                       // Name: The null-terminated name follows.
+  };
+
+  LocalSym(const Hdr *H, StringRef Name)
+      : SymbolRecord(SymbolRecordKind::LocalSym), Header(H), Name(Name) {}
+
+  static ErrorOr<LocalSym> deserialize(SymbolRecordKind Kind,
+                                       ArrayRef<uint8_t> &Data) {
+    const Hdr *H = nullptr;
+    StringRef Name;
+    CV_DESERIALIZE(Data, H, Name);
+
+    return LocalSym(H, Name);
+  }
+
+  const Hdr *Header;
+  StringRef Name;
 };
 
 struct LocalVariableAddrRange {

tools/llvm-readobj/COFFDumper.cpp

   void printCodeViewInlineeLines(StringRef Subsection);
 
   void printRelocatedField(StringRef Label, const coff_section *Sec,
+    StringRef SectionContents, const uint32_t Offset, StringRef *RelocSym = nullptr);
+
+    void printRelocatedField(StringRef Label, const coff_section *Sec,
                            StringRef SectionContents, const ulittle32_t *Field,
                            StringRef *RelocSym = nullptr);
 

 }
 
 void COFFDumper::printRelocatedField(StringRef Label, const coff_section *Sec,
+  StringRef SectionContents, const uint32_t Offset, StringRef *RelocSym) {
+  StringRef SymStorage;
+  StringRef &Symbol = RelocSym ? *RelocSym : SymStorage;
+  if (!resolveSymbolName(Sec, Offset, Symbol))
+    W.printSymbolOffset(Label, Symbol, Offset);
+  else
+    W.printHex(Label, Offset);
+}
+
+void COFFDumper::printRelocatedField(StringRef Label, const coff_section *Sec,
                                      StringRef SectionContents,
                                      const ulittle32_t *Field,
                                      StringRef *RelocSym) {

     case S_LPROC32_DPC:
     case S_LPROC32_DPC_ID: {
       DictScope S(W, "ProcStart");
-      const ProcSym *Proc;
-      error(consumeObject(SymData, Proc));
+      ArrayRef<uint8_t> Data(SymData.bytes_begin(), SymData.bytes_end());
+      auto ProcOrError =
+          ProcSym::deserialize(static_cast<SymbolRecordKind>(Kind), Data);
+      if (!ProcOrError)
+        error(ProcOrError.getError());
+      auto &Proc = ProcOrError.get();
+
       if (InFunctionScope)
         return error(object_error::parse_failed);
       InFunctionScope = true;
 
       StringRef LinkageName;
-      StringRef DisplayName = SymData.split('\0').first;
-      W.printHex("PtrParent", Proc->PtrParent);
-      W.printHex("PtrEnd", Proc->PtrEnd);
-      W.printHex("PtrNext", Proc->PtrNext);
-      W.printHex("CodeSize", Proc->CodeSize);
-      W.printHex("DbgStart", Proc->DbgStart);
-      W.printHex("DbgEnd", Proc->DbgEnd);
-      printTypeIndex("FunctionType", Proc->FunctionType);
-      printRelocatedField("CodeOffset", Sec, SectionContents, &Proc->CodeOffset,
-                          &LinkageName);
-      W.printHex("Segment", Proc->Segment);
-      W.printFlags("Flags", Proc->Flags, makeArrayRef(ProcSymFlagNames));
-      W.printString("DisplayName", DisplayName);
+      W.printHex("PtrParent", Proc.Header->PtrParent);
+      W.printHex("PtrEnd", Proc.Header->PtrEnd);
+      W.printHex("PtrNext", Proc.Header->PtrNext);
+      W.printHex("CodeSize", Proc.Header->CodeSize);
+      W.printHex("DbgStart", Proc.Header->DbgStart);
+      W.printHex("DbgEnd", Proc.Header->DbgEnd);
+      printTypeIndex("FunctionType", Proc.Header->FunctionType);
+      printRelocatedField("CodeOffset", Sec, SectionContents,
+                          &Proc.Header->CodeOffset, &LinkageName);
+      W.printHex("Segment", Proc.Header->Segment);
+      W.printFlags("Flags", static_cast<uint8_t>(Proc.Header->Flags),
+                   makeArrayRef(ProcSymFlagNames));
+      W.printString("DisplayName", Proc.Name);
       W.printString("LinkageName", LinkageName);
       break;
     }

 
     case S_INLINESITE: {
       DictScope S(W, "InlineSite");
-      const InlineSiteSym *InlineSite;
-      error(consumeObject(SymData, InlineSite));
-      W.printHex("PtrParent", InlineSite->PtrParent);
-      W.printHex("PtrEnd", InlineSite->PtrEnd);
-      printTypeIndex("Inlinee", InlineSite->Inlinee);
-
-      auto GetCompressedAnnotation = [&]() -> uint32_t {
-        if (SymData.empty())
-          return -1;
+      ArrayRef<uint8_t> Data(SymData.bytes_begin(), SymData.bytes_end());
+      auto InlineSiteOrError =
+          InlineSiteSym::deserialize(static_cast<SymbolRecordKind>(Kind), Data);
+      if (!InlineSiteOrError)
+        error(InlineSiteOrError.getError());
+      auto &InlineSite = InlineSiteOrError.get();
 
-        uint8_t FirstByte = SymData.front();
-        SymData = SymData.drop_front();
-
-        if ((FirstByte & 0x80) == 0x00)
-          return FirstByte;
-
-        if (SymData.empty())
-          return -1;
-
-        uint8_t SecondByte = SymData.front();
-        SymData = SymData.drop_front();
-
-        if ((FirstByte & 0xC0) == 0x80)
-          return ((FirstByte & 0x3F) << 8) | SecondByte;
-
-        if (SymData.empty())
-          return -1;
-
-        uint8_t ThirdByte = SymData.front();
-        SymData = SymData.drop_front();
-
-        if (SymData.empty())
-          return -1;
-
-        uint8_t FourthByte = SymData.front();
-        SymData = SymData.drop_front();
-
-        if ((FirstByte & 0xE0) == 0xC0)
-          return ((FirstByte & 0x1F) << 24) | (SecondByte << 16) |
-                 (ThirdByte << 8) | FourthByte;
-
-        return -1;
-      };
-      auto DecodeSignedOperand = [](uint32_t Operand) -> int32_t {
-        if (Operand & 1)
-          return -(Operand >> 1);
-        return Operand >> 1;
-      };
+      W.printHex("PtrParent", InlineSite.Header->PtrParent);
+      W.printHex("PtrEnd", InlineSite.Header->PtrEnd);
+      printTypeIndex("Inlinee", InlineSite.Header->Inlinee);
 
       ListScope BinaryAnnotations(W, "BinaryAnnotations");
-      while (!SymData.empty()) {
-        auto OpCode =
-            static_cast<BinaryAnnotationsOpCode>(GetCompressedAnnotation());
+      while (!InlineSite.Annotations.empty()) {
+        auto OpCode = static_cast<BinaryAnnotationsOpCode>(
+            InlineSite.GetCompressedAnnotation());
         switch (OpCode) {
         case BinaryAnnotationsOpCode::Invalid:
           return error(object_error::parse_failed);
         case BinaryAnnotationsOpCode::CodeOffset:
-          W.printHex("CodeOffset", GetCompressedAnnotation());
+          W.printHex("CodeOffset", InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeCodeOffsetBase:
-          W.printNumber("ChangeCodeOffsetBase", GetCompressedAnnotation());
+          W.printNumber("ChangeCodeOffsetBase",
+                        InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeCodeOffset:
-          W.printHex("ChangeCodeOffset", GetCompressedAnnotation());
+          W.printHex("ChangeCodeOffset", InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeCodeLength:
-          W.printHex("ChangeCodeLength", GetCompressedAnnotation());
+          W.printHex("ChangeCodeLength", InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeFile:
-          printFileNameForOffset("ChangeFile", GetCompressedAnnotation());
+          printFileNameForOffset("ChangeFile",
+                                 InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeLineOffset:
-          W.printNumber("ChangeLineOffset",
-                        DecodeSignedOperand(GetCompressedAnnotation()));
+          W.printNumber("ChangeLineOffset", InlineSite.DecodeSignedOperand());
           break;
         case BinaryAnnotationsOpCode::ChangeLineEndDelta:
-          W.printNumber("ChangeLineEndDelta", GetCompressedAnnotation());
+          W.printNumber("ChangeLineEndDelta",
+                        InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeRangeKind:
-          W.printNumber("ChangeRangeKind", GetCompressedAnnotation());
+          W.printNumber("ChangeRangeKind",
+                        InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeColumnStart:
-          W.printNumber("ChangeColumnStart", GetCompressedAnnotation());
+          W.printNumber("ChangeColumnStart",
+                        InlineSite.GetCompressedAnnotation());
           break;
         case BinaryAnnotationsOpCode::ChangeColumnEndDelta:
           W.printNumber("ChangeColumnEndDelta",
-                        DecodeSignedOperand(GetCompressedAnnotation()));
+                        InlineSite.DecodeSignedOperand());
           break;
         case BinaryAnnotationsOpCode::ChangeCodeOffsetAndLineOffset: {
-          uint32_t Annotation = GetCompressedAnnotation();
-          int32_t LineOffset = DecodeSignedOperand(Annotation >> 4);
+          uint32_t Annotation = InlineSite.GetCompressedAnnotation();
+          int32_t LineOffset = InlineSite.DecodeSignedOperand(Annotation >> 4);
           uint32_t CodeOffset = Annotation & 0xf;
           W.startLine() << "ChangeCodeOffsetAndLineOffset: {CodeOffset: "
                         << W.hex(CodeOffset) << ", LineOffset: " << LineOffset

           break;
         }
         case BinaryAnnotationsOpCode::ChangeCodeLengthAndCodeOffset: {
-          uint32_t Length = GetCompressedAnnotation();
-          uint32_t CodeOffset = GetCompressedAnnotation();
+          uint32_t Length = InlineSite.GetCompressedAnnotation();
+          uint32_t CodeOffset = InlineSite.GetCompressedAnnotation();
           W.startLine() << "ChangeCodeLengthAndCodeOffset: {CodeOffset: "
                         << W.hex(CodeOffset) << ", Length: " << W.hex(Length)
                         << "}\n";
           break;
         }
         case BinaryAnnotationsOpCode::ChangeColumnEnd:
-          W.printNumber("ChangeColumnEnd", GetCompressedAnnotation());
+          W.printNumber("ChangeColumnEnd",
+                        InlineSite.GetCompressedAnnotation());
           break;
         }
       }

 
     case S_LOCAL: {
       DictScope S(W, "Local");
-      const LocalSym *Local;
-      error(consumeObject(SymData, Local));
-      printTypeIndex("Type", Local->Type);
-      W.printFlags("Flags", uint16_t(Local->Flags), makeArrayRef(LocalFlags));
-      StringRef VarName = SymData.split('\0').first;
-      W.printString("VarName", VarName);
+      ArrayRef<uint8_t> Data(SymData.bytes_begin(), SymData.bytes_end());
+      auto LocalOrError =
+          LocalSym::deserialize(static_cast<SymbolRecordKind>(Kind), Data);
+      if (!LocalOrError)
+        error(LocalOrError.getError());
+      auto &Local = LocalOrError.get();
+
+      printTypeIndex("Type", Local.Header->Type);
+      W.printFlags("Flags", uint16_t(Local.Header->Flags),
+                   makeArrayRef(LocalFlags));
+      W.printString("VarName", Local.Name);
       break;
     }