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.
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.
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.
Again, this comment probably needs a bit of improvement.
Why has this been added?
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.
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.
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.
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.
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 [.
These lines trigger generating .debug_info and some related debug info sections.
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.
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."
Approximately same suggestion here, if updating the comment again: "The line table is at the start of that section, so the offset is zero."
- Rearranged branches in if (AbbrevSectionSymbol) to unify it with other similar cases.
- Updated comments about emitting zero values.
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?