[llvm] r217260 - Set the parent pointer of cloned DBG_VALUE instructions correctly.

Adrian Prantl aprantl at apple.com
Fri Sep 5 13:36:11 PDT 2014


> On Sep 5, 2014, at 1:26 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> 
> On Fri, Sep 5, 2014 at 1:13 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> 
>> On Sep 5, 2014, at 10:31 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> On Sep 5, 2014 10:22 AM, "Adrian Prantl" <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>> >
>> > Author: adrian
>> > Date: Fri Sep  5 12:10:10 2014
>> > New Revision: 217260
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=217260&view=rev <http://llvm.org/viewvc/llvm-project?rev=217260&view=rev>
>> > Log:
>> > Set the parent pointer of cloned DBG_VALUE instructions correctly.
>> > Fixes PR20523.
>> >
>> > When spilling variables onto the stack, spillVirtReg() is setting the
>> > parent pointer of the cloned DBG_VALUE intrinsic for the stack location
>> > to the parent pointer of the original intrinsic. MachineInstr parent
>> > pointers should however always point to the parent basic block.
>> >
>> > MBB is shadowing the MBB member variable. The instruction still ends up
>> > being inserted into the right basic block, because it's inserted after MI
>> > which serves as the iterator.
>> 
>> Perhaps we should assert that the iterator within the basic block?
>> 
>> 
> That makes sense on its own, but it wouldn’t help with catching the bug in the PR. I’ll add this as an additional assertion.
> 
> Hmm, why not? Wouldn't that assertion fail - since the MI is not in the MBB in this case (before your patch)?

I meant, (int theory) if somebody really were to re-insert this line into the code, the MI assertion would still hold. (That is, if we insert that assertion at the beginning of the function — I may have misinterpreted you there). Anyway is fine really, I don’t care enough.

      } else
        DL = MI->getDebugLoc();
-      MachineBasicBlock *MBB = DBG->getParent();
      MachineInstr *NewDV =
          BuildMI(*MBB, MI, DL, TII->get(TargetOpcode::DBG_VALUE))
              .addFrameIndex(FI).addImm(Offset).addMetadata(MDPtr);

>  
> 
>> > I failed at constructing a reliable testcase for this, see
>> > http://llvm.org/bugs/show_bug.cgi?id=20523 <http://llvm.org/bugs/show_bug.cgi?id=20523> for a large testcases.
>> >
>> > Modified:
>> >     llvm/trunk/lib/CodeGen/RegAllocFast.cpp
>> >
>> > Modified: llvm/trunk/lib/CodeGen/RegAllocFast.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocFast.cpp?rev=217260&r1=217259&r2=217260&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/RegAllocFast.cpp?rev=217260&r1=217259&r2=217260&view=diff>
>> > ==============================================================================
>> > --- llvm/trunk/lib/CodeGen/RegAllocFast.cpp (original)
>> > +++ llvm/trunk/lib/CodeGen/RegAllocFast.cpp Fri Sep  5 12:10:10 2014
>> > @@ -309,10 +309,10 @@ void RAFast::spillVirtReg(MachineBasicBl
>> >          DL = (--EI)->getDebugLoc();
>> >        } else
>> >          DL = MI->getDebugLoc();
>> > -      MachineBasicBlock *MBB = DBG->getParent();
>> >        MachineInstr *NewDV =
>> >            BuildMI(*MBB, MI, DL, TII->get(TargetOpcode::DBG_VALUE))
>> >                .addFrameIndex(FI).addImm(Offset).addMetadata(MDPtr);
>> > +      assert(NewDV->getParent() == MBB && "dangling parent pointer");
>> 
>> Maybe this assertion (in some form) could be moved into a more central location so this invariant (that BuildMI functions are passed an iterator into the same MBB - but maybe even below the build function, in the insertion itself)
>> 
>> 
> 
> I experimented with the following patch and it seems to lead into a world of pain... Apparently we violate this invariant quite often at the moment.
> 
> What kind of violations? Is the parent just null here, perhaps? but even then, seems like something not good.

Right, just from poking at random testcases it’s running into the assertion from FunctionLoweringInfo::set() with a similar setup where the iterator points to a different basic block.

>  
> 
> diff --git a/include/llvm/CodeGen/MachineBasicBlock.h b/include/llvm/CodeGen/MachineBasicBlock.h
> index a08cc2e..edd228a 100644
> --- a/include/llvm/CodeGen/MachineBasicBlock.h
> +++ b/include/llvm/CodeGen/MachineBasicBlock.h
> @@ -486,11 +486,13 @@ public:
>    /// Insert a range of instructions into the instruction list before I.
>    template<typename IT>
>    void insert(iterator I, IT S, IT E) {
> +    assert(I->getParent() == this && "iterator points outside of basic block");
>      Insts.insert(I.getInstrIterator(), S, E);
>    }
>  
>    /// Insert MI into the instruction list before I.
>    iterator insert(iterator I, MachineInstr *MI) {
> +    assert(I->getParent() == this && "iterator points outside of basic block");
>      assert(!MI->isBundledWithPred() && !MI->isBundledWithSucc() &&
>             "Cannot insert instruction with bundle flags");
>      return Insts.insert(I.getInstrIterator(), MI);
> @@ -498,6 +500,7 @@ public:
>  
>    /// Insert MI into the instruction list after I.
>    iterator insertAfter(iterator I, MachineInstr *MI) {
> +    assert(I->getParent() == this && "iterator points outside of basic block");
>      assert(!MI->isBundledWithPred() && !MI->isBundledWithSucc() &&
>             "Cannot insert instruction with bundle flags");
>      return Insts.insertAfter(I.getInstrIterator(), MI);
> 
> 
> 
>> >        (void)NewDV;
>> >        DEBUG(dbgs() << "Inserting debug info due to spill:" << "\n" << *NewDV);
>> >      }
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140905/9d6cd8ad/attachment.html>


More information about the llvm-commits mailing list