[PATCH] D75375: [MCDwarf] Generate DWARF v5 .debug_rnglists for assembly files
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 3 14:24:33 PST 2020
MaskRay marked 2 inline comments as done.
MaskRay added inline comments.
================
Comment at: llvm/lib/MC/MCDwarf.cpp:48-59
+void mcdwarf::emitListsTableHeaderStart(MCStreamer *S, MCSymbol *TableStart,
+ MCSymbol *TableEnd) {
+ S->AddComment("Length");
+ S->emitAbsoluteSymbolDiff(TableEnd, TableStart, 4);
+ S->emitLabel(TableStart);
+ S->AddComment("Version");
+ S->emitInt16(S->getContext().getDwarfVersion());
----------------
dblaikie wrote:
> Please pass "S" by reference here, since it's unconditionally dereferenced (so the null state isn't needed) - this will simplify some of the callers too (x.get() -> *x)
>
> TableStart and TableEnd can remain pointers since their underlying APIs use them/they're more "tokens" to pass around.
Agreed with the analysis.
It also makes sense for `MCStreamer::emitAbsoluteSymbolDiff` to take references as parameters. However, `MCSymbol *` typed parameters are much more common than `MCSymbol &`.
================
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);
----------------
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`?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75375/new/
https://reviews.llvm.org/D75375
More information about the llvm-commits
mailing list