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

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
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())
----------------
SouraVX wrote:
> 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.
If you're not supporting 64-bit offsets (which is entirely reasonable) you should have an assert() that the offset size is 4.


================
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
----------------
SouraVX wrote:
> 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.
@dblaikie The reference to .debug_line is required for DW_MACRO_start_file, because that opcode has an operand that is a file number from the line-number program's file table.  It's optional if that opcode is not used.

@SouraVX it is inconsistent to conditionalize setting the flag bit, and then unconditionally emit the debug_line_offset field.  I have no problem with always emitting the field, but in that case you should unconditionally set the flag bit as well.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2777
+    DEBUG_LINE_OFFSET = 2,
+    OPCODE_OPERANDS_TABLE = 3
+  };
----------------
DWARF does not define symbols for the flag bits, but I'd still expect to see these in Dwarf.def where they could be shared with the dumper code.
Also, OPCODE_OPERANDS_TABLE should be 4 not 3; these are masks, not bit numbers.


================
Comment at: llvm/test/DebugInfo/X86/stringpool.ll:43
+; DARWIN: .section __DWARF,__debug_str,regular,debug
+; DARWIN: .asciz "yyyy" ## string offset=[[YYYY:[0-9]+]]
 !6 = !{!0}
----------------
This captures the string offset into YYYY, but never uses the captured value.  The old version of the test did verify the offset was used, presumably somewhere in the __debug_info section.  So, this change has lost coverage.


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

https://reviews.llvm.org/D72828