Fix sign/zero extension in Dwarf expressions (with pseudo ops)

[markus]

Summary

Alternative to https://reviews.llvm.org/D56587

[aprantl]

Can you summarize what the advantage of the new operation as opposed to using DWARF 5 type conversions would be?

[markus]

In D57010#1366617, @aprantl wrote:

Can you summarize what the advantage of the new operation as opposed to using DWARF 5 type conversions would be?

The advantages of this approach as I see it would be

• does not require the infrastructure update to allow a debug expression reference a base_type DIE (especially the emit part of this seems hairy)
• even if we have full DW5 ops there is still the problem of when we need to deref. This pseudo-op has that baked in (which is perhaps not such a good idea after all?). Actually I think that when and where to DW_OP_deref (and DW_OP_deref_size as is required for big-endian) is the hardest part. I am trying to raise that issue on llvm-dev https://lists.llvm.org/pipermail/llvm-dev/2019-January/129522.html and hopefully it will result in some useful insight.

include/llvm/BinaryFormat/Dwarf.h

#include "llvm/BinaryFormat/Dwarf.def"
   DW_FORM_lo_user = 0x1f00, ///< Not specified by DWARF.
 };
 
 enum LocationAtom {
 #define HANDLE_DW_OP(ID, NAME, VERSION, VENDOR) DW_OP_##NAME = ID,
 #include "llvm/BinaryFormat/Dwarf.def"
   DW_OP_lo_user = 0xe0,
   DW_OP_hi_user = 0xff,
-  DW_OP_LLVM_fragment = 0x1000 ///< Only used in LLVM metadata.
+  DW_OP_LLVM_fragment = 0x1000, ///< Only used in LLVM metadata.
+  DW_OP_LLVM_magic_sext = 0x1001, ///< Only used in LLVM metadata.
+  DW_OP_LLVM_magic_zext = 0x1002 ///< Only used in LLVM metadata.
 };
 
 enum TypeKind : uint8_t {
 #define HANDLE_DW_ATE(ID, NAME, VERSION, VENDOR) DW_ATE_##NAME = ID,
 #include "llvm/BinaryFormat/Dwarf.def"
   DW_ATE_lo_user = 0x80,
   DW_ATE_hi_user = 0xff
 };

lib/BinaryFormat/Dwarf.cpp

