[LLVMdev] Expected behavior of eliminateFrameIndex() on dbg_value machine instructions

[Jakob Stoklund Olesen]

On Oct 11, 2011, at 6:18 AM, Richard Osborne wrote:

> On 10/10/11 19:19, Jakob Stoklund Olesen wrote:
>> On Oct 10, 2011, at 10:26 AM, Richard Osborne wrote:
>>> I'm investigating a bug associated with debug information that manifests
>>> itself in the XCore backend (PR11105). I'd like to understand what the
>>> expected behavior of eliminateFrameIndex() is when it is called on a
>>> dbg_value machine instruction.
>> That is up to the target.
>> 
>> The TII::emitFrameIndexDebugValue() hook is called to insert DBG_VALUE instructions referring to values on the stack.
> Thanks for pointing this out. Looking at the code in constructVariableDIE it seems to use the number of operands to determine if the DBG_VALUE is target specific or not. It is considered target specific if it does not have exactly 3 operands in which case the getDebugValueLocation hook is called.

That's right.

> Implementing both TII::emitFrameIndexDebugValue() and AsmPrinter::getDebugValueLocation() in the XCore backend seems to fix the problem I was seeing.

Good!

> I'm still confused about what the following code in CompileUnit::constructVariableDIE is trying to do:
> 
>  if (const MachineInstr *DVInsn = DV->getMInsn()) {
>    bool updated = false;
>    if (DVInsn->getNumOperands() == 3) {
>      if (DVInsn->getOperand(0).isReg()) {
>        const MachineOperand RegOp = DVInsn->getOperand(0);
>        const TargetRegisterInfo *TRI = Asm->TM.getRegisterInfo();
>        if (DVInsn->getOperand(1).isImm() &&
>            TRI->getFrameRegister(*Asm->MF) == RegOp.getReg()) {
>          unsigned FrameReg = 0;
>          const TargetFrameLowering *TFI = Asm->TM.getFrameLowering();
>          int Offset =
>            TFI->getFrameIndexReference(*Asm->MF,
>                                        DVInsn->getOperand(1).getImm(),
>                                        FrameReg);
>          MachineLocation Location(FrameReg, Offset);
>          addVariableAddress(DV, VariableDie, Location);
> 
> If the intention is to handle the case where the target doesn't implement emitFrameIndexDebugValue(), then there is a mismatch between the operands it expects and the operands added in UserValue::insertDebugValue(). I've attached a patch which fixes this. Does this look correct to you?

I believe that code handles the special case of referring to an incoming function argument passed on the stack.

Ideally, we should get rid of this special case, and just emit a reference to <fi#-1>.

There is another problem with this code: It is using the Offset operand the wrong way.

A DBG_VALUE instruction takes (Location, Offset, UserVariable) operands. The Location part can be an immediate, a register, a frameindex reference, or multiple target-dependent operands.

The Offset operand refers to the UserVariable, not the Location. For example:

  DBG_VALUE %R0, 4, !"x"

This means that bytes 4-7 of the user variable "x" are in %R0.  It does not mean that "x" is at *(R0 + 4), but that is how the special case code interprets it.


Ideally, a reference to an incoming argument should look like this:

  DBG_VALUE <fi#-1+8>, 4, !"x"
  DBG_VALUE %R4, 0, !"x"

That is, bytes 4-7 of the incoming argument "x" are at an offset of 8 bytes into fi#-1, and bytes 0-3 of "x" are in %R4.  We normally use a single fi#-1 for all incoming stack arguments.

Two problems: Our FrameIndex machine operand doesn't currently support offsets, and emitFrameIndexDebugValue() is missing a second offset argument.

So we need a bit of infrastructure work before we can get rid if this hack.  In the meantime, don't fix what isn't broken.

>> If you don't implement the hook, you will get DBG_VALUE instructions with just a FrameIndex operand followed by the mandatory Offset and Metadata operands.
> This behavior doesn't match the comment above TII::emitFrameIndexDebugValue() which says "For targets that do not support this the debug info is simply lost". In UserValue::insertDebugValue in LiveDebugVariables.cpp if emitFrameIndexDebugValue returns 0 then it adds a DBG_VALUE with the standard operands. However in other places where emitFrameIndexDebugValue is called it appears to removes the debug value (e.g. in InlineSpiller::spillAroundUses()). Should the comment be updated or is the code in LiveDebugVariables.cpp doing the wrong thing?

I think it is best to always insert a single-operand DBG_VALUE so target writers like you will notice the strange DBG_VALUE instructions in PEI.

We should provide a default implementation of the hook that inserts a target-independent (fi, offset, md) DBG_VALUE instruction, and remove the fallback code in LDV.

Would you like to write that patch?

/jakob