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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

The general direction is fine, but some details need to be tidied up and I'd like to undo a few unnecessary changes.



================
Comment at: llvm/include/llvm/MC/MCDwarf.h:48
+                                        MCSymbol *TableEnd);
+};
+
----------------
I think it would be more usual to use a namespace here (which would be lowercase, `mcdwarf`).


================
Comment at: llvm/lib/MC/MCDwarf.cpp:941
                              const MCSymbol *LineSectionSymbol,
-                             const MCSymbol *RangesSectionSymbol) {
+                             const MCSymbol *RangesStartSymbol) {
   MCContext &context = MCOS->getContext();
----------------
This name change seems inappropriate.  This symbol is not paired with an End symbol, and for the GenDwarf case there is only one range list.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:966
   int AddrSize = AsmInfo.getCodePointerSize();
-  if (context.getDwarfVersion() >= 5) {
+  bool DWARF5 = context.getDwarfVersion() >= 5;
+  if (DWARF5) {
----------------
`isDWARF5`


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1147
+      const MCSymbol *StartSymbol = Sec->getBeginSymbol();
+      const MCSymbol *EndSymbol = Sec->getEndSymbol(context);
 
----------------
Why remove the asserts?


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1153
+                          StartSymbol, MCSymbolRefExpr::VK_None, context),
+                      AddrSize);
 
----------------
AFAICT changing these lines has no functional effect, and IMO the result is less readable.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1159
+                   MakeStartMinusEndExpr(*MCOS, *StartSymbol, *EndSymbol, 0),
+                   AddrSize);
+    }
----------------
This change also appears to make the code less readable.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1186
   MCSymbol *InfoSectionSymbol = nullptr;
-  MCSymbol *RangesSectionSymbol = nullptr;
+  MCSymbol *RangesStartSymbol = nullptr;
 
----------------
This seems like a gratuitous name change.  There is only one range list, and it has its own section, so the old name seems fine.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1220
   if (UseRangesSection)
-    EmitGenDwarfRanges(MCOS);
+    RangesStartSymbol = emitGenDwarfRanges(MCOS);
 
----------------
Lost another assertion here.  You could move it inside emitGenDwarfRanges() if you wish, but I would rather not remove assertions.


================
Comment at: llvm/test/MC/ARM/dwarf-asm-multiple-sections.s:2
 // RUN: llvm-mc < %s -triple=armv7-linux-gnueabi -filetype=obj -o %t -g -dwarf-version 5 -fdebug-compilation-dir=/tmp
-// RUN: llvm-dwarfdump -v %t | FileCheck -check-prefix DWARF -check-prefix DWARF45 %s
+// RUN: llvm-dwarfdump -v %t | FileCheck -check-prefix DWARF5 %s
 // RUN: llvm-dwarfdump --debug-line %t | FileCheck -check-prefix DWARF-DL -check-prefix DWARF-DL-5 -DDWVER=5 -DDWFILE=0 %s
----------------
I'd prefer to have this check both DWARF and DWARF5 prefixes. The generic checks provide context.


================
Comment at: llvm/test/MC/ARM/dwarf-asm-multiple-sections.s:4
 // RUN: llvm-dwarfdump --debug-line %t | FileCheck -check-prefix DWARF-DL -check-prefix DWARF-DL-5 -DDWVER=5 -DDWFILE=0 %s
-// RUN: llvm-objdump -r %t | FileCheck -check-prefix RELOC -check-prefix RELOC5 %s
+// RUN: llvm-objdump -r %t | FileCheck -check-prefix RELOC5 %s
 // RUN: llvm-mc < %s -triple=armv7-linux-gnueabi -filetype=obj -o %t -g -dwarf-version 4 -fdebug-compilation-dir=/tmp
----------------
Keep this using both RELOC and RELOC5 here.


================
Comment at: llvm/test/MC/ARM/dwarf-asm-multiple-sections.s:78
 // DWARF: 00000000 00000000 00000004
 // DWARF: 00000000 <End of list>
 
----------------
The above checks (for .debug_ranges) should all become DWARF4 checks.


================
Comment at: llvm/test/MC/ARM/dwarf-asm-multiple-sections.s:110
+// RELOC-NEXT: 00000004 R_ARM_ABS32 .text
+// RELOC-NEXT: 00000014 R_ARM_ABS32 foo
+
----------------
The .debug_ranges checks should all be using RELOC4.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75375