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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM.



================
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)
----------------
dblaikie wrote:
> ikudrin wrote:
> > 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.
> 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."


================
Comment at: llvm/lib/MC/MCDwarf.cpp:997-998
 
-  // DW_AT_stmt_list, a 4 byte offset from the start of the .debug_line section,
-  // which is at the start of that section so this is zero.
+  // DW_AT_stmt_list, a 4 (8 for DWARF64) byte offset from the start of the
+  // .debug_line section, which is at the start of that section so this is zero.
   if (LineSectionSymbol)
----------------
jhenderson wrote:
> Again, this comment probably needs a bit of improvement.
Approximately same suggestion here, if updating the comment again: "The line table is at the start of that section, so the offset is zero."


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

https://reviews.llvm.org/D81145