[llvm] r185993 - Un-break the buildbot by tweaking the indirection flag.

[Adrian Prantl]

On Jul 12, 2013, at 3:09 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Wed, Jul 10, 2013 at 5:53 PM, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>> On Jul 10, 2013, at 2:58 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>>> On Wed, Jul 10, 2013 at 2:56 PM, Adrian Prantl <aprantl at apple.com> wrote:
>>>> 
>>>> On Jul 10, 2013, at 2:51 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>> 
>>>>> So we're on the same page: isIndirect implies that the IR type is a
>>>>> pointer. That was my intention, at least - is that clear/agreed?
>>>> 
>>>> That the IR type is a pointer of foo, but the type of the DIVariable is foo. (As in your example with nontrivial by-value arguments).
>>> 
>>> Yep.
>> 
>> Hi David,
>> 
>> I’ve got it working now. There is one thing I’d like to double-check with you before I commit my changes, though.
>> 
>> Can you elaborate on this change you made in r184387?
>> 
>>> @@ -384,13 +385,17 @@ void CompileUnit::addRegisterOffset(DIE *TheDie, unsigned
>>> /// addAddress - Add an address attribute to a die based on the location
>>> /// provided.
>>> void CompileUnit::addAddress(DIE *Die, unsigned Attribute,
>>> -                             const MachineLocation &Location) {
>>> +                             const MachineLocation &Location, bool Indirect) {
>>>   DIEBlock *Block = new (DIEValueAllocator) DIEBlock();
>>> 
>>> -  if (Location.isReg())
>>> +  if (Location.isReg() && !Indirect)
>>>     addRegisterOp(Block, Location.getReg());
>>> -  else
>>> +  else {
>>>     addRegisterOffset(Block, Location.getReg(), Location.getOffset());
>>> +    if (Indirect && !Location.isReg()) {
>>> +      addUInt(Block, 0, dwarf::DW_FORM_data1, dwarf::DW_OP_deref);
>>> +    }
>>> +  }
>>> 
>> 
>> The commit message does not really mention it.
>> Specifically, can there be a situation where MachineLocation::isIndirect() and DIVariable::isIndirect() disagree?
> 
> I believe they're orthogonal. In fact I'm pretty sure in the case I
> needed it they're decidedly would /not/ agree (the parameter was
> stored directly in a register (not indirect) - but the ABI-level
> parameter was a pointer (due to a non-trivial by-value parameter) to
> the object in question. So we needed to record in the IR debug info
> metadata that the parameter wasn't at the location given by the
> dbg.value - but indirected through the pointer there.)
> 
> Make sense?

Your explanation makes sense, thanks! Do we actually have a testcase for that situation?

> 
>> My patch essentially would revert this change, because we are now piping the IsIndirect flag all the way down to the MachineLocation, but I want to make sure that I’m not missing something.
> 
> I haven't really looked at your patch deeply enough to fully
> understand this. There was a reason that I didn't do this in my
> original change even though I wrote the patch that could do this (for
> dbg_value MI intrinsics, not for the MachineLocation itself) but
> didn't end up needing i- though I don't fully recall why it wasn't
> suitable for my needs in the end.

In the light of this it seems that we need to update the documentation:

DIVariable::isIndirect() - Indicates that the variable is stored indirect (such as a VLA).
MachineLocation::isIndirect() - Indicates a layer of indirection introduced by the ABI (such as a byval struct).

This also means that it is not correct to ket MachineLocation inherit the DIVariable::isIndirect flag during lowering. That is -- unless it is impossible to have two layers of indirection...

I’m going to work on that.

-- adrian