[codeview] Improve readability of type record assembly

[rnk]

Summary

Adds the method MCStreamer::EmitBinaryData, which is usually an alias
for EmitBytes. In the MCAsmStreamer case, it is overridden to emit hex
dump output like this:

.byte   0x0e, 0x00, 0x08, 0x10
.byte   0x03, 0x00, 0x00, 0x00
.byte   0x00, 0x00, 0x00, 0x00
.byte   0x00, 0x10, 0x00, 0x00

Also, when verbose asm comments are enabled, this patch prints the dump
output for each comment before its record, like this:

1. ArgList (0x1000) {
2. TypeLeafKind: LF_ARGLIST (0x1201)
3. NumArgs: 0
4. Arguments [
5. ]
6. } .byte 0x06, 0x00, 0x01, 0x12 .byte 0x00, 0x00, 0x00, 0x00

This should make debugging easier and testing more convenient.

[aaboud]

Looks good to me.
Only few comments below.

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

290 ↗

(On Diff #58716)

Can you explain what we need the CommentPrefix string?
We are not going to print it after all, right?

lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp

26 ↗

(On Diff #58716)

How about defining two "static const" variables? Like this:
static const int Alignment = 4;
static const int SizeFieldLength = 2;

31 ↗

(On Diff #58716)

The Padding is created in the wrong order (check how it is emitted in CodeViewDebug.cpp lines 299-300.
This should be like this:

for (int I = TotalSize - (Data.size() + 2) - 1; I >= 0; --I)
  Mem[I] = LF_PAD0 + I;

lib/MC/MCAsmStreamer.cpp

707 ↗

(On Diff #58716)

Should not "Cols" be "static const"?

[aaboud]

lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp

31 ↗

(On Diff #58716)

It seems that LF_PAD0 should not be used in padding so the correct loop will look like this:

for (int I = TotalSize - (Data.size() + 2) ; I > 0; --I)
  Mem[I] = LF_PAD0 + I;

[rnk]

Thanks!

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

290 ↗

(On Diff #58716)

The comment block is multiple lines, and we only chop off the comment prefix of the first line.

lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp

31 ↗

(On Diff #58716)

I'm pretty sure this loop is correct, because it passes the tests in test/DebugInfo/COFF/inlining-padding.ll for this. We never emit LF_PAD0, because we exit the loop before I reaches TotalSize.

I thought it was easier to reason about a count-up loop than a count-down loop, so I went with this.

lib/MC/MCAsmStreamer.cpp

707 ↗

(On Diff #58716)

I could const-ify it, but I don't generally like making readers think about whether or not local variables require static initialization.

llvm/trunk/include/llvm/DebugInfo/CodeView/MemoryTypeTableBuilder.h

 //===- MemoryTypeTableBuilder.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_MEMORYTYPETABLEBUILDER_H
 #define LLVM_DEBUGINFO_CODEVIEW_MEMORYTYPETABLEBUILDER_H
 
-#include "llvm/ADT/Hashing.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/DebugInfo/CodeView/TypeTableBuilder.h"
-#include <functional>
-#include <memory>
-#include <unordered_map>
 #include <vector>
 
 namespace llvm {
 namespace codeview {
 
 class MemoryTypeTableBuilder : public TypeTableBuilder {
 public:
-  class Record {
-  public:
-    explicit Record(llvm::StringRef RData);
-
-    const char *data() const { return Data.get(); }
-    uint16_t size() const { return Size; }
-
-  private:
-    uint16_t Size;
-    std::unique_ptr<char[]> Data;
-  };
-
-private:
-  class RecordHash : std::unary_function<llvm::StringRef, size_t> {
-  public:
-    size_t operator()(llvm::StringRef Val) const {
-      return static_cast<size_t>(llvm::hash_value(Val));
-    }
-  };
-
-public:
   MemoryTypeTableBuilder() {}
 
   bool empty() const { return Records.empty(); }
 
   template <typename TFunc> void ForEachRecord(TFunc Func) {
     uint32_t Index = TypeIndex::FirstNonSimpleIndex;
 
-    for (const std::unique_ptr<Record> &R : Records) {
-      Func(TypeIndex(Index), R.get());
+    for (StringRef R : Records) {
+      Func(TypeIndex(Index), R);
       ++Index;
     }
   }
 
 protected:
   TypeIndex writeRecord(llvm::StringRef Data) override;
 
 private:
-  std::vector<std::unique_ptr<Record>> Records;
-  std::unordered_map<llvm::StringRef, TypeIndex, RecordHash> HashedRecords;
+  std::vector<StringRef> Records;
+  BumpPtrAllocator RecordStorage;
+  DenseMap<StringRef, TypeIndex> HashedRecords;
 };
 
 } // end namespace codeview
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_CODEVIEW_MEMORYTYPETABLEBUILDER_H

llvm/trunk/include/llvm/DebugInfo/CodeView/TypeDumper.h

 namespace llvm {
 class ScopedPrinter;
 
 namespace codeview {
 
 /// Dumper for CodeView type streams found in COFF object files and PDB files.
 class CVTypeDumper {
 public:
-  CVTypeDumper(ScopedPrinter &W, bool PrintRecordBytes)
-      : W(&W), PrintRecordBytes(PrintRecordBytes) {}
+  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.

llvm/trunk/include/llvm/MC/MCStreamer.h

public:
   /// @{
 
   /// \brief Emit the bytes in \p Data into the output.
   ///
   /// This is used to implement assembler directives such as .byte, .ascii,
   /// etc.
   virtual void EmitBytes(StringRef Data);
 
+  /// Functionally identical to EmitBytes. When emitting textual assembly, this
+  /// method uses .byte directives instead of .ascii or .asciz for readability.
+  virtual void EmitBinaryData(StringRef Data);
+
   /// \brief Emit the expression \p Value into the output as a native
   /// integer of the given \p Size bytes.
   ///
   /// This is used to implement assembler directives such as .word, .quad,
   /// etc.
   ///
   /// \param Value - The value to emit.
   /// \param Size - The size of the integer (in bytes) to emit. This must

llvm/trunk/include/llvm/Support/ScopedPrinter.h

public:
   void flush() { OS.flush(); }
 
   void indent(int Levels = 1) { IndentLevel += Levels; }
 
   void unindent(int Levels = 1) {
     IndentLevel = std::max(0, IndentLevel - Levels);
   }
 
+  void resetIndent() { IndentLevel = 0; }
+
+  void setPrefix(StringRef P) { Prefix = P; }
+
   void printIndent() {
+    OS << Prefix;
     for (int i = 0; i < IndentLevel; ++i)
       OS << "  ";
   }
 
   template <typename T> HexNumber hex(T Value) { return HexNumber(Value); }
 
   template <typename T, typename TEnum>
   void printEnum(StringRef Label, T Value,
static bool flagName(const EnumEntry<T> &lhs, const EnumEntry<T> &rhs) {
     return lhs.Name < rhs.Name;
   }
 
   void printBinaryImpl(StringRef Label, StringRef Str, ArrayRef<uint8_t> Value,
                        bool Block);
 
   raw_ostream &OS;
   int IndentLevel;
+  StringRef Prefix;
 };
 
 template <>
 inline void
 ScopedPrinter::printHex<support::ulittle16_t>(StringRef Label,
                                               support::ulittle16_t Value) {
   startLine() << Label << ": " << hex(Value) << "\n";
 }

llvm/trunk/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

 // This file contains support for writing Microsoft CodeView debug info.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeViewDebug.h"
 #include "llvm/DebugInfo/CodeView/CodeView.h"
 #include "llvm/DebugInfo/CodeView/Line.h"
 #include "llvm/DebugInfo/CodeView/SymbolRecord.h"
+#include "llvm/DebugInfo/CodeView/TypeDumper.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 #include "llvm/DebugInfo/CodeView/TypeRecord.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCSectionCOFF.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/COFF.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 #include "llvm/Target/TargetRegisterInfo.h"
 #include "llvm/Target/TargetFrameLowering.h"
 
 using namespace llvm;
 using namespace llvm::codeview;
 
 CodeViewDebug::CodeViewDebug(AsmPrinter *AP)
if (!CU_Nodes)
     return;
   if (TypeTable.empty())
     return;
 
   // Start the .debug$T section with 0x4.
   OS.SwitchSection(Asm->getObjFileLowering().getCOFFDebugTypesSection());
   emitCodeViewMagicVersion();
 
+  SmallString<8> CommentPrefix;
+  if (OS.isVerboseAsm()) {
+    CommentPrefix += '\t';
+    CommentPrefix += Asm->MAI->getCommentString();
+    CommentPrefix += ' ';
+  }
+
+  CVTypeDumper CVTD(nullptr, /*PrintRecordBytes=*/false);
   TypeTable.ForEachRecord(
-      [&](TypeIndex Index, const MemoryTypeTableBuilder::Record *R) {
-        // Each record should be 4 byte aligned. We achieve that by emitting
-        // LF_PAD padding bytes. The on-disk record size includes the padding
-        // bytes so that consumers don't have to skip past them.
-        uint64_t RecordSize = R->size() + 2;
-        uint64_t AlignedSize = alignTo(RecordSize, 4);
-        uint64_t AlignedRecordSize = AlignedSize - 2;
-        assert(AlignedRecordSize < (1 << 16) && "type record size overflow");
-        OS.AddComment("Type record length");
-        OS.EmitIntValue(AlignedRecordSize, 2);
-        OS.AddComment("Type record data");
-        OS.EmitBytes(StringRef(R->data(), R->size()));
-        // Pad the record with LF_PAD bytes.
-        for (unsigned I = AlignedSize - RecordSize; I > 0; --I)
-          OS.EmitIntValue(LF_PAD0 + I, 1);
+      [&](TypeIndex Index, StringRef Record) {
+        if (OS.isVerboseAsm()) {
+          // Emit a block comment describing the type record for readability.
+          SmallString<512> CommentBlock;
+          raw_svector_ostream CommentOS(CommentBlock);
+          ScopedPrinter SP(CommentOS);
+          SP.setPrefix(CommentPrefix);
+          CVTD.setPrinter(&SP);
+          bool DumpSuccess =
+              CVTD.dump({Record.bytes_begin(), Record.bytes_end()});
+          (void)DumpSuccess;
+          assert(DumpSuccess && "produced malformed type record");
+          // emitRawComment will insert its own tab and comment string before
+          // the first line, so strip off our first one. It also prints its own
+          // newline.
+          OS.emitRawComment(
+              CommentOS.str().drop_front(CommentPrefix.size() - 1).rtrim());
+        }
+        OS.EmitBinaryData(Record);
       });
 }
 
 void CodeViewDebug::emitInlineeLinesSubsection() {
   if (InlinedSubprograms.empty())
     return;
 
   // Use the generic .debug$S section.

llvm/trunk/lib/DebugInfo/CodeView/MemoryTypeTableBuilder.cpp

 //===-- MemoryTypeTableBuilder.cpp ----------------------------------------===//
 //
 //                     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/MemoryTypeTableBuilder.h"
 #include "llvm/DebugInfo/CodeView/TypeIndex.h"
 
 using namespace llvm;
 using namespace codeview;
 
-MemoryTypeTableBuilder::Record::Record(StringRef RData)
-    : Size(RData.size()), Data(new char[RData.size()]) {
-  memcpy(Data.get(), RData.data(), RData.size());
-}
-
 TypeIndex MemoryTypeTableBuilder::writeRecord(StringRef Data) {
+  assert(Data.size() <= UINT16_MAX);
   auto I = HashedRecords.find(Data);
   if (I != HashedRecords.end()) {
     return I->second;
   }
 
-  std::unique_ptr<Record> R(new Record(Data));
+  // The record provided by the user lacks the 2 byte size field prefix and is
+  // not padded to 4 bytes. Ultimately, that is what gets emitted in the object
+  // file, so pad it out now.
+  const int SizeOfRecLen = 2;
+  const int Align = 4;
+  int TotalSize = alignTo(Data.size() + SizeOfRecLen, Align);
+  assert(TotalSize - SizeOfRecLen <= UINT16_MAX);
+  char *Mem =
+      reinterpret_cast<char *>(RecordStorage.Allocate(TotalSize, Align));
+  *reinterpret_cast<ulittle16_t *>(Mem) = uint16_t(TotalSize - SizeOfRecLen);
+  memcpy(Mem + SizeOfRecLen, Data.data(), Data.size());
+  for (int I = Data.size() + SizeOfRecLen; I < TotalSize; ++I)
+    Mem[I] = LF_PAD0 + (TotalSize - I);
 
   TypeIndex TI(static_cast<uint32_t>(Records.size()) +
                TypeIndex::FirstNonSimpleIndex);
-  HashedRecords.insert(std::make_pair(StringRef(R->data(), R->size()), TI));
-  Records.push_back(std::move(R));
+
+  // Use only the data supplied by the user as a key to the hash table, so that
+  // future lookups will succeed.
+  HashedRecords.insert(std::make_pair(StringRef(Mem + SizeOfRecLen, Data.size()), TI));
+  Records.push_back(StringRef(Mem, TotalSize));
 
   return TI;
 }

llvm/trunk/lib/MC/MCAsmStreamer.cpp

void EmitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                              unsigned ByteAlignment) override;
 
   void EmitZerofill(MCSection *Section, MCSymbol *Symbol = nullptr,
                     uint64_t Size = 0, unsigned ByteAlignment = 0) override;
 
   void EmitTBSSSymbol(MCSection *Section, MCSymbol *Symbol, uint64_t Size,
                       unsigned ByteAlignment = 0) override;
 
+  void EmitBinaryData(StringRef Data) override;
+
   void EmitBytes(StringRef Data) override;
 
   void EmitValueImpl(const MCExpr *Value, unsigned Size,
                      SMLoc Loc = SMLoc()) override;
   void EmitIntValue(uint64_t Value, unsigned Size) override;
 
   void EmitULEB128Value(const MCExpr *Value) override;
 
void MCAsmStreamer::EmitBytes(StringRef Data) {
   } else {
     OS << MAI->getAsciiDirective();
   }
 
   PrintQuotedString(Data, OS);
   EmitEOL();
 }
 
+void MCAsmStreamer::EmitBinaryData(StringRef Data) {
+  // This is binary data. Print it in a grid of hex bytes for readability.
+  const size_t Cols = 4;
+  for (size_t I = 0, EI = alignTo(Data.size(), Cols); I < EI; I += Cols) {
+    size_t J = I, EJ = std::min(I + Cols, Data.size());
+    assert(EJ > 0);
+    OS << MAI->getData8bitsDirective();
+    for (; J < EJ - 1; ++J)
+      OS << format("0x%02x", uint8_t(Data[J])) << ", ";
+    OS << format("0x%02x", uint8_t(Data[J]));
+    EmitEOL();
+  }
+}
+
 void MCAsmStreamer::EmitIntValue(uint64_t Value, unsigned Size) {
   EmitValue(MCConstantExpr::create(Value, getContext()), Size);
 }
 
 void MCAsmStreamer::EmitValueImpl(const MCExpr *Value, unsigned Size,
                                   SMLoc Loc) {
   assert(Size <= 8 && "Invalid size");
   assert(getCurrentSection().first &&

llvm/trunk/lib/MC/MCStreamer.cpp

 void MCStreamer::emitELFSize(MCSymbolELF *Symbol, const MCExpr *Value) {}
 void MCStreamer::EmitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                        unsigned ByteAlignment) {}
 void MCStreamer::EmitTBSSSymbol(MCSection *Section, MCSymbol *Symbol,
                                 uint64_t Size, unsigned ByteAlignment) {}
 void MCStreamer::ChangeSection(MCSection *, const MCExpr *) {}
 void MCStreamer::EmitWeakReference(MCSymbol *Alias, const MCSymbol *Symbol) {}
 void MCStreamer::EmitBytes(StringRef Data) {}
+void MCStreamer::EmitBinaryData(StringRef Data) { EmitBytes(Data); }
 void MCStreamer::EmitValueImpl(const MCExpr *Value, unsigned Size, SMLoc Loc) {
   visitUsedExpr(*Value);
 }
 void MCStreamer::EmitULEB128Value(const MCExpr *Value) {}
 void MCStreamer::EmitSLEB128Value(const MCExpr *Value) {}
 void MCStreamer::emitFill(const MCExpr &NumBytes, uint64_t Value, SMLoc Loc) {}
 void MCStreamer::emitFill(const MCExpr &NumValues, int64_t Size, int64_t Expr,
                           SMLoc Loc) {}

llvm/trunk/test/DebugInfo/COFF/inlining.ll

 ; ASM: .cv_loc 2 1 6 5                 # t.cpp:6:5
 ; ASM: addl    $3, "?x@@3HC"
 ; ASM: .cv_loc 1 1 11 5                # t.cpp:11:5
 ; ASM: addl    $5, "?x@@3HC"
 ; ASM: .cv_loc 0 1 16 5                # t.cpp:16:5
 ; ASM: addl    $7, "?x@@3HC"
 ; ASM: .cv_loc 0 1 17 1                # t.cpp:17:1
 
+; ASM: .section .debug$T,"dr"
+; ASM: .long 4 # Debug section magic
+; ASM: # ArgList (0x1000) {
+; ASM: #   TypeLeafKind: LF_ARGLIST (0x1201)
+; ASM: #   NumArgs: 0
+; ASM: #   Arguments [
+; ASM: #   ]
+; ASM: # }
+; ASM: .byte   0x06, 0x00, 0x01, 0x12
+; ASM: .byte   0x00, 0x00, 0x00, 0x00
+; ASM: # Procedure (0x1001) {
+; ASM: #   TypeLeafKind: LF_PROCEDURE (0x1008)
+; ASM: #   ReturnType: void (0x3)
+; ASM: #   CallingConvention: NearC (0x0)
+; ASM: #   FunctionOptions [ (0x0)
+; ASM: #   ]
+; ASM: #   NumParameters: 0
+; ASM: #   ArgListType: () (0x1000)
+; ASM: # }
+; ASM: .byte   0x0e, 0x00, 0x08, 0x10
+; ASM: .byte   0x03, 0x00, 0x00, 0x00
+; ASM: .byte   0x00, 0x00, 0x00, 0x00
+; ASM: .byte   0x00, 0x10, 0x00, 0x00
+; ASM: # FuncId (0x1002) {
+; ASM: #   TypeLeafKind: LF_FUNC_ID (0x1601)
+; ASM: #   ParentScope: 0x0
+; ASM: #   FunctionType: void () (0x1001)
+; ASM: #   Name: bar
+; ASM: # }
+; ASM: .byte   0x0e, 0x00, 0x01, 0x16
+; ASM: .byte   0x00, 0x00, 0x00, 0x00
+; ASM: .byte   0x01, 0x10, 0x00, 0x00
+; ASM: .byte   0x62, 0x61, 0x72, 0x00
+; ASM: # FuncId (0x1003) {
+; ASM: #   TypeLeafKind: LF_FUNC_ID (0x1601)
+; ASM: #   ParentScope: 0x0
+; ASM: #   FunctionType: void () (0x1001)
+; ASM: #   Name: foo
+; ASM: # }
+; ASM: .byte   0x0e, 0x00, 0x01, 0x16
+; ASM: .byte   0x00, 0x00, 0x00, 0x00
+; ASM: .byte   0x01, 0x10, 0x00, 0x00
+; ASM: .byte   0x66, 0x6f, 0x6f, 0x00
+
 ; ASM: .section .debug$S,"dr"
 ; ASM: .long   246                     # Inlinee lines subsection
 ; ASM: .long   [[inline_end:.*]]-[[inline_beg:.*]] #
 ; ASM: [[inline_beg]]:
 ; ASM: .long   0
 ; ASM: # Inlined function bar starts at t.cpp:8
 ; ASM: .long   4098                    # Type index of inlined function
 ; ASM: .long   0                       # Offset into filechecksum table

llvm/trunk/tools/llvm-pdbdump/llvm-pdbdump.cpp

if (auto EC = dumpStreamData(P, File))
     return EC;
 
   if (auto EC = dumpInfoStream(P, File))
     return EC;
 
   if (auto EC = dumpNamedStream(P, File))
     return EC;
 
-  codeview::CVTypeDumper TD(P, false);
+  codeview::CVTypeDumper TD(&P, false);
   if (auto EC = dumpTpiStream(P, File, TD, StreamTPI))
     return EC;
   if (auto EC = dumpTpiStream(P, File, TD, StreamIPI))
     return EC;
 
   if (auto EC = dumpDbiStream(P, File, TD))
     return EC;
 

llvm/trunk/tools/llvm-readobj/COFFDumper.cpp

 
 namespace {
 
 class COFFDumper : public ObjDumper {
 public:
   friend class COFFObjectDumpDelegate;
   COFFDumper(const llvm::object::COFFObjectFile *Obj, ScopedPrinter &Writer)
       : ObjDumper(Writer), Obj(Obj),
-        CVTD(Writer, opts::CodeViewSubsectionBytes) {}
+        CVTD(&Writer, opts::CodeViewSubsectionBytes) {}
 
   void printFileHeaders() override;
   void printSections() override;
   void printRelocations() override;
   void printSymbols() override;
   void printDynamicSymbols() override;
   void printUnwindInfo() override;
   void printCOFFImports() override;
prettyPrintStackMap(llvm::outs(),
                         StackMapV1Parser<support::big>(StackMapContentsArray));
 }
 
 void llvm::dumpCodeViewMergedTypes(
     ScopedPrinter &Writer, llvm::codeview::MemoryTypeTableBuilder &CVTypes) {
   // Flatten it first, then run our dumper on it.
   ListScope S(Writer, "MergedTypeStream");
   SmallString<0> Buf;
-  CVTypes.ForEachRecord([&](TypeIndex TI, MemoryTypeTableBuilder::Record *R) {
-    // The record data doesn't include the 16 bit size.
-    Buf.push_back(R->size() & 0xff);
-    Buf.push_back((R->size() >> 8) & 0xff);
-    Buf.append(R->data(), R->data() + R->size());
+  CVTypes.ForEachRecord([&](TypeIndex TI, StringRef Record) {
+    Buf.append(Record.begin(), Record.end());
   });
-  CVTypeDumper CVTD(Writer, opts::CodeViewSubsectionBytes);
+  CVTypeDumper CVTD(&Writer, opts::CodeViewSubsectionBytes);
   if (!CVTD.dump({Buf.str().bytes_begin(), Buf.str().bytes_end()})) {
     Writer.flush();
     error(object_error::parse_failed);
   }
 }