[PATCH] D81145: [MC] Generate a compilation unit in the 64-bit DWARF format [3/7]

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin added inline comments.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:983
+  // The 4 (8 for DWARF64) byte offset to the debug abbrevs from the start of
+  // the .debug_abbrev, it is at the start of that section so this is zero.
   if (AbbrevSectionSymbol == nullptr)
----------------
jhenderson wrote:
> Probably should be a full stop instead of a comma on this line, though I'm not 100% sure I entirely follow the comment as a whole.
I've added a full stop. For my taste, it sounds OK, meaning that we know that we generate only one block of data, which is at the start of the section, so the real offset to that data is zero. Probably, Mach-O does not require a relocation for that case, so the raw value can be used.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:985-987
+    MCOS->emitIntValue(0, OffsetSize);
   else
+    MCOS->emitSymbolValue(AbbrevSectionSymbol, OffsetSize,
----------------
jhenderson wrote:
> There are two modified cases here, but only one test update for .debug_abbrev as far as I can see.  Does that indicate some missing coverage? Similar comment re. DW_AT_stmt_list below.
The code paths without symbols are for the Mach-O target. I have added a new test, but have to admit that I do not know much about that platform.


================
Comment at: llvm/test/MC/ELF/gen-dwarf64.s:9
 # REL:       Relocations [
+# REL:         Section ({{[0-9]+}}) .rela.debug_info {
+# REL-NEXT:      R_X86_64_64 .debug_abbrev 0x0
----------------
MaskRay wrote:
> `REL-NEXT:`
Well, the test does not intend to check the absence of other relocations, just that the expected relocations for particular sections are in place. I think I'd better remove the check for `Relocations [`.


================
Comment at: llvm/test/MC/ELF/gen-dwarf64.s:36-38
+    .section .foo, "ax", @progbits
+foo:
+    nop
----------------
jhenderson wrote:
> Why has this been added?
These lines trigger generating `.debug_info` and some related debug info sections.


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

https://reviews.llvm.org/D81145