[PATCH] D20435: [codeview] Adding support for CodeView types

[Reid Kleckner via llvm-commits]

rnk added a comment.

Thanks Amjad! Adrian Mccarthy, who I added to the review, was also just getting started on this approach. At a high level, I'd like to keep as much CV serialization logic in lib/DebugInfo/CodeView as possible.

My thinking was that for every CV type record, we'd serialize the record in memory using MemoryTypeTableBuilder as we encounter DITypes. After translation, for each record we would call some new virtual MCStreamer method like EmitCVType. In MCObjectStreamer, this would delegate directly to EmitBytes. In MCAsmStreamer, this would do more work to try to make the output pretty. My initial idea is that it would emit each record as a sequence of 32-bit hex integers preceded by a big block comment generated by the same type dumper that llvm-readobj and llvm-pdbdump use.

Thoughts?


================
Comment at: include/llvm/DebugInfo/CodeView/TypeRecord.h:880
@@ -879,1 +879,3 @@
 
+  static uint32_t getLayoutSize() { return 2 + sizeof(Layout); }
+
----------------
zturner wrote:
> This looks wrong to me.  It does not include the length of the name.  Also, I would prefer if the `2 + ` were raised somehow, because it is common to every single type of CodeView record, and this logic of knowing that a type record has 2 header bytes should be common somewhere.
> 
> Also, each type record knows how to deserialize itself (see the `deserialize` method above), should we centralize the serialization logic here as well using a similar pattern?
We won't need to know the layout size if we use MemoryTypeTableBuilder or something like it.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1095
@@ +1094,3 @@
+void CodeViewDebug::layout() {
+  // Assign unique indexes to each type in the type table.
+  //
----------------
Why not assign type indices as we lower types, and then maintain a mapping from DIType* to TypeIndex? That seems like it would be considerably more efficient for C++ programs with lots of types.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1181
@@ +1180,3 @@
+
+void CodeViewDebug::emitTypeFunctionID(const CodeViewTypeFunctionID *CVType) {
+  StringRef Name = CVType->getName();
----------------
I actually wanted to move away from serializing type records using the MCStreamer interface, because it completely duplicates the CV type record layout. Instead I was suggesting to Adrian Mccarthy that we use MemoryTypeTableBuilder.

We can preserve the pretty assembly printing functionality by calling the dumper on our serialized CV types and printing that as a comment before every binary record. If we do that, we should emit CV records as a series of 32-hex bit words instead of using "EmitBytes", which uses the .ascii directive, which is useless when trying to read the assembly.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewType.h:1
@@ +1,2 @@
+//===-- llvm/lib/CodeGen/AsmPrinter/CodeViewType.h ----*- C++ -*--===//
+//
----------------
Is there a reason you can't modify TypeRecord.h to make its types suitable for this?


http://reviews.llvm.org/D20435