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

Fri May 7 18:17:46 PDT 2021

dblaikie added inline comments.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1750
+        !Instr->getParent()->sameSection(&Asm->MF->front())) {
+      MachineBasicBlock *MBB_I = &Asm->MF->front();
+      const MCSymbol *BeginSectionLabel = StartLabel;
----------------
Hmm - using "I" for this sort of implies it's an iterator, but it isn't (if it was, we could increment it - but if we try to increment this pointer presumably we end up with garbage/random things that are in memory, because it's a linked list) - maybe if this was Asm->MF->begin() instead of front? 

(also the name's still being flagged by the style checker - so would be good to rename it to something that matches the LLVM naming conventions)

It might be possible this could/should be a range-based-for loop?
```
for (MachineBasicBlock &MBB : *Asm->MF) {
}
```

And the checks like `MBB_I != &Asm->MF->front()` could be tested with `&MBB != &Asm->MF->front()`?

================
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
----------------
tmsriram wrote:
> 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.
> 
> 
Stared at this a few times & still a bit confused, sorry.

Hmm, so there's some code somewhere else that handles the normal case of fragmenting basic blocks? Where is that code? (maybe there's a chance some of the logic here could be shared with that code) - what if that code elsewhere was removed, and this code were simplified (removing the "StartLabel == function begin" check) and became the only place location ranges were fragmented for bb sections? Would that be simpler?

This code here is specifically to patch up some strangeness in DebugHandlerBase.cpp - that strangeness is that certain parameter locations have their starting location drawn back up to the start of the function?

OK, so a few things - I'm guessing most of the tests I wrote don't cover this particular case? They do cover the general case of multi-basic block scope locations being fragmented by BB sections (& also the case of multi-basic block but not full-scope locations being broken up by bb sections). Do any of the tests pass/fail with this particular change applied or not?

Then, maybe it's easier for me to understand by looking at a particular test case that this code addresses an issue in - which test case would that be?

================
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")
----------------
tmsriram wrote:
> 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.
Looks good - if you like, the test can be made more robust (in case new unrelated attributes are added in the future) by using `CHECK-NOT: DW_TAG` between attributes rather than using CHECK-NEXT (that way you'd still be checking that the attributes are all associated with the same tag - but allowing the possibility of other attributes).

It doesn't make the test resilient to changes in the order of attributes (you might be able to do that with something like:
```
CHECK: DW_TAG_variable
CHECK-DAG: DW_AT_location
CHECK-DAG: DW_AT_name
CHECK: DW_TAG
```
But if you do that, I'm not sure if the CHECK-NEXT needed for the DW_AT_location would work (not sure how it interacts with CHECK-DAG))

================
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
----------------
tmsriram wrote:
> 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. 
Might be worth a comment in the test explaining this?

Maybe updating/changing/or moving the comment from the C source below and putting it up somewhere to say "When a location list must be used (because the location isn't valid through the entire enclosing scope - the assignment to 'i' towards the end truncates the 7 constant value location) - without basic block sections a single entry location list would be used. With basic block sections that one location list needs to be broken up to describe the separate address ranges in each section" or the like.

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

https://reviews.llvm.org/D85085