[DebugInfo] Use DW_OP_LLVM_convert_generic after sign/zero exts

[dstenb]

Summary

This is a follow-up to D76145.

DW_OP_LLVM_convert is used to do sign- and zero extensions in
DIExpressions. When using DWARFv5, that LLVM operation is translated
into DWARFv5's DW_OP_convert operation.

DWARFv5 section 2.5.1.4 specifies the following for arithmetical and
logical operations:

"Operands of an operation with two operands must have the same type,
either the same base type or the generic type."

So when we have used DW_OP_LLVM_convert in an expression, we need to
make sure that any arithmetical and logical operations operating on that
element have operands of the same type. As we currently don't have a
good interface for detecting what type is on top of the stack, and we
add arithmetical and logical operations, e.g. appending expressions like
{DW_OP_constu 1, DW_OP_plus}, in many places, this patch attempts to
alleviate such cases by introduced a new operation,
DW_OP_LLVM_convert_generic, which converts the top of the stack back to
the generic type.

This operation is appended to the sign/zero extension expression that
DIExpression::getExtOps() produces.

[aprantl]

Mechanically LGTM to me. I'm not sure if this shouldn't be done in DwarfExpression instead.

[dstenb]

In D76146#1922222, @aprantl wrote:

Mechanically LGTM to me. I'm not sure if this shouldn't be done in DwarfExpression instead.

So that we basically insert a DW_OP_convert 0x0 after each chain of DW_OP_convert operations in the input DIExpression? That sounds reasonable, and it should have a lot smaller impact on the code. I wonder though if it would be limiting to only have the DwarfExpression world type ware if/when we later want on to support more base type-based operations (e.g. DW_OP_const_type), but perhaps not?

[vsk]

It seems sensible to limit conversions back to the generic type as much as possible, so this approach lgtm.

[mib]

Hi @dstenb, any update on this ? I tried to look for DW_OP_LLVM_convert_generic in the codebase but couldn't find any occurence ... Thanks.

[dstenb]

In D76146#2438388, @mib wrote:

Hi @dstenb, any update on this ? I tried to look for DW_OP_LLVM_convert_generic in the codebase but couldn't find any occurence ... Thanks.

Hi! Sorry for the radio silence here!

I did not land this since this would break sign-extended cases like this:

intX_t local = (intX_t)signed_val_with_smaller_width;

where intX_t is an integer that is wider than the generic type in DWARF. That is, we would incorrectly truncate the value for such cases.

I guess one solution to that would be to be more conservative in adding DW_OP_LLVM_convert_generic in the DIExpression expressions, and avoid adding that when no operation will be applied on the sign-extended value, but we don't really know if more operations will be added later on when adding the convert expression to DIExpression, so I guess we instead would want to insert those generic conversions when appending operations to the DIExpression. Another solution could be to postpone the emission of the generic convert operations to DwarfExpression, and only do that there when required, as @aprantl suggests.

llvm/include/llvm/IR/DebugInfoMetadata.h

static int fragmentCmp(const FragmentInfo &A, const FragmentInfo &B) {
     if (r1 <= l2)
       return -1;
     else if (r2 <= l1)
       return 1;
     else
       return 0;
   }
 
-  using ExtOps = std::array<uint64_t, 6>;
+  using ExtOps = std::array<uint64_t, 7>;
 
   /// Returns the ops for a zero- or sign-extension in a DIExpression.
+  /// Appends a DW_OP_LLVM_convert_generic operation to convert the
+  /// result back to the generic type.
   static ExtOps getExtOps(unsigned FromSize, unsigned ToSize, bool Signed);
 
-  /// Append a zero- or sign-extension to \p Expr. Converts the expression to a
-  /// stack value if it isn't one already.
+  /// Append a zero- or sign-extension to \p Expr. Converts the extended result
+  /// back to the generic type. Converts the expression to a stack value if it
+  /// isn't one already.
   static DIExpression *appendExt(const DIExpression *Expr, unsigned FromSize,
                                  unsigned ToSize, bool Signed);
 
   /// Check if fragments overlap between a pair of FragmentInfos.
   static bool fragmentsOverlap(const FragmentInfo &A, const FragmentInfo &B) {
     return fragmentCmp(A, B) == 0;
   }
 

