[PATCH] D80382: [MC] Implement generating 64-bit debug info.

[Igor Kudrin via Phabricator via llvm-commits]

ikudrin marked 6 inline comments as done.
ikudrin added a comment.

In D80382#2050252 <https://reviews.llvm.org/D80382#2050252>, @dblaikie wrote:

> So this is DWARF64 support for debug_line in LLVM?


Not exactly. The patch supports all tables that `llvm-mc` can generate if I have not missed anything. And the support, for now, is enabled only in the tool itself.

> And the first support for DWARF64 emission in LLVM in general?

That is probably true, though I do not have deep knowledge of all LLVM tree to say for sure.

> Probably good to include that in the title - something like "Add initial (debug_line only) support for DWARF64" perhaps.

The patch aims to support all tables `llvm-mc` can generate in one shot, so that comment would not be exactly correct. Or maybe I am not following you.



================
Comment at: llvm/test/MC/ELF/gen-dwarf64.s:81-83
+// DUMP5:       .debug_rnglists contents:
+// DUMP5-NEXT:  0x00000000: range list header:
+// DUMP5-SAME:      format = DWARF64
----------------
dblaikie wrote:
> This concerns me slightly - looks like there was some latent/hidden/untested DWARF64 support in the rnglists that's just now become testable? So it might need some more testing if it's been so far untested? (similarly with any other sections with latent support - debug_info, debug_aranges, abbrev(? not sure if abbrev is different in 32/64))
There was no support for DWARF64 before this patch. As for `.debug_rnglists`, there are not many changes concerning this format, because we do not emit offsets for range lists, see `emitGenDwarfRanges()`.

I've extended the test adding some additional fields, but in general, I suppose that the produced tables are correct as long as `llvm-dwarfdump` can successfully parse them and report the format.


================
Comment at: llvm/tools/llvm-mc/llvm-mc.cpp:405
+    }
+    // If needsDwarfSectionOffsetDirective is true, we would eventually call
+    // MCStreamer::emitSymbolValue() with IsSectionRelative = true, but that
----------------
MaskRay wrote:
> dblaikie wrote:
> > Perhaps this should have a FIXME? and/or the code where the 4-byte only support is should have a FIXME? Are you planning to do that in a follow-up patch?
> > 
> > And is this error-path tested?
> Does it make sense to explicitly check for COFF?
This setting affects the emitting of 64-bit info directly, as it is directly passed to `MCStreamer::emitSymbolValue()`. Checking for targeting COFF would be an indirect condition, which potentially might become incorrect without noticing. I prefer a more robust condition.


================
Comment at: llvm/tools/llvm-mc/llvm-mc.cpp:405-414
+    // If needsDwarfSectionOffsetDirective is true, we would eventually call
+    // MCStreamer::emitSymbolValue() with IsSectionRelative = true, but that
+    // is supported only for 4-byte long references.
+    if (MAI->needsDwarfSectionOffsetDirective()) {
+      errs() << ProgName << ": the 64-bit DWARF format is not supported for "
+             << TheTriple.normalize() << ".\n";
+      return 1;
----------------
ikudrin wrote:
> MaskRay wrote:
> > dblaikie wrote:
> > > Perhaps this should have a FIXME? and/or the code where the 4-byte only support is should have a FIXME? Are you planning to do that in a follow-up patch?
> > > 
> > > And is this error-path tested?
> > Does it make sense to explicitly check for COFF?
> This setting affects the emitting of 64-bit info directly, as it is directly passed to `MCStreamer::emitSymbolValue()`. Checking for targeting COFF would be an indirect condition, which potentially might become incorrect without noticing. I prefer a more robust condition.
Not sure if anyone would struggle to support DWARF64 on 32-bit targets. That would be tricky without 64-bit relocations. Personally, I do not have such plans.

All three cases have tests, see `COFF/dwarf64-err.s` and `ELF/dwarf64-err.s`.


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

https://reviews.llvm.org/D80382