[PATCH] D78851: Debug Info Support for Basic Block Sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:402-412
+    SmallVector<RangeSpan, 2> BB_List;
+    BB_List.push_back({Asm->getFunctionBegin(), Asm->getFunctionEnd()});
+    // If basic block sections are on, the [getFunctionBegin(),
+    // getFunctionEnd()] range will include all BBs which are in the same
+    // section as the entry block. Ranges for the other BBs have to be emitted
+    // separately.
+    for (auto &MBB : *Asm->MF) {
----------------
dblaikie wrote:
> Does this code produce the correct behavior when !BBSections? If so, I don't think it's worth the special-case for !BBSections - happy to just let it flow out from this code.
This is a bit of a duplicate of the code in DwarfDebug::endFunctionImpl - perhaps it could be refactored into a reusable function in some way?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:402-414
+    SmallVector<RangeSpan, 2> BB_List;
+    BB_List.push_back({Asm->getFunctionBegin(), Asm->getFunctionEnd()});
+    // If basic block sections are on, the [getFunctionBegin(),
+    // getFunctionEnd()] range will include all BBs which are in the same
+    // section as the entry block. Ranges for the other BBs have to be emitted
+    // separately.
+    for (auto &MBB : *Asm->MF) {
----------------
Does this code produce the correct behavior when !BBSections? If so, I don't think it's worth the special-case for !BBSections - happy to just let it flow out from this code.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:597
+    // list.  If there is more than one range, debug ranges must be used.
+    // Otherwise, High PC can be used.
+    do {
----------------
Probably "Otherwise, low/high PC can be used."


================
Comment at: llvm/test/DebugInfo/X86/basicblock-sections_1.ll:44-45
+; BB-SECTIONS-ASM-NEXT:	.quad	0
+; BB-SECTIONS: DW_AT_low_pc     DW_FORM_addr
+; BB-SECTIONS: DW_AT_ranges	DW_FORM_sec_offset
+
----------------
Probably put this up along with the NO-SECTIONS dwarfdump testing.

Also - probably best to test the .debug_info section rather than the .debug_abbrev section. You can add -v to dump which will print forms and sections:

no sections:
```
DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000 ".text")
DW_AT_high_pc [DW_FORM_data4] ({{.*}})
```

basic block sections:
```
DW_AT_low_pc [DW_FORM_addr] (0x0000000000000000)
DW_AT_ranegs [DW_FORM_sec_offset] (
                 [0x0000000000000000, {{.*}}) ".text._Z2f1v"
                 [0x0000000000000000, {{.*}}) ".text._Z2f2v")
```

or something like that (I mean that sample was from function sections, but bb sections should work similarly - easier with unique section names to identify which ones are there)


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

https://reviews.llvm.org/D78851