llvm/lib/IR/DebugInfoMetadata.cpp

if (getNumElements() == 6 && getElement(3) != dwarf::DW_OP_LLVM_fragment)
     return false;
   return true;
 }
 
 DIExpression::ExtOps DIExpression::getExtOps(unsigned FromSize, unsigned ToSize,
                                              bool Signed) {
   dwarf::TypeKind TK = Signed ? dwarf::DW_ATE_signed : dwarf::DW_ATE_unsigned;
   DIExpression::ExtOps Ops{{dwarf::DW_OP_LLVM_convert, FromSize, TK,
-                            dwarf::DW_OP_LLVM_convert, ToSize, TK}};
+                            dwarf::DW_OP_LLVM_convert, ToSize, TK,
+                            dwarf::DW_OP_LLVM_convert_generic}};
   return Ops;
 }
 
 DIExpression *DIExpression::appendExt(const DIExpression *Expr,
                                       unsigned FromSize, unsigned ToSize,
                                       bool Signed) {
   return appendToStack(Expr, getExtOps(FromSize, ToSize, Signed));
 }

llvm/test/DebugInfo/MIR/X86/dbgcall-site-two-fwd-reg-defs.mir

 
 # Verify that call site entries are emitted for both parameters in the call to
 # %call. Both parameters should be described by the preserved $ebx register,
 # and the value for the first parameter (passed in $rdi) should be
 # sign-extended to 64 bits.
 
 # CHECK: DW_TAG_call_site_parameter
 # CHECK-NEXT: DW_AT_location      (DW_OP_reg5 RDI)
-# CHECK-NEXT: DW_AT_call_value    (DW_OP_breg3 RBX+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_convert ({{.*}}) "DW_ATE_signed_32", DW_OP_convert ({{.*}}) "DW_ATE_signed_64")
+# CHECK-NEXT: DW_AT_call_value    (DW_OP_breg3 RBX+0, DW_OP_constu 0xffffffff, DW_OP_and, DW_OP_convert ({{.*}}) "DW_ATE_signed_32", DW_OP_convert ({{.*}}) "DW_ATE_signed_64", DW_OP_convert 0x0)
 #
 # CHECK: DW_TAG_call_site_parameter
 # CHECK-NEXT: DW_AT_location      (DW_OP_reg4 RSI)
 # CHECK-NEXT: DW_AT_call_value    (DW_OP_breg3 RBX+0)
 

llvm/test/Transforms/EarlyCSE/debug-info-undef.ll

 ; RUN: opt -S %s -early-cse | FileCheck %s
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 
 @a = global i8 25, align 1, !dbg !0
 
 define signext i16 @b() !dbg !12 {
 entry:
   call void @llvm.dbg.value(metadata i16 23680, metadata !17, metadata !DIExpression()), !dbg !18
   %0 = load i8, i8* @a, align 1, !dbg !19, !tbaa !20
   %conv = sext i8 %0 to i16, !dbg !19
 
-; CHECK: call void @llvm.dbg.value(metadata i8 %0, metadata !17, metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_stack_value)), !dbg !18
+; CHECK: call void @llvm.dbg.value(metadata i8 %0, metadata !17, metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert_generic, DW_OP_stack_value)), !dbg !18
 ; CHECK-NEXT:  call i32 (...) @optimize_me_not()
 
   call void @llvm.dbg.value(metadata i16 %conv, metadata !17, metadata !DIExpression()), !dbg !18
   %call = call i32 (...) @optimize_me_not(), !dbg !23
   %1 = load i8, i8* @a, align 1, !dbg !24, !tbaa !20
   %conv1 = sext i8 %1 to i16, !dbg !24
   ret i16 %conv1, !dbg !25
 }

llvm/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_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)
+  ; CHECK-SAME:    metadata !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert_generic, 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

llvm/test/Transforms/InstCombine/dbg-sign-ext-to-generic.ll