StringRef llvm::dwarf::OperationEncodingString(unsigned Encoding) {
   default:
     return StringRef();
 #define HANDLE_DW_OP(ID, NAME, VERSION, VENDOR)                                \
   case DW_OP_##NAME:                                                           \
     return "DW_OP_" #NAME;
 #include "llvm/BinaryFormat/Dwarf.def"
   case DW_OP_LLVM_fragment:
     return "DW_OP_LLVM_fragment";
+  case DW_OP_LLVM_magic_sext:
+    return "DW_OP_LLVM_magic_sext";
+  case DW_OP_LLVM_magic_zext:
+    return "DW_OP_LLVM_magic_zext";
   }
 }
 
 unsigned llvm::dwarf::getOperationEncoding(StringRef OperationEncodingString) {
   return StringSwitch<unsigned>(OperationEncodingString)
 #define HANDLE_DW_OP(ID, NAME, VERSION, VENDOR)                                \
   .Case("DW_OP_" #NAME, DW_OP_##NAME)
 #include "llvm/BinaryFormat/Dwarf.def"
       .Case("DW_OP_LLVM_fragment", DW_OP_LLVM_fragment)
+      .Case("DW_OP_LLVM_magic_sext", DW_OP_LLVM_magic_sext)
+      .Case("DW_OP_LLVM_magic_zext", DW_OP_LLVM_magic_zext)
       .Default(0);
 }
 
 unsigned llvm::dwarf::OperationVersion(dwarf::LocationAtom Op) {
   switch (Op) {
   default:
     return 0;
 #define HANDLE_DW_OP(ID, NAME, VERSION, VENDOR)                                \

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

case dwarf::DW_OP_LLVM_fragment: {
 
       // Emit the DW_OP_piece.
       addOpPiece(SizeInBits, SubRegisterOffsetInBits);
       setSubRegisterPiece(0, 0);
       // Reset the location description kind.
       LocationKind = Unknown;
       return;
     }
+    case dwarf::DW_OP_LLVM_magic_sext: {
+      unsigned ToBits = Op->getArg(0);
+      //unsigned FromBits = Op->getArg(1);
+
+      if (LocationKind == Memory) {
+        emitOp(dwarf::DW_OP_deref_size);
+        emitUnsigned(ToBits/8);
+        LocationKind = Implicit;
+      }
+
+      // (((X >> (ToBits - 1)) * (~0)) << ToBits) | X
+      emitOp(dwarf::DW_OP_dup);
+      emitOp(dwarf::DW_OP_constu);
+      emitUnsigned(ToBits - 1);
+      emitOp(dwarf::DW_OP_shr);
+      emitOp(dwarf::DW_OP_lit0);
+      emitOp(dwarf::DW_OP_not);
+      emitOp(dwarf::DW_OP_mul);
+      emitOp(dwarf::DW_OP_constu);
+      emitUnsigned(ToBits);
+      emitOp(dwarf::DW_OP_shl);
+      emitOp(dwarf::DW_OP_or);
+      break;
+    }
+    case dwarf::DW_OP_LLVM_magic_zext: {
+      unsigned ToBits = Op->getArg(0);
+      //unsigned FromBits = Op->getArg(1);
+
+      if (LocationKind == Memory) {
+        emitOp(dwarf::DW_OP_deref_size);
+        emitUnsigned(ToBits/8);
+        // deref_size zero extends so we are done here
+        LocationKind = Implicit;
+      } else {
+        // (X & (1 << ToBits - 1))
+        emitOp(dwarf::DW_OP_constu);
+        emitUnsigned((1ULL << ToBits) - 1);
+        emitOp(dwarf::DW_OP_and);
+      }
+      break;
+    }
     case dwarf::DW_OP_plus_uconst:
       assert(LocationKind != Register);
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
     case dwarf::DW_OP_mul:

lib/IR/DebugInfoMetadata.cpp

DIExpression *DIExpression::getImpl(LLVMContext &Context,
                                     StorageType Storage, bool ShouldCreate) {
   DEFINE_GETIMPL_LOOKUP(DIExpression, (Elements));
   DEFINE_GETIMPL_STORE_NO_OPS(DIExpression, (Elements));
 }
 
 unsigned DIExpression::ExprOperand::getSize() const {
   switch (getOp()) {
   case dwarf::DW_OP_LLVM_fragment:
+  case dwarf::DW_OP_LLVM_magic_sext:
+  case dwarf::DW_OP_LLVM_magic_zext:
     return 3;
   case dwarf::DW_OP_constu:
   case dwarf::DW_OP_plus_uconst:
     return 2;
   default:
     return 1;
   }
 }
for (auto I = expr_op_begin(), E = expr_op_end(); I != E; ++I) {
     case dwarf::DW_OP_shl:
     case dwarf::DW_OP_shr:
     case dwarf::DW_OP_shra:
     case dwarf::DW_OP_deref:
     case dwarf::DW_OP_xderef:
     case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
+    case dwarf::DW_OP_LLVM_magic_sext:
+    case dwarf::DW_OP_LLVM_magic_zext:
       break;
     }
   }
   return true;
 }
 
 Optional<DIExpression::FragmentInfo>
 DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {

lib/Transforms/Utils/Local.cpp

auto SignOrZeroExt = [&](DbgVariableIntrinsic &DII) -> DbgValReplacement {
       DILocalVariable *Var = DII.getVariable();
 
       // Without knowing signedness, sign/zero extension isn't possible.
       auto Signedness = Var->getSignedness();
       if (!Signedness)
         return None;
 
       bool Signed = *Signedness == DIBasicType::Signedness::Signed;
-
-      if (!Signed) {
-        // In the unsigned case, assume that a debugger will initialize the
-        // high bits to 0 and do a no-op conversion.
-        return Identity(DII);
-      } else {
-        // In the signed case, the high bits are given by sign extension, i.e:
-        //   (To >> (ToBits - 1)) * ((2 ^ FromBits) - 1)
-        // Calculate the high bits and OR them together with the low bits.
-        SmallVector<uint64_t, 8> Ops({dwarf::DW_OP_dup, dwarf::DW_OP_constu,
-                                      (ToBits - 1), dwarf::DW_OP_shr,
-                                      dwarf::DW_OP_lit0, dwarf::DW_OP_not,
-                                      dwarf::DW_OP_mul, dwarf::DW_OP_or});
-        return DIExpression::appendToStack(DII.getExpression(), Ops);
-      }
+      SmallVector<uint64_t, 4> Ops({Signed ? dwarf::DW_OP_LLVM_magic_sext : dwarf::DW_OP_LLVM_magic_zext, ToBits, FromBits});
     };
     return rewriteDebugUsers(From, To, DomPoint, DT, SignOrZeroExt);
   }
 
   // TODO: Floating-point conversions, vectors.
   return false;
 }
 

