Page MenuHomePhabricator

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

Authored by ikudrin on Jun 4 2020, 5:00 AM.

Details

Summary

The patch enables producing DWARF64 compilation units and fixes generating references to .debug_abbrev and .debug_line sections. A similar change for .debug_ranges/.debug_rnglists will be added in a forthcoming patch.

Diff Detail

Event Timeline

ikudrin created this revision.Jun 4 2020, 5:00 AM
jhenderson added inline comments.Jun 4 2020, 5:51 AM
llvm/lib/MC/MCDwarf.cpp
981–985

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.

982

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.

998–1000

Again, this comment probably needs a bit of improvement.

llvm/test/MC/ELF/gen-dwarf64.s
36–38

Why has this been added?

MaskRay added inline comments.Jun 4 2020, 11:26 AM
llvm/test/MC/ELF/gen-dwarf64.s
8

REL-NEXT:

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

Might be best to keep the fundamental design direction/question here in one thread ( https://reviews.llvm.org/D81144 ) rather than in all of them. Patches are in a series, so if that one ends up going another direction I don't think there's a risk these later ones will be reviewed/approved/committed in spite of that fundamental question.

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

Might be best to keep the fundamental design direction/question here in one thread ( https://reviews.llvm.org/D81144 ) rather than in all of them. Patches are in a series, so if that one ends up going another direction I don't think there's a risk these later ones will be reviewed/approved/committed in spite of that fundamental question.

Okay, but is the idea that this patch handles both asm source and the normal case? Because I don't think it does.

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

Might be best to keep the fundamental design direction/question here in one thread ( https://reviews.llvm.org/D81144 ) rather than in all of them. Patches are in a series, so if that one ends up going another direction I don't think there's a risk these later ones will be reviewed/approved/committed in spite of that fundamental question.

Okay, but is the idea that this patch handles both asm source and the normal case? Because I don't think it does.

Nope, the LLVM IR support should be in a separate patch - that'll be in llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp - llvm::DwarfUnit::emitCommonHeader, for instance. And other support through lib/CodeGen/AsmPrinter for the form encoding length, etc.

ikudrin updated this revision to Diff 268810.Jun 5 2020, 8:24 AM
ikudrin marked 8 inline comments as done.
  • Fixed comments.
  • Added a check for a DIE for "foo" in the test to validate that the CU is not skewed.
  • Added a test targeting Mach-O, so that all changed code paths are covered.
ikudrin added inline comments.Jun 5 2020, 8:32 AM
llvm/lib/MC/MCDwarf.cpp
981–985

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.

982

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.

llvm/test/MC/ELF/gen-dwarf64.s
8

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 [.

36–38

These lines trigger generating .debug_info and some related debug info sections.

dblaikie added inline comments.Jun 5 2020, 9:36 AM
llvm/lib/MC/MCDwarf.cpp
982

Yeah, it's still a bit confusing to me too. The ambiguities in the second sentence I think are what I'm tripping over - it is unclear to me what "it" and "this" are in that context.

But maybe out of scope for this patch - if someone comes up with a good phrasing & wants to replace it - just commit that directly/separately.

jhenderson accepted this revision.Jun 8 2020, 12:26 AM

LGTM.

llvm/lib/MC/MCDwarf.cpp
982

I'll leave this up to @ikudrin whether he includes it or not. My repo is a little bit of a mess at the moment, so can't easily add it myself. If someone does want to improve the wording, I'd suggest "Since the abbrevs are at the start of that section, the offset is zero."

998–1000

Approximately same suggestion here, if updating the comment again: "The line table is at the start of that section, so the offset is zero."

This revision is now accepted and ready to land.Jun 8 2020, 12:26 AM
ikudrin updated this revision to Diff 269151.Jun 8 2020, 3:31 AM
ikudrin marked 3 inline comments as done.
  • Rearranged branches in if (AbbrevSectionSymbol) to unify it with other similar cases.
  • Updated comments about emitting zero values.
llvm/lib/MC/MCDwarf.cpp
982

Thanks for the suggestion! I've used your wording but changed "that section" to "the section" because it is now a separate comment. How does it look now?

jhenderson accepted this revision.Jun 8 2020, 3:49 AM
jhenderson added inline comments.
llvm/lib/MC/MCDwarf.cpp
982

Works for me, thanks.

This revision was automatically updated to reflect the committed changes.