[DebugInfo] Make legal and emit DW_OP_swap and DW_OP_xderef

[kzhuravl]

Summary

Tested with D29673 and D29674

[aprantl]

Thanks!

• please include testcases
• there probably needs to be a Verifier change as well

As you probably noticed the other binary operators (like DW_OP_plus) are currently accepting their RHS argument as an "argument" rather than using the top-of-stack (-1). I consider this a bug, so I'm fine with these new operators behaving the way they are (using the stack) in this patch.

[kzhuravl]

Add additional test

[kzhuravl]

In D29672#669785, @aprantl wrote:

Thanks!

• please include testcases

Done.

• there probably needs to be a Verifier change as well

Isn't Verifier calling DIExpression::isValid()?

As you probably noticed the other binary operators (like DW_OP_plus) are currently accepting their RHS argument as an "argument" rather than using the top-of-stack (-1). I consider this a bug, so I'm fine with these new operators behaving the way they are (using the stack) in this patch.

Do you know if there is bugzilla for it or if someone is working on fixing this bug?

Thanks.

[aprantl]

I'd like to request one more feature:

isValid() should reject

!DIExpression(DW_OP_swap)

because there is only one (implicit) element on the stack.

[kzhuravl]

Address review feedback

[aprantl]

lib/IR/DebugInfoMetadata.cpp

622

I was just going to comment that a better way to implement this would be to add a local variable that keeps track of the stack depth. But I realize that we can't actually do that without introducing something like a DW_LLVM_OP_implicit_location as a placeholder for the location this DIExpression is attached to, or else pass the number of implicit stack elements into isValid (isValid(unsigned StackDepth).

For example:

!DIExpression(DW_OP_constu 1, DW_OP_swap, DW_OP_xderef)

is fine when attached to a llvm.dbg.value (with one implicit stack element).
But it would be wrong if attached to a !DIGlobalVariableExpression that is describing a constant (with no implicit stack elements).

This is not necessarily something we need to fix in this patch. It might be nice to add a FIXME about the limitation here.

docs/LangRef.rst

 variable.
 
 The current supported vocabulary is limited:
 
 - ``DW_OP_deref`` dereferences the working expression.
 - ``DW_OP_plus, 93`` adds ``93`` to the working expression.
 - ``DW_OP_bit_piece, 16, 8`` specifies the offset and size (``16`` and ``8``
   here, respectively) of the variable piece from the working expression.
+- ``DW_OP_swap`` swaps top two stack entries.
+- ``DW_OP_xderef`` provides extended dereference mechanism. The entry at the top
+  of the stack is treated as an address. The second stack entry is treated as an
+  address space identifier.
 
 .. code-block:: text
 
     !0 = !DIExpression(DW_OP_deref)
     !1 = !DIExpression(DW_OP_plus, 3)
     !2 = !DIExpression(DW_OP_bit_piece, 3, 7)
     !3 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_bit_piece, 3, 7)
+    !4 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
 
 DIObjCProperty
 """"""""""""""
 
 ``DIObjCProperty`` nodes represent Objective-C property nodes.
 
 .. code-block:: llvm
 

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

case dwarf::DW_OP_deref:
       break;
     case dwarf::DW_OP_constu:
       EmitOp(dwarf::DW_OP_constu);
       EmitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_stack_value:
       AddStackValue();
       break;
+    case dwarf::DW_OP_swap:
+      EmitOp(dwarf::DW_OP_swap);
+      break;
+    case dwarf::DW_OP_xderef:
+      EmitOp(dwarf::DW_OP_xderef);
+      break;
     default:
       llvm_unreachable("unhandled opcode found in expression");
     }
   }
 }
 
 void DwarfExpression::finalize() {
   if (SubRegisterSizeInBits)

lib/IR/DebugInfoMetadata.cpp

case dwarf::DW_OP_stack_value: {
       // Must be the last one or followed by a DW_OP_LLVM_fragment.
       if (I->get() + I->getSize() == E->get())
         break;
       auto J = I;
       if ((++J)->getOp() != dwarf::DW_OP_LLVM_fragment)
         return false;
       break;
     }
+    case dwarf::DW_OP_swap: {
+      // Must be more than one implicit element on the stack.
+      if (getNumElements() == 1)
+        return false;
+      break;
+    }
     case dwarf::DW_OP_constu:
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
     case dwarf::DW_OP_deref:
+    case dwarf::DW_OP_xderef:
       break;
     }
   }
   return true;
 }
 
 Optional<DIExpression::FragmentInfo>
 DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {

test/Assembler/diexpression.ll

 ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder %s
 
-; CHECK: !named = !{!0, !1, !2, !3, !4}
-!named = !{!0, !1, !2, !3, !4}
+; CHECK: !named = !{!0, !1, !2, !3, !4, !5}
+!named = !{!0, !1, !2, !3, !4, !5}
 
 ; CHECK:      !0 = !DIExpression()
 ; CHECK-NEXT: !1 = !DIExpression(DW_OP_deref)
 ; CHECK-NEXT: !2 = !DIExpression(DW_OP_plus, 3)
 ; CHECK-NEXT: !3 = !DIExpression(DW_OP_LLVM_fragment, 3, 7)
 ; CHECK-NEXT: !4 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_LLVM_fragment, 3, 7)
+; CHECK-NEXT: !5 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
 !0 = !DIExpression()
 !1 = !DIExpression(DW_OP_deref)
 !2 = !DIExpression(DW_OP_plus, 3)
 !3 = !DIExpression(DW_OP_LLVM_fragment, 3, 7)
 !4 = !DIExpression(DW_OP_deref, DW_OP_plus, 3, DW_OP_LLVM_fragment, 3, 7)
+!5 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)

test/Verifier/diexpression-swap.ll

+; RUN: not opt -S < %s 2>&1 | FileCheck %s
+
+!named = !{!0}
+; CHECK: invalid expression
+!0 = !DIExpression(DW_OP_swap)