test/Transforms/InstCombine/cast-set-preserve-signed-dbg-val.ll

 ; RUN: opt -instcombine -S < %s | FileCheck %s
 
 ; CHECK-LABEL: define {{.*}} @test5
 define i16 @test5(i16 %A) !dbg !34 {
   ; CHECK: [[and:%.*]] = and i16 %A, 15
 
   %B = sext i16 %A to i32, !dbg !40
   call void @llvm.dbg.value(metadata i32 %B, metadata !36, metadata !DIExpression()), !dbg !40
 
   %C = and i32 %B, 15, !dbg !41
   call void @llvm.dbg.value(metadata i32 %C, metadata !37, metadata !DIExpression()), !dbg !41
 
   ; Preserve the dbg.value for the DCE'd 32-bit 'and'.
   ;
   ; The high 16 bits of the original 'and' require sign-extending the new 16-bit and:
   ; CHECK-NEXT: call void @llvm.dbg.value(metadata i16 [[and]], metadata [[C:![0-9]+]],
-  ; CHECK-SAME:    metadata !DIExpression(DW_OP_dup, DW_OP_constu, 15, DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_or, DW_OP_stack_value)
+  ; CHECK-SAME:    metadata !DIExpression(DW_OP_LLVM_magic_sext, 16, 32, DW_OP_stack_value)
 
   %D = trunc i32 %C to i16, !dbg !42
   call void @llvm.dbg.value(metadata i16 %D, metadata !38, metadata !DIExpression()), !dbg !42
 
   ; The dbg.value for a truncate should simply point to the result of the 16-bit 'and'.
   ; CHECK-NEXT: call void @llvm.dbg.value(metadata i16 [[and]], metadata [[D:![0-9]+]], metadata !DIExpression())
 
   ret i16 %D, !dbg !43

unittests/Transforms/Utils/LocalTest.cpp

TEST(Local, ReplaceAllDbgUsesWith) {
   auto hasADbgVal = [&](ArrayRef<uint64_t> Ops) {
     return any_of(ADbgVals, [&](DbgValueInst *DVI) {
       assert(DVI->getVariable()->getName() == "2");
       return DVI->getExpression()->getElements() == Ops;
     });
   };
 
   // Case 1: The original expr is empty, so no deref is needed.
-  EXPECT_TRUE(hasADbgVal({DW_OP_dup, DW_OP_constu, 31, DW_OP_shr, DW_OP_lit0,
-                          DW_OP_not, DW_OP_mul, DW_OP_or, DW_OP_stack_value}));
+  EXPECT_TRUE(hasADbgVal({DW_OP_LLVM_magic_sext, 32, 64, DW_OP_stack_value}));
 
   // Case 2: Perform an address calculation with the original expr, deref it,
   // then sign-extend the result.
-  EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_deref, DW_OP_dup,
-                          DW_OP_constu, 31, DW_OP_shr, DW_OP_lit0, DW_OP_not,
-                          DW_OP_mul, DW_OP_or, DW_OP_stack_value}));
+  EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_deref,
+                         DW_OP_LLVM_magic_sext, 32, 64, DW_OP_stack_value}));
 
   // Case 3: Insert the sign-extension logic before the DW_OP_stack_value.
-  EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_dup, DW_OP_constu, 31,
-                          DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_or,
+  EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_LLVM_magic_sext, 32, 64,
                           DW_OP_stack_value}));
 
   // Cases 4-6: Just like cases 1-3, but preserve the fragment at the end.
-  EXPECT_TRUE(hasADbgVal({DW_OP_dup, DW_OP_constu, 31, DW_OP_shr, DW_OP_lit0,
-                          DW_OP_not, DW_OP_mul, DW_OP_or, DW_OP_stack_value,
-                          DW_OP_LLVM_fragment, 0, 8}));
+  EXPECT_TRUE(hasADbgVal({DW_OP_LLVM_magic_sext, 32, 64, DW_OP_stack_value,
   EXPECT_TRUE(
-      hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_deref, DW_OP_dup, DW_OP_constu,
-                  31, DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_or,
+      hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_deref, DW_OP_LLVM_magic_sext, 32, 64,
                   DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 8}));
-  EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_dup, DW_OP_constu, 31,
-                          DW_OP_shr, DW_OP_lit0, DW_OP_not, DW_OP_mul, DW_OP_or,
+  EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_LLVM_magic_sext, 32, 64,
                           DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 8}));
 
   verifyModule(*M, &errs(), &BrokenDebugInfo);
   ASSERT_FALSE(BrokenDebugInfo);
 }
 
 TEST(Local, RemoveUnreachableBlocks) {
   LLVMContext C;