[PATCH] Bugfix for debug intrinsic handling in InstCombiner

[Bob Wilson]

Adrian, please go ahead and commit the patch at least for now.  It has been 10 days since you replied to Eric's questions and he hasn't replied.  From what I can tell, it's not even clear that Eric's concerns are applicable to this patch, since he seems to have thought the problem was occurring when an alloca is removed, which is not the case.

Certainly if Eric has further comments, you will need to respond, possibly by reverting the patch if that is appropriate.

It is not OK for simple bug fix patches like this to get blocked for weeks waiting for a review.

On Apr 23, 2013, at 8:15 PM, Adrian Prantl <aprantl at apple.com> wrote:

> Hi Eric,
> 
> To recap: LowerDbgDeclare() tries to replace a dbg.declare intrinsic for an alloca with dbg.value intrinsics at each use of the alloca, where use is one of {load, store}. If it can’t convert all  uses (actual ones, not just loads and stores) of the alloca it will leave the dbg.declare and all dbg.values. LowerDbgDeclare gets called a lot. If there is a use other than a load/store, in this case a call with a function pointer argument, we end up with tons of duplicate dbg.values because the dbg.declare is never removed.
> This has nothing to do with the alloca being removed by RA or elsewhere as you suspected earlier, but with the (flawed) assumption of LowerDbgDeclare() that all uses of an alloca are load/stores.
> My patch checks for existing dbg.values before adding them because I’m not sure if we can assert that all uses of an alloca are load, stores (and calls). Let me know if the optimizer makes that guarantee or if you see a better way to address this. I just would really like to get this fixed soon :-)
> 
> thanks,
> Adrian
> 
> On Apr 16, 2013, at 6:22 PM, Adrian Prantl <aprantl at apple.com> wrote:
> 
>> Hi Eric,
>> 
>> sorry about the delay! Here’s the promised explanation:
>> As I mentioned in #llvm earlier today, the logic in llvm::LowerDbgDeclare() is obviously flawed. We try to lower a dbg.declare intrinsic into a dbg.value intrinsic for each use of the alloca referenced by the dbg.declare. If, however, we cannot convert all uses, we don’t remove the dog.declare, so subsequent calls to llvm::LowerDbgDeclare() (and they are plentiful) will try again and emit the dbg.values for (almost) each use again, and again.
>> Now, the interesting question is why it fails to add dbg.values for _all_ uses in the testcase.
>> The basic assumption of LowerDbgDeclare is that all uses of an alloca are either a load or a store. But in the testcase there is the call to printf
>>> %i14 = alloca %struct.i14*, align 8
>>> %call = call i32 @foo(i8* bitcast (void ()* @bar to i8*), %struct.i14** %i14) #3, !dbg !64
>> which also uses the alloca as an argument.
>> 
>> Of course we can also fix the bug by handling call instructions akin to load/store’s, but can we a guarantee that there won’t be any other instructions that would use an alloca? (Honest question).
>> That’s why I went with the current implementation of the patch.
>> 
>> thanks for the review!
>> Adrian
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits