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

[David Blaikie]

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?

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

>
> thanks,
> Adrian
>
> PS: Patch attached for reference.
>