llvm rejects DWARF operator DW_OP_lit[1-31] in IR

[alok]

Summary

Currently only DWARF operator DW_OP_lit0 is accepted other in variants like DW_OP_lit1-31 are rejected.

Summary

llvm doesnt accept DW_OP_lit[1-31], it accepts only DW_OP_lit0.
it produces below error.
. . . .
invalid expression
!DIExpression(53, 30, 159)
. . . .
Current patch completes support for DWARF operator DW_OP_lit[1-31].

Testing

• Added unit test for validation thru llvm-dwarfdump
• check-debuginfo
• check-llvm

[probinson]

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

400–402

I know you did not introduce this, so I won't insist you change it, but LLVM style says do not use else after a continue.

llvm/test/DebugInfo/dwarfdump-litN.ll

5

Remove the -mtriple, because you are using the %llc_dwarf substitution, which will add -mtriple when necessary.

15

Typo, "whether"

37

Remove the target triple line, so the input will work for any target.

[aprantl]

I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.

[alok]

In D79190#2013650, @probinson wrote:

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

Yes correct. The C program works because clang generates "DW_OP_constu, 5", which is optimized to DW_OP_lit5, but it doesnt accept DW_OP_lit5 in IR. I missed to state that clearly in commit log. I shall update the commit log. Thanks for your review.

In D79190#2014425, @aprantl wrote:

I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.

Actually I found this while working on debugability support for flang. It looked strange that DW_OP_lit0 works but not other values. I understand that the same can be represented by DW_OP_const(u) but the smaller representation with DW_OP_lit is tempting. Supporting the same from completeness point of view as well as avoiding unnecessary issue reporting by the new front-ends emitting DW_OP_litX looked tempting.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

400–402

Thanks for pointing this out, I shall correct this.

llvm/test/DebugInfo/dwarfdump-litN.ll

5

Thanks for your comment. I shall incorporate this.

15

Thanks for pointing this out. I shall incorporate this.

37

Thanks. I shall update this in next version.

[djtodoro]

If we move forward with this, should we update the DIExpression::isConstant()?

[alok]

Updated to address comments from @probinson

[dblaikie]

In D79190#2014586, @alok wrote:

In D79190#2013650, @probinson wrote:

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR. Is that correct? If so, please make sure the commit log states that.

Yes correct. The C program works because clang generates "DW_OP_constu, 5", which is optimized to DW_OP_lit5, but it doesnt accept DW_OP_lit5 in IR. I missed to state that clearly in commit log. I shall update the commit log. Thanks for your review.

In D79190#2014425, @aprantl wrote:

I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.

The inverse argument might be that keeping the LLVM IR in a more singular/canonical form will better for a semantic representation that's expected to be handled/mutated through the optimizations, then lowering to whatever is optimal at the end.

@aprantl - when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

[aprantl]

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

[alok]

In D79190#2015441, @aprantl wrote:

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

[aprantl]

In D79190#2016252, @alok wrote:

In D79190#2015441, @aprantl wrote:

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.

[dblaikie]

In D79190#2018046, @aprantl wrote:

In D79190#2016252, @alok wrote:

In D79190#2015441, @aprantl wrote:

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.

I'd be a little against it - adding compatibility for something we were never compatible with in the first place - but not strongly enough to make/break it.

[Orlando]

In D79190#2018830, @dblaikie wrote:

In D79190#2018046, @aprantl wrote:

In D79190#2016252, @alok wrote:

In D79190#2015441, @aprantl wrote:

when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?

As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for DW_OP_lit0 in llvm/lit shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.

Should we do upgrade to other lits (1-31) also in place of rejecting them ?

I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.

I'd be a little against it - adding compatibility for something we were never compatible with in the first place - but not strongly enough to make/break it.

+1 a little against it - I don't see the appeal of moving the DIExpressions any closer to the final (DWARF) output. It's more on principle than against this specific change though; As @aprantl pointed out this case will be trivial to handle. But on the other hand, the lit0 case which already exists shows that setting a precedent could be significant, IMO.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

