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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

This is currently only for "basic block sections", right? No linker relaxation, etc? (so symbol differences within a bb section (even using symbols at the very end of the bb section, after the trailing jump) are valid/knowable at compile-time/don't change during linking)



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp:215-223
+    //
+    // If the first mention of an argument is in a unique-section basic block,
+    // we cannot assign the CurrentFnBegin label, as it lies in a different
+    // section. For simplicity, we simply ignore such mentions in this code.
     const DILocalVariable *DIVar =
         Entries.front().getInstr()->getDebugVariable();
     if (DIVar->isParameter() &&
----------------
Is there a test case covering this? Rough sense of how much this hurts debug info quality?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:592-597
+    if (BeginMBB->sameSection(EndMBB)) {
+      // If begin and end share their section, there is just one continuous
+      // range.
+      List.push_back({BeginLabel, EndLabel});
+      continue;
+    }
----------------
Would this fall out naturally/produce the same answer as the loop below? If so, I'd probably let it happen that way rather than special casing it.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:598-609
+
+    const auto *MBB = BeginMBB;
+    do {
+      if (MBB->sameSection(EndMBB) || MBB->isEndSection()) {
+        auto MBBSectionRange = Asm->MBBSectionRanges[MBB->getSectionID()];
+        List.push_back(
+            {MBB->sameSection(BeginMBB) ? BeginLabel
----------------
Probably add a comment describing what's happening here (similar to the comment above about the continuous range case)


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2029-2039
   // Add the range of this function to the list of ranges for the CU.
   TheCU.addRange({Asm->getFunctionBegin(), Asm->getFunctionEnd()});
 
+  // With basic block sections, add ranges for all basic block sections.
+  if (MF->hasBBSections()) {
+    for (auto &MBB : *MF) {
+      if (&MBB != &MF->front() && MBB.isBeginSection())
----------------
If you remove the `&MBB != &MF->front()` case, would the function begin/end entry fall out/be produced by the loop? (or is "MBBSectionRanges" only valid if "hasBBSections"? Maybe it'd be handy if it always contained the ranges - just one range when "hasBBSections" is false?)


================
Comment at: llvm/test/DebugInfo/X86/basicblock-sections_1.ll:13-16
+; NO-SECTIONS: DW_AT_low_pc     DW_FORM_addr
+; NO-SECTIONS: DW_AT_high_pc    DW_FORM_data4
+; BB-SECTIONS: DW_AT_low_pc     DW_FORM_addr
+; BB-SECTIONS: DW_AT_ranges	DW_FORM_sec_offset
----------------
This might be a case where testing the assembly would be appropriate - to check which symbols and symbolic expressions are used for the ranges/high/low rather than only that is "some range".


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

https://reviews.llvm.org/D78851