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

Mon May 24 18:16:03 PDT 2021

tmsriram 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;
----------------
dblaikie wrote:
> 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()`?
Nice catch!

================
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:
> 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?
> 
> 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?

https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp#L382

Please look at beginInstruction and endInstruction in DebugHandlerBase.cpp.  LabelsBeforeInsn and LabelsAfterInsn are start and end label for each location list entry which will be either merged or kept unsplit in buildLocationList.  LabelsBeforeInsn and LabelsAfterInsn are set here and these get a new label everytime we cross a basic block boundary via a jmp insn etc.  This is true with or without sections.

Every jmp instruction and call insn for example will split the range.  This would later be coalesced if possible in buildLocationList.  endInstruction() in DebugHandlerBase.cpp is where the fragmenting happens in general, with or without basic block sections.

There is no code elsewhere that does anything specific for basic block sections with regards to location list.  The real issue is with the coalescing of ranges which happens here for both sections and no-sections.  With sections, this one assumption about the use starting from an intermediate basic block needs special handling.

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

Correct, that one use case where the parameter is used at a basic block other than the entry but is defined from the start.  An alternate way is to repeat the entry for this parameter multiple times pretending it was used in the entry too but that is a lot of code to do it.  It also unnecessarily complicates the case for compiling without basic block sections.  Please note that if we do this, we would split up the range first and then merge it when building without sections.

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

I added a specific test for this, basic-block-setions-debug-loc-split-range.ll.  That test will exercise this code to split the range as the first use is not in the entry basic block.   A location list will be generated for this as the two uses of the value "buflen" in this test are different, 157 amd 159.

 The original test which prompted the whole cleanup, basic-block-sections-debug-loc.ll will also exercise this code.  Here, it will be merged into a const_value 157 as both uses of the value of buflen are the same.  This means the range will first be split with basic block sections and then merged again later.

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

https://reviews.llvm.org/D85085