[PATCH] D75375: [MCDwarf] Generate DWARF v5 .debug_rnglists for assembly files

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1120-1124
+    MCSymbol *StartSymbol =
+        context.createTempSymbol("debug_rnglists_start", true, true);
+    MCSymbol *EndSymbol =
+        context.createTempSymbol("debug_rnglists_end", true, true);
+    mcdwarf::emitListsTableHeaderStart(MCOS, StartSymbol, EndSymbol);
----------------
MaskRay wrote:
> dblaikie wrote:
> > MaskRay wrote:
> > > dblaikie wrote:
> > > > Might be worth considering rolling the start/end label creation into this utility function & having this function return the EndSymbol for later use?
> > > > 
> > > > (some of the other "emit a header with the usual length field" implementations in AsmPrinter use this idiom - the function creates both labels, returns the end label for the outer code to emit at the end of the contents)
> > > ```
> > > // https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L2425
> > > static MCSymbol *emitLoclistsTableHeader(AsmPrinter *Asm,
> > >                                          const DwarfDebug &DD) {
> > >   MCSymbol *TableStart = Asm->createTempSymbol("debug_loclist_table_start");
> > >   MCSymbol *TableEnd = Asm->createTempSymbol("debug_loclist_table_end");
> > >   mcdwarf::emitListsTableHeaderStart(*Asm->OutStreamer, TableStart, TableEnd);
> > > ```
> > > 
> > > The temporary labels may have a different name (loclist). Shall I add another parameter `SymbolName` accepting either `debug_loclists_header` or `debug_rnglists_header`?
> > I'm undecided/your choice - I wonder if it might be reasonable to not worry about giving them such distinct names? let them be debug_list_header0, debug_list_header1, etc? I don't mind too much either way. Adding the list type string, or not. I guess it's probably nice to add it.
> In D75568, I moved TableStart/TableEnd generation into emitListsTableHeaderStart().  PTAL~
> 
> It seems we don't have a test checking -filetype=asm output, so the name is not significant.
Yeah, I wouldn't expect we'd test the asm output - but we still like to keep it readable and editable (eg: the names of labels, the text of comments, the use of labels rather than precalculating offsets (sometimes - we haven't done that completely, but it might be worthwhile to make for highly editable DWARF)). Having the more specific names /could/ be nice - but like I said above, I don't think it's super important & should still be quite legible/editable as-is. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75375