if (SubRegisterSizeInBits && N && (N->getOp() != dwarf::DW_OP_LLVM_fragment))
     maskSubRegister();
 
   Optional<DIExpression::ExprOperand> PrevConvertOp = None;
 
   while (ExprCursor) {
     auto Op = ExprCursor.take();
     uint64_t OpNum = Op->getOp();
 
-    if (OpNum >= dwarf::DW_OP_reg0 && OpNum <= dwarf::DW_OP_reg31) {
+    if ((OpNum >= dwarf::DW_OP_reg0 && OpNum <= dwarf::DW_OP_reg31) ||
+        (OpNum >= dwarf::DW_OP_lit0 && OpNum <= dwarf::DW_OP_lit31)) {
       emitOp(OpNum);
       continue;
-    } else if (OpNum >= dwarf::DW_OP_breg0 && OpNum <= dwarf::DW_OP_breg31) {
+    }
+
+    if (OpNum >= dwarf::DW_OP_breg0 && OpNum <= dwarf::DW_OP_breg31) {
       addBReg(OpNum - dwarf::DW_OP_breg0, Op->getArg(0));
       continue;
     }
 
     switch (OpNum) {
     case dwarf::DW_OP_LLVM_fragment: {
       unsigned SizeInBits = Op->getArg(1);
       unsigned FragmentOffset = Op->getArg(0);
while (ExprCursor) {
     case dwarf::DW_OP_div:
     case dwarf::DW_OP_mod:
     case dwarf::DW_OP_or:
     case dwarf::DW_OP_and:
     case dwarf::DW_OP_xor:
     case dwarf::DW_OP_shl:
     case dwarf::DW_OP_shr:
     case dwarf::DW_OP_shra:
-    case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
       emitOp(OpNum);
       break;
     case dwarf::DW_OP_deref:
       assert(!isRegisterLocation());
       if (!isMemoryLocation() && ::isMemoryLocation(ExprCursor))
         // Turning this into a memory location description makes the deref

llvm/lib/IR/DebugInfoMetadata.cpp

for (auto I = expr_op_begin(), E = expr_op_end(); I != E; ++I) {
     if (I->get() + I->getSize() > E->get())
       return false;
 
     uint64_t Op = I->getOp();
     if ((Op >= dwarf::DW_OP_reg0 && Op <= dwarf::DW_OP_reg31) ||
         (Op >= dwarf::DW_OP_breg0 && Op <= dwarf::DW_OP_breg31))
       return true;
 
+    if (Op >= dwarf::DW_OP_lit0 && Op <= dwarf::DW_OP_lit31)
+      continue;
+
     // Check that the operand is valid.
     switch (Op) {
     default:
       return false;
     case dwarf::DW_OP_LLVM_fragment:
       // A fragment operator must appear at the end.
       return I->get() + I->getSize() == E->get();
     case dwarf::DW_OP_stack_value: {
for (auto I = expr_op_begin(), E = expr_op_end(); I != E; ++I) {
     case dwarf::DW_OP_and:
     case dwarf::DW_OP_xor:
     case dwarf::DW_OP_shl:
     case dwarf::DW_OP_shr:
     case dwarf::DW_OP_shra:
     case dwarf::DW_OP_deref:
     case dwarf::DW_OP_deref_size:
     case dwarf::DW_OP_xderef:
-    case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
     case dwarf::DW_OP_regx:
     case dwarf::DW_OP_bregx:
       break;
     }
   }
   return true;

llvm/test/DebugInfo/dwarfdump-litN.ll

+; This test checks whether DW_OP_litN is accepted and
+; processed.
+
+; RUN: %llc_dwarf %s -O2 -filetype=obj -o %t.o
+; RUN: llvm-dwarfdump  %t.o | FileCheck %s
+
+; CHECK-LABEL:  DW_AT_name    ("var1")
+
+; 1. Test whether DW_OP_lit0 still works.
+; CHECK-LABEL:       DW_TAG_variable
+; CHECK:        DW_AT_location        (DW_OP_lit13, DW_OP_lit0, DW_OP_mul, DW_OP_stack_value)
+; CHECK-NEXT:   DW_AT_name    ("var2")
+
+; 2. Test whether DW_OP_lit5 works.
+; CHECK-LABEL:       DW_TAG_variable
+; CHECK:        DW_AT_location        (DW_OP_lit13, DW_OP_lit5, DW_OP_mul, DW_OP_stack_value)
+; CHECK-NEXT:   DW_AT_name    ("var3")
+
+; Below is the original test case this IR is generated from
+;---------------------------
+;int main() {
+;  int var1 = 10, var2 = (var1+3)*0, var3 = (var1+3)*5;
+;  int *ptr1;
+;  ptr1 = &var1;
+;  return *ptr1;
+;}
+;---------------------------
+; step 1: generate IR using -g -O2 -S -emit-llvm
+; step 2: replace DW_OP_constu, 0 with DW_OP_lit0
+; step 3: replace DW_OP_constu, 5 with DW_OP_lit5
+;---------------------------
+
+; ModuleID = 'dwarfdump-litN.c'
+source_filename = "dwarfdump-litN.c"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+; Function Attrs: norecurse nounwind readnone uwtable
+define dso_local i32 @main() local_unnamed_addr !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 10, metadata !12, metadata !DIExpression()), !dbg !17
+  call void @llvm.dbg.value(metadata i32 13, metadata !13, metadata !DIExpression(DW_OP_lit0, DW_OP_mul, DW_OP_stack_value)), !dbg !17
+  call void @llvm.dbg.value(metadata i32 13, metadata !14, metadata !DIExpression(DW_OP_lit5, DW_OP_mul, DW_OP_stack_value)), !dbg !17
+  ret i32 10, !dbg !18
+}
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!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: "dwarfdump-litN.c", directory: "/dir")
+!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: "main", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12, !13, !14, !15}
+!12 = !DILocalVariable(name: "var1", scope: !7, file: !1, line: 2, type: !10)
+!13 = !DILocalVariable(name: "var2", scope: !7, file: !1, line: 2, type: !10)
+!14 = !DILocalVariable(name: "var3", scope: !7, file: !1, line: 2, type: !10)
+!15 = !DILocalVariable(name: "ptr1", scope: !7, file: !1, line: 3, type: !16)
+!16 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!17 = !DILocation(line: 0, scope: !7)
+!18 = !DILocation(line: 5, column: 3, scope: !7)