Expected behavior of eliminateFrameIndex() on dbg_value machine instructions

Project Infrastructure LLVM Dev List Archives
#1

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.

Currently the XCore target replaces the frame index with the frame register and sets the next operand to the byte offset from the frame register. A quick glance at some of the other targets suggests this is the right thing to do (for example ARMBaseRegisterInfo::eliminateFrameIndex and Thumb1RegisterInfo::eliminateFrameIndex appear to handle dbg_value in exactly the same way).

Unfortunately this results in an assertion firing in MachineFrameInfo::getObjectOffset(). This is due to CompileUnit::constructVariableDIE() passing the second operand of the dbg_value instruction (set to the byte offset) to getFrameIndexReference() which expects a frame index as an argument.

Am I doing something wrong in eliminateFrameIndex()? Is this a bug in CompileUnit::constructVariableDIE()? Is there something else I'm missing?

Thanks,

Richard

#2

That is up to the target.

The TII::emitFrameIndexDebugValue() hook is called to insert DBG_VALUE instructions referring to values on the stack.

The target's eliminateFrameIndex() should be able to handle any DBG_VALUE instructions created by this hook.

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.

/jakob

#3

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. Implementing both TII::emitFrameIndexDebugValue() and AsmPrinter::getDebugValueLocation() in the XCore backend seems to fix the problem I was seeing.

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?

The target's eliminateFrameIndex() should be able to handle any DBG_VALUE instructions created by this hook.

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?

dbg_value.patch (1.14 KB)

#4

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