[PATCH] D85085: Fix debug_loc offset difference with basic block sections

Thu Apr 22 08:43:19 PDT 2021

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Thanks for this - tests generally look great. Code mostly makes sense to me (that's on me, not you) - couple of minor quibbles with comment wording, but nothing major I think.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1742-1744
+    // Instr are not in the same section.  This happens when the StartLabel is
+    // the function begin label and the dbg value appears in a basic block
+    // that is not the entry.  In this case, the range needs to be split to
----------------
Presumably this happens also when StartLabel isn't the function begin symbol - but could be inside but not at the start of the first BB, or in some other BB - but where the end is in some other BB?
(the test cases are all simple enough/happen to have the interesting location range start in the first BB, but it's not a requirement)

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1787-1788
+    if (DebugLoc.size() == 1) {
+      return true;
+    } else if (Asm->MF->hasBBSections()) {
+      // Check here to see if loclist can be replaced with const_value. If not,
----------------
Drop the else after return, ie:
```
if (x)
  return
if (hasBBSections()) {
  ...
```
Might also be worth inverting a bunch of these tests to reduce indentation/early return, eg:
```
if (!isSafeForSingleLocation)
  return false;
if (!validThroughout)
  return false;
if (size == 1)
  return true;
if (hasBBSections())
  return false;
// ...
MachineBasicBlock *RangeMBB = nullptr;
...
```

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1789
+    } else if (Asm->MF->hasBBSections()) {
+      // Check here to see if loclist can be replaced with const_value. If not,
+      // we must keep the split loclists per section.  This does exactly what
----------------
presumably this isn't only about const_values - but any location (including non-const values) that's valid across the whole scope?

================
Comment at: llvm/test/DebugInfo/X86/basic-block-sections-debug-loc-split-range.ll:17-18
+
+; This IR is generated by copying basic-block-sections-debug-loc.ll and
+; manually adding one more dbg value in block 2 and modifying the value.
+
----------------
might be worth a few more words, maybe the original source/description of the original test case - so this one can be read more independently (still useful to reference where it came from, though) - otherwise it's hard to know why that test + the transformation should have the location this test is testing for

================
Comment at: llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-2.ll:13
+; CHECK-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
+; SECTIONS:   [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
+; CHECK-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +9, DW_OP_stack_value
----------------
probably SECTIONS-NEXT

================
Comment at: llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-3.ll:12-16
+; CHECK-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_reg3
+; CHECK-NEXT: DW_AT_name	("i")
+; CHECK-NEXT: DW_AT_decl_file	("/code/loclist_3.cc")
+; CHECK-NEXT: DW_AT_decl_line	(8)
+; CHECK-NEXT: DW_AT_type	(0x{{[0-9a-f]+}} "int")
----------------
I /think/ this test might be able to be made a bit more resilient to future unrelated DWARF changes (such as adding/removing/renaming unrelated attributes) with something like:
```
CHECK-NEXT: ... reg3
CHECK-NEXT: DW_AT
CHECK-NOT: DW_TAG
CHECK: _name ("i")
```
I'm not 100% sure that CHECK will match both in the case when there's a DW_AT_name immediately after the DW_TAG_location, and when there's some other attributes between location and name.

We don't always go to this much bother - but at least you can probably remove the decl file/line/type - the name seems enough to verify that the test hasn't accidentally started matching the location of some other variable like 'tmp'

(consider this sort of change/design aspect for the other test cases too)

================
Comment at: llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-5.ll:13
+; CHECK-NEXT: [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
+; SECTIONS:   [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
+; SECTIONS-NEXT:   [0x{{[0-9a-f]+}}, 0x{{[0-9a-f]+}}): DW_OP_consts +7, DW_OP_stack_value
----------------
Guess this should probably be `SECTIONS-NEXT` to continue matching this location list/not accidentally skip over some other location

I take it there are 3 chunks because there are 3 BBs this constant value is live over (the initial BB, the if, and then a fragment of the last/returning BB (during the execution of the f2 call, but before the result has been assigned that will overwrite the constant value)

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

https://reviews.llvm.org/D85085