[PATCH] D72828: [DWARF5] Added support for emission of debug_macro section.

Tue Feb 11 03:06:04 PST 2020

SouraVX added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/Dwarf.def:830
+// DWARF v5 Macro header flags
+HANDLE_MACRO_FLAGS(0x00, OFFSET_SIZE_32)
+HANDLE_MACRO_FLAGS(0x01, OFFSET_SIZE_64)
----------------
ikudrin wrote:
> Remove `OFFSET_SIZE_32`, as this value does not define any real flag.
Yes, theirs no particular flag in Spec. Spec says about "offset_size_flag" and how it's value is interpreted
0 - DWARF32 1 - DWARF64. I created these flags for readable/informative purpose.

================
Comment at: llvm/include/llvm/BinaryFormat/Dwarf.def:833
+HANDLE_MACRO_FLAGS(0x02, DEBUG_LINE_OFFSET)
+HANDLE_MACRO_FLAGS(0x03, OPCODE_OPERANDS_TABLE)

----------------
ikudrin wrote:
> The value for `OPCODE_OPERANDS_TABLE` should be `0x04`. It looks like @dblaikie already said that for one of previous versions of the patch.
Sorry for the trouble, I forgot to replace this with "0x03". Since, their was a discussion to keep this or not in first place because we are not emitting opcode_operands_table, So this flag is not used, at least right now.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2810
+  Asm->emitInt16(5);
+  assert(DwarfParams.getDwarfOffsetByteSize() == 4 && "DWARF64 not supported!");
+  // We are setting Offset and line offset flags unconditionally here,
----------------
ikudrin wrote:
> This crashes on a Debug build, probably because `DwarfParams` is not initialized. 
Oh.., Yeah I also noticed this, even after initalizing dwarf::FormParams DwarfParams({5, 8, DWARF32}).
Still 
`assert(DwarfParams.getDwarfOffsetByteSize() == 4 && "DWARF64 not supported!");`  not on release build.
this is failing, some how `getDwarfOffsetByteSize()` is not returning correctly.
Anyway I found this check to be a bit redundant in it's own way. So removed it.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72828/new/

https://reviews.llvm.org/D72828