+; RUN: opt -instcombine -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Based on the following C reproducer:
+;
+; short foo(short param) {
+;  int local = param - 123;
+;  return param;
+; }
+
+; Verify that a DW_OP_LLVM_convert_generic operation is emitted after the
+; sign-extension operations, so that the result has the same type as the other
+; DW_OP_minus operand.
+
+; CHECK: call void @llvm.dbg.value(metadata i16 %param, metadata {{.*}}, metadata !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert_generic, DW_OP_constu, 123, DW_OP_minus, DW_OP_stack_value))
+
+; Function Attrs: nounwind uwtable
+define dso_local signext i16 @foo(i16 signext %param) local_unnamed_addr #0 !dbg !7 {
+entry:
+  %conv = sext i16 %param to i32, !dbg !15
+  call void @llvm.dbg.value(metadata i32 %conv, metadata !12, metadata !DIExpression(DW_OP_constu, 123, DW_OP_minus, DW_OP_stack_value)), !dbg !14
+  ret i16 %param, !dbg !16
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { nounwind uwtable }
+attributes #1 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "ext.c", directory: "/")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 11.0.0"}
+!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10, !10}
+!10 = !DIBasicType(name: "short", size: 16, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DILocalVariable(name: "local", scope: !7, file: !1, line: 2, type: !13)
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 0, scope: !7)
+!15 = !DILocation(line: 2, scope: !7)
+!16 = !DILocation(line: 3, scope: !7)

llvm/test/Transforms/InstCombine/unavailable-debug.ll

 ; RUN: opt < %s -instcombine -S | FileCheck %s
 
 ; Make sure to update the debug value after dead code elimination.
 ; CHECK: %call = call signext i8 @b(i32 6), !dbg !39
-; CHECK-NEXT: call void @llvm.dbg.value(metadata i8 %call, metadata !30, metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !38
+; CHECK-NEXT: call void @llvm.dbg.value(metadata i8 %call, metadata !30, metadata !DIExpression(DW_OP_LLVM_convert, 8, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_LLVM_convert_generic, DW_OP_stack_value)), !dbg !38
 
 @e = common local_unnamed_addr global i8 0, align 1, !dbg !0
 @c = common local_unnamed_addr global i32 0, align 4, !dbg !6
 @d = common local_unnamed_addr global i32 0, align 4, !dbg !10
 
 define signext i8 @b(i32 %f) local_unnamed_addr #0 !dbg !18 {
 entry:
   call void @llvm.dbg.value(metadata i32 %f, metadata !22, metadata !DIExpression()), !dbg !23

llvm/unittests/Transforms/Utils/LocalTest.cpp

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_LLVM_convert, 32, DW_ATE_signed,
                          DW_OP_LLVM_convert, 64, DW_ATE_signed,
+                         DW_OP_LLVM_convert_generic,
                          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_LLVM_convert, 32, DW_ATE_signed,
                          DW_OP_LLVM_convert, 64, DW_ATE_signed,
+                         DW_OP_LLVM_convert_generic,
                          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_LLVM_convert, 32,
                          DW_ATE_signed, DW_OP_LLVM_convert, 64, DW_ATE_signed,
+                         DW_OP_LLVM_convert_generic,
                          DW_OP_stack_value}));
 
   // Cases 4-6: Just like cases 1-3, but preserve the fragment at the end.
   EXPECT_TRUE(hasADbgVal({DW_OP_LLVM_convert, 32, DW_ATE_signed,
                          DW_OP_LLVM_convert, 64, DW_ATE_signed,
+                         DW_OP_LLVM_convert_generic,
                          DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 8}));
 
   EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_deref,
                          DW_OP_LLVM_convert, 32, DW_ATE_signed,
                          DW_OP_LLVM_convert, 64, DW_ATE_signed,
+                         DW_OP_LLVM_convert_generic,
                          DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 8}));
 
   EXPECT_TRUE(hasADbgVal({DW_OP_lit0, DW_OP_mul, DW_OP_LLVM_convert, 32,
                          DW_ATE_signed, DW_OP_LLVM_convert, 64, DW_ATE_signed,
+                         DW_OP_LLVM_convert_generic,
                          DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 8}));
 
   verifyModule(*M, &errs(), &BrokenDebugInfo);
   ASSERT_FALSE(BrokenDebugInfo);
 }
 
 TEST(Local, RemoveUnreachableBlocks) {
   LLVMContext C;