[PATCH] D16292: Improved Macro emission in dwarf

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:627
@@ +626,3 @@
+      U.addLabelDelta(U.getUnitDie(), dwarf::DW_AT_macro_info, CurrLabel,
+                      PrevLabel, dwarf::DW_FORM_sec_offset);
+    PrevLabel = CurrLabel;
----------------
This seems wrong - a delta isn't a section offset. Once this DWARF is relocated (eg: linked with another object file containing DWARF) these deltas would become incorrect.

A label delta is constant, not a dynamic/link time relocation - representing the distance between two labels. That doesn't change when the debug info is linked and multiple macinfo sections are concatenated together.

But DWARF requires the sec_offset to represent the final offset within the section of the linked debug info. So I assume, much like DW_AT_stmt_list and DW_AT_ranges. Like those two attributes, I assume this one should be emitted using "addSectionOffset" to ensure it has the right kind of relocation, etc. (& on MachO it doesn't use a reloc - addSectionOffset handles that case too)



================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:211
@@ +210,3 @@
+  /// Maps DwarfCompileUnit with label emitted before macro section entry.
+  DenseMap<const DwarfCompileUnit *, MCSymbol *> LabelsBeforeMSE;
+
----------------
Rather than adding a map, the MCSymbol should probably just be part of the DwarfCompileUnit itself?

(you can check DwarfCompileUnit::attachRangesOrLowHighPC, used to add DW_AT_ranges to the DW_TAG_compile_unit (& elsewhere) for some inspiration - though it won't match that exactly (that one builds a range list, keeps a label for it, attaches a reloc to that label, later on emits the range list with the label - some of that should be similar to what you need/want, without the need to build an intermediate data structure like the range list))


http://reviews.llvm.org/D16292