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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 5 inline comments as done.
SouraVX added a comment.



In D72828#1824527 <https://reviews.llvm.org/D72828#1824527>, @dblaikie wrote:

> Please implement the dwarfdump support first (this patch can be reviewed during that time - but shouldn't be committed without tests & ideally those tests should be written using dwarfdump (sometimes there are exceptions for especially complicated dumping situations, where raw assembly testing is used instead, but I don't think it's necessary to make that tradeoff here))


I'm trying to get that patch ready. As for this, this patch doesn't have dependency on dwarfdump except that testing the section contents part.  I've verified integrity of section content using objdump and lldb/gdb. My rationale behind this was this patch can get in, till we discuss and agree on dwarfdump extension to accommodate macro support.

> & I'm surprised this patch causes updates to the tests - what changes in the patch caused changes in these tests? (stringpool and dbg-stack-value-range.mir) - ah, because debug_str now needs to be emitted after debug_macro because the macro section might introduce new strings? OK. Could you separate that ordering change/test update out into a standalone patch. It'll make the rest of this more targeted when reviewing.

Yes, that' s intentional. I've shifted the string emission to post macro emission to put macro content in string section. BTW are you suggesting here to do that change/patch commit separately ??

Thanks David for reviewing/sharing your thoughts on this.
Refactoring suggestion, I will address those in next revision.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2785-2786
+  Asm->emitInt16(5);
+  if (DwarfParams.getDwarfOffsetByteSize() == 4)
+    Flags |= OFFSET_SIZE_32;
+  if (Asm->getObjFileLowering().getDwarfLineSection())
----------------
dblaikie wrote:
> This looks to be backwards - since OFFSET_SIZE_32 is zero, these two lines don't have any effect (& 32 bit is the default - if the bit is zero).
> 
> So it should /probably/ be:
> 
> ```
> if (offset byte size == 8)
>   Flags |= OFFSET_SIZE_32;
> ```
I may be wrong here. Are you trying to suggest this ?? 
``` if (offset byte size == 8)
  Flags |= OFFSET_SIZE_64;```
I think we only need that in DWARF64 case, which this patch does not support.
So for consistency/readibility, I intentionally set `OFFSET_byte_size_32 `that even that has no effect.
Should we leave it as it is OR remove it and add comment that in default case DWARF32 Offset size will be 32.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2787-2788
+    Flags |= OFFSET_SIZE_32;
+  if (Asm->getObjFileLowering().getDwarfLineSection())
+    Flags |= DEBUG_LINE_OFFSET;
+  // FIXME: We are not setting opcode_operands_table_flag, since we're not
----------------
dblaikie wrote:
> Seems this line offset is optional and the DWARF spec doesn't mention what it might be useful for. Do you have any specific use in mind? Otherwise I'd leave it out. (@aprantl @probinson )
Yes, One crucial thing I noticed while verifying section content is that, consumers objdump/lldb/gdb agrees on this representation and expect offset of debug_line section within macro section. So if we omit that, I'm afraid we might loose debuggability.


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

https://reviews.llvm.org/D72828