[DebugInfo] Support for signed constants inside DIExpression

[SouraVX]

Summary

Negative numbers are represented using DW_OP_consts along with signed representation
of the number as the argument.

Test case IR is generated using Fortran front-end.

[SouraVX]

Patch is stemmed from the discussions from https://reviews.llvm.org/D97311

[dstenb]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

3052 ↗

(On Diff #333015)

Why do we only care about unsigned constants here?

(And in the other cases where isConstant() is changed to isUnsignedConstant())

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

216

Can we make a isConstant() function in DIExpression that wraps these two?

llvm/lib/IR/DebugInfoMetadata.cpp

1475

Why do we not allow fragmented expressions here but we do for the unsigned variant?

[SouraVX]

Thanks for review @dstenb !

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

3052 ↗

(On Diff #333015)

Thanks for the question! As noted in the patch summary, we are trying to be conservative here.
Motivation behind this change, Previous code made a check for isConstant which sounds/seems ambiguous to me when I introduced isSignedConstant so I took the opportunity to make the change conservatively.

[dstenb]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

3052 ↗

(On Diff #333015)

Okay, thanks! So all of those changes are done conservatively, rather than signed constants being unwanted there. Can we add TODO comments that say that we should investigate if signed constants should be covered also for each of these cases?

[aprantl]

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

3236 ↗

(On Diff #333015)

Based on the assertion message though, it sounds like this assertion, for example, really wnats to check for any constants?

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

216

Yes, that would be a good idea.

[SouraVX]

Thanks @aprantl for reviewing this.
Changes:

• Rebased to tip.
• Addressed @dstenb review comments regarding covering fragments and added test case to excercise that.
• Addressed @aprantl review comments of combinining check to a single function i.e isConstant.
• Replaced most checks of isUnsignedConstant with isConstant adding appropriate test case modifications to exercise the changes.

[SouraVX]

llvm/include/llvm/IR/DebugInfoMetadata.h

2587

These functions can be cascaded/merged into isConstant function body(with multiple if statements). However keeping them orthogonal/independent has advantages of it's own.
One motivating example can be seen at llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:217 where this function call fulfill the intent of conveying the necessary info i.e signed or unsigned.

[SouraVX]

• Minor comment updation in isSignedConstant Function in DebugInfoMetadata.cpp

[jmorse]

Sounds good overall, with a comment inline about the test for isSignedConstant. IMO, there should be some test coverage of expressions that shouldn't be recognised as a signed constant too.

llvm/lib/IR/DebugInfoMetadata.cpp

1471

I may have mis-read, but shouldn't the end && on this line be ||? Either the zeroth element not being consts, *or* the second element not being stack value, would mean this doesn't match the constant pattern, no?

Additionally: shouldn't there be a clause for getNumElements() == 6 here? Otherwise, any six element expression with a fragment suffix will be identified a constant by this function, regardless of the first three elements.

[SouraVX]

Thanks for the review @jmorse!

• Addressed review comments by @jmorse.
• Reinforced checks for both isUnsignedConstant and isSignedConstant.
• Added an invalid expression test case covering isSignedConstant 6 arguments case with operand 0 and operand 2 matching.

[SouraVX]

llvm/lib/IR/DebugInfoMetadata.cpp

1471

Taken care thanks!

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1475

It's costly to maintain two function that share 95% of their implementation. What would you think about having a single function

llvm::Optional<enum {unsigned, signed}> DIExpression::isConstant() const;

instead? Then it can serve as a drop-in replacements for call sites that don't care about the signedness and ones that do still get that information.

[SouraVX]

Changes:

• Rebased to the tip.
• Addressed @aprantl review comments.

[SouraVX]

Rebased

[SouraVX]

llvm/lib/IR/DebugInfoMetadata.cpp

1475

Thanks for the suggestion! I went a little ahead and did it in a more functional way, preserving the semantics in individual lambdas.
Please let me know, WDYT ?

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1475

You implemented my suggestion very literally. Ideally we would share the *implementation* of the two lambdas as well, for example, by inverting the condition and use early exists:

if ((getNumElements() == 2 && (getElement(0) != dwarf::DW_OP_consts))
  return true;
if  (getNumElements() != 3_
  return false;
if ((getElement(0) != dwarf::DW_OP_consts || (getElement(0) != dwarf::DW_OP_constu))
  return false;
return getElement(2) != dwarf::DW_OP_stack_value);

...

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1475

should have been
if ((getElement(0) != dwarf::DW_OP_consts && (getElement(0) != dwarf::DW_OP_constu))

[SouraVX]

• Rebased & addressed @aprantl review comments.

[SouraVX]

Minor comment correction.

[SouraVX]

llvm/lib/IR/DebugInfoMetadata.cpp

1475

I've tried to follow what you meant :)
Changes:

• De-duplicated/factored out common code.
• Arranged expensive checks first, this should help in short-circuiting in case of malformed expression.

[aprantl]

llvm/lib/IR/DebugInfoMetadata.cpp

1466

signed

1468

is there a missing Ofs and ) here?

1477

Shouldn't this compare getElement(0)?

[SouraVX]

• Addressed @aprantl review comments. Thanks!.

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
 
   unsigned getNumElements() const { return Elements.size(); }
 
   uint64_t getElement(unsigned I) const {
     assert(I < Elements.size() && "Index out of range");
     return Elements[I];
   }
 
-  /// Determine whether this represents a standalone constant value.
-  bool isConstant() const;
+  /// Determine whether this represents a standalone unsigned constant value.
+  bool isUnsignedConstant() const;
 
   /// Determine whether this represents a standalone signed constant value.
   bool isSignedConstant() const;
 
+  /// Determine whether this represents a constant value.
+  bool isConstant() const;
+
   using element_iterator = ArrayRef<uint64_t>::iterator;
 
   element_iterator elements_begin() const { return getElements().begin(); }
   element_iterator elements_end() const { return getElements().end(); }
 
   /// A lightweight wrapper around an expression operand.
   ///
   /// TODO: Store arguments directly and change \a DIExpression to store a

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

void DwarfCompileUnit::addLocationAttribute(
   DIELoc *Loc = nullptr;
   Optional<unsigned> NVPTXAddressSpace;
   std::unique_ptr<DIEDwarfExpression> DwarfExpr;
   for (const auto &GE : GlobalExprs) {
     const GlobalVariable *Global = GE.Var;
     const DIExpression *Expr = GE.Expr;
 
     // For compatibility with DWARF 3 and earlier,
-    // DW_AT_location(DW_OP_constu, X, DW_OP_stack_value) becomes
+    // DW_AT_location(DW_OP_constu, X, DW_OP_stack_value) or
+    // DW_AT_location(DW_OP_consts, X, DW_OP_stack_value) becomes
     // DW_AT_const_value(X).
     if (GlobalExprs.size() == 1 && Expr && Expr->isConstant()) {
       addToAccelTable = true;
-      addConstantValue(*VariableDIE, /*Unsigned=*/true, Expr->getElement(1));
+      addConstantValue(*VariableDIE, Expr->isUnsignedConstant(),
+                       Expr->getElement(1));
       break;
     }
 
     // We cannot describe the location of dllimport'd variables: the
     // computation of their address requires loads from the IAT.
     if (Global && Global->hasDLLImportStorageClass())
       continue;
 

llvm/lib/IR/DebugInfoMetadata.cpp

Optional<DIExpression *> DIExpression::createFragmentExpression(
   }
   assert(Expr && "Unknown DIExpression");
   Ops.push_back(dwarf::DW_OP_LLVM_fragment);
   Ops.push_back(OffsetInBits);
   Ops.push_back(SizeInBits);
   return DIExpression::get(Expr->getContext(), Ops);
 }
 
-bool DIExpression::isConstant() const {
+bool DIExpression::isUnsignedConstant() const {
   // Recognize DW_OP_constu C DW_OP_stack_value (DW_OP_LLVM_fragment Len Ofs)?.
   if (getNumElements() != 3 && getNumElements() != 6)
     return false;
   if (getElement(0) != dwarf::DW_OP_constu ||
       getElement(2) != dwarf::DW_OP_stack_value)
     return false;
   if (getNumElements() == 6 && getElement(3) != dwarf::DW_OP_LLVM_fragment)
     return false;
   return true;
 }
 
 bool DIExpression::isSignedConstant() const {
-  // Recognize DW_OP_consts C
-  if (getNumElements() != 2)
+  // Recognize DW_OP_consts C and
+  // DW_OP_constu C DW_OP_stack_value (DW_OP_LLVM_fragment Len Ofs)?.
+  if (getNumElements() != 2 && getNumElements() != 3 && getNumElements() != 6)
+    return false;
+  if ((getNumElements() == 2 && getElement(0) != dwarf::DW_OP_consts) ||
+      (getNumElements() == 3 && getElement(0) != dwarf::DW_OP_consts &&
+       getElement(2) != dwarf::DW_OP_stack_value))
     return false;
-  if (getElement(0) != dwarf::DW_OP_consts)
+  if (getNumElements() == 6 && getElement(3) != dwarf::DW_OP_LLVM_fragment)
     return false;
   return true;
 }
 
+bool DIExpression::isConstant() const {
+  return isUnsignedConstant() || isSignedConstant();
+}
+
 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}};
   return Ops;
 }
 

llvm/test/DebugInfo/X86/global-constants.ll

+; RUN: llc %s -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
+
+; CHECK-DAG-LABEL:  DW_AT_name ("negconstant")
+; CHECK:  DW_AT_const_value (-1)
+; CHECK-DAG-LABEL:  DW_AT_name ("negconstant2")
+; CHECK:  DW_AT_const_value (-2)
+; CHECK-DAG-LABEL:  DW_AT_name ("posconstant")
+; CHECK:  DW_AT_const_value (1)
+; CHECK-DAG-LABEL:  DW_AT_name ("posconstant1")
+; CHECK:  DW_AT_const_value (2)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!13, !14, !15, !16}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !3)
+!1 = !DIFile(filename: "globalconst.c", directory: "/")
+!2 = !{}
+!3 = !{!4, !7, !9, !11}
+!4 = !DIGlobalVariableExpression(var: !5, expr: !DIExpression(DW_OP_consts, 18446744073709551615, DW_OP_stack_value))
+!5 = distinct !DIGlobalVariable(name: "negconstant", scope: !0, file: !1, line: 2, type: !6, isLocal: false, isDefinition: true)
+!6 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!7 = !DIGlobalVariableExpression(var: !8, expr: !DIExpression(DW_OP_consts, 18446744073709551614, DW_OP_stack_value))
+!8 = distinct !DIGlobalVariable(name: "negconstant2", scope: !0, file: !1, line: 3, type: !6, isLocal: false, isDefinition: true)
+!9 = !DIGlobalVariableExpression(var: !10, expr: !DIExpression(DW_OP_consts, 1, DW_OP_stack_value))
+!10 = distinct !DIGlobalVariable(name: "posconstant", scope: !0, file: !1, line: 2, type: !6, isLocal: false, isDefinition: true)
+!11 = !DIGlobalVariableExpression(var: !12, expr: !DIExpression(DW_OP_consts, 2, DW_OP_stack_value))
+!12 = distinct !DIGlobalVariable(name: "posconstant2", scope: !0, file: !1, line: 3, type: !6, isLocal: false, isDefinition: true)
+!13 = !{i32 2, !"Dwarf Version", i32 4}
+!14 = !{i32 2, !"Debug Info Version", i32 3}
+!15 = !{i32 1, !"wchar_size", i32 4}
+!16 = !{i32 7, !"PIC Level", i32 2}

llvm/test/DebugInfo/X86/stack-value-dwarf4.ll

 ; RUN: llc -o - %s | FileCheck --check-prefix=CHECK-DWARF2 %s
 ; RUN: llc -dwarf-version=4 -o - %s | FileCheck --check-prefix=CHECK-DWARF4 %s
 
 ; Exercise DW_OP_stack_value on global constants.
 
 target datalayout = "e-p:64:64"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; CHECK-DWARF2: .byte   6                       # DW_AT_location
 ; CHECK-DWARF2-NEXT: .byte   52
 ; CHECK-DWARF2-NEXT: .byte   147
 ; CHECK-DWARF2-NEXT: .byte   2
 ; CHECK-DWARF2-NEXT: .byte   48
 ; CHECK-DWARF2-NEXT: .byte   147
 ; CHECK-DWARF2-NEXT: .byte   2
+; CHECK-DWARF2: .byte   8                       # DW_AT_location
+; CHECK-DWARF2-NEXT: .byte   17
+; CHECK-DWARF2-NEXT: .byte   127
+; CHECK-DWARF2-NEXT: .byte   147
+; CHECK-DWARF2-NEXT: .byte   2
+; CHECK-DWARF2-NEXT: .byte   17
+; CHECK-DWARF2-NEXT: .byte   126
+; CHECK-DWARF2-NEXT: .byte   147
+; CHECK-DWARF2-NEXT: .byte   2
+; CHECK-DWARF2-NEXT: .byte   0          # End Of Children Mark
 
 ; CHECK-DWARF4: .byte   8                       # DW_AT_location
 ; CHECK-DWARF4-NEXT:.byte   52
 ; CHECK-DWARF4-NEXT:.byte   159
 ; CHECK-DWARF4-NEXT:.byte   147
 ; CHECK-DWARF4-NEXT:.byte   2
 ; CHECK-DWARF4-NEXT:.byte   48
 ; CHECK-DWARF4-NEXT:.byte   159
 ; CHECK-DWARF4-NEXT:.byte   147
 ; CHECK-DWARF4-NEXT:.byte   2
+; CHECK-DWARF4: .byte   10                      # DW_AT_location
+; CHECK-DWARF4-NEXT: .byte   17
+; CHECK-DWARF4-NEXT: .byte   127
+; CHECK-DWARF4-NEXT: .byte   159
+; CHECK-DWARF4-NEXT: .byte   147
+; CHECK-DWARF4-NEXT: .byte   2
+; CHECK-DWARF4-NEXT: .byte   17
+; CHECK-DWARF4-NEXT: .byte   126
+; CHECK-DWARF4-NEXT: .byte   159
+; CHECK-DWARF4-NEXT: .byte   147
+; CHECK-DWARF4-NEXT: .byte   2
+; CHECK-DWARF4-NEXT: .byte   0          # End Of Children Mark
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, producer: "clang", file: !4, globals: !1, emissionKind: FullDebug)
-!1 = !{!2, !10}
+!1 = !{!2, !10, !11, !14}
 !2 = !DIGlobalVariableExpression(var: !8, expr: !3)
 !3 = !DIExpression(DW_OP_constu, 4, DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 16)
 !4 = !DIFile(filename: "<stdin>", directory: "/")
 !5 = !DIBasicType(name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
 !6 = !{i32 2, !"Dwarf Version", i32 2}
 !7 = !{i32 2, !"Debug Info Version", i32 3}
 !8 = distinct !DIGlobalVariable(name: "a", scope: null, isLocal: false, isDefinition: true, type: !5)
 !9 = !DIExpression(DW_OP_constu, 0, DW_OP_stack_value, DW_OP_LLVM_fragment, 16, 16)
 !10 = !DIGlobalVariableExpression(var: !8, expr: !9)
+!11 = !DIGlobalVariableExpression(var: !13, expr: !12)
+!12 = !DIExpression(DW_OP_consts, 18446744073709551615, DW_OP_stack_value, DW_OP_LLVM_fragment, 0, 16)
+!13 = distinct !DIGlobalVariable(name: "b", scope: null, isLocal: false, isDefinition: true, type: !5)
+!14 = !DIGlobalVariableExpression(var: !13, expr: !15)
+!15 = !DIExpression(DW_OP_consts, 18446744073709551614, DW_OP_stack_value, DW_OP_LLVM_fragment, 16, 16)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!6, !7}

llvm/test/Transforms/StripSymbols/strip-dead-debug-info.ll

 ; RUN: opt -strip-dead-debug-info -verify %s -S | FileCheck %s
 ; RUN: opt -passes='strip-dead-debug-info,verify' %s -S | FileCheck %s
 
 ; CHECK: ModuleID = '{{.*}}'
 ; CHECK-NOT: "bar"
 ; CHECK-NOT: "abcd"
 ; CHECK-NOT: "GCC"
 ; CHECK: "Globals"
 ; CHECK: "abcd2"
+; CHECK: "abcd3"
 
 source_filename = "test/Transforms/StripSymbols/strip-dead-debug-info.ll"
 
 @xyz = global i32 2, !dbg !0
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.value(metadata, metadata, metadata) #0
 

 !17 = !{!3, !3}
 !18 = !DILocalVariable(name: "i", arg: 1, scope: !15, file: !2, line: 7, type: !3)
 !19 = !DIExpression()
 !20 = !DILocation(line: 7, scope: !15)
 !21 = !DILocation(line: 10, scope: !22)
 !22 = distinct !DILexicalBlock(scope: !15, file: !2, line: 7)
 !23 = distinct !DICompileUnit(language: DW_LANG_C89, file: !2, producer: "GCC", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, globals: !5)
 !24 = distinct !DICompileUnit(language: DW_LANG_C89, file: !2, producer: "Globals", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, globals: !25)
-!25 = !{!26}
+!25 = !{!26, !31}
 !26 = !DIGlobalVariableExpression(var: !27, expr: !DIExpression(DW_OP_constu, 0, DW_OP_stack_value))
 !27 = !DIGlobalVariable(name: "abcd2", scope: !2, file: !2, line: 2, type: !3, isLocal: true, isDefinition: true)
 !28 = distinct !DICompileUnit(language: DW_LANG_C89, file: !2, producer: "InlineTest", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !5, retainedTypes: !5, globals: !5)
 !29 = distinct !DISubprogram(name: "inlinefunc", linkageName: "inlinefunc", scope: null, file: !2, line: 7, type: !16, isLocal: false, isDefinition: true, isOptimized: true, unit: !28)
 !30 = !DILocation(line: 100, scope: !29, inlinedAt: !21)
+!31 = !DIGlobalVariableExpression(var: !32, expr: !DIExpression(DW_OP_consts, 18446744073709551615, DW_OP_stack_value))
+!32 = !DIGlobalVariable(name: "abcd3", scope: !2, file: !2, line: 3, type: !3, isLocal: true, isDefinition: true)