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

Tue Apr 27 15:32:31 PDT 2021

tmsriram added a comment.

In D85085#2708898 <https://reviews.llvm.org/D85085#2708898>, @dblaikie wrote:

> 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.

Thanks for reviewing this and providing the test cases. Took me quite a while to understand how to fix this!

================
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
----------------
dblaikie wrote:
> 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)
I may have misunderstood your question but I am assuming you are asking if StartLabel and Instr can be in different sections when StartLabel is not function begin?

This is a special case, mentioned in DebugHandlerBase.cpp in function "beginFunction".  Quoting : "The first mention of a function argument gets the CurrentFnBegin label, so arguments are visible when breaking at function entry."

This means the Debug Instr and function begin could be in different sections which was the original problem to begin with.  We make sure that we update the label each time we cross a section boundary otherwise, so an instr and a label being in different sections apart from this case is not possible.  In function "endBasicBlock" PrevLabel is explicitly set to nullptr.  This will force an emission of a temp label (or reuse the section's begin label) in the same section as the instruction for which a LabelsBeforeInsn is desired.

================
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,
----------------
dblaikie wrote:
> 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;
> ...
> ```
Apologies for not refactoring.

================
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
----------------
dblaikie wrote:
> presumably this isn't only about const_values - but any location (including non-const values) that's valid across the whole scope?
Right, reworded to say it is about merging ranges.

================
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.
+
----------------
dblaikie wrote:
> 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
Understood, I added some more text describing what this test does.  The source is not available because the IR was obtained using a reducer from a larger failing compile. 

================
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")
----------------
dblaikie wrote:
> 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)
I removed file, line and type.  Seems like the other stuff is useful but if you think I should prune more, please lmk.

================
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
----------------
dblaikie wrote:
> 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)
Fixed and you are right about the chunks. The extra chunks for sections because there are the 2 additional basic blocks in different sections which cannot be collapsed. 

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

https://reviews.llvm.org/D85085