DW_OP_const* is invalid as a location

[kromanova]

Summary

This is a fix for BZ #21776.
http://llvm.org/bugs/show_bug.cgi?id=21776

Before DW_OP_stack_value was introduced in DWARF4, llvm relied on heuristics to disambiguate the value vs. location status of the expression DW_OP_consts <const>.

The proper (compliant with DWARF) way to describe a location where a value is constant is "DW_OP_constu <const>, DW_OP_stack_value”.

I pushed DW_OP_stack_value on the stack, where DW_OP_consts or DW_OP_constu were used to describe location description with the DWARF expression that represents the actual value of the object rather than its location.

[dblaikie]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1721

Should this just go outside the if/else, so it's written once rather than twice?

1721

Do we need to conditionalize this based on whether we're emitting DWARF >= 4? And just leave it the old/broken way in DWARF < 4 for old consumers to keep guessing the way they have before (like LLDB)?

test/DebugInfo/incorrect-variable-debugloc1.ll

15

Can this example be simplified further? (do we need the loop, etc?)

[echristo]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1721

Seems the most reasonable thing to do, yes.

[friss]

Did you confirm that the OP_stack_value is correctly handled by lldb and that this produces no regression?

And I agree with others that this should be conditional on Dwarf 4 output. This gives consumers at least some coarse grain switch to know how to interpret expressions.

[kromanova]

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1721

Reasonable :)

1721

Good point. I will add the condition. When I submit a bug against the LLDB, I will make a note that "old/broken" expressions will still be generated for < DWARF 4.

test/DebugInfo/incorrect-variable-debugloc1.ll

15

I will simplify the test as much as I can.
BTW, thank you David for reviewing the patch so quickly!

15

Is there an elegant solution to test different Dwarf versions (Dwarf-2, Dwarf-3 and Dwarf-4) using .ll file, while avoiding creation of multiple copies of the same test with only one line difference in metadata representing Dwarf Version.

!12 = metadata !{i32 2, metadata !"Dwarf Version", i32 4} needed for dwarf-4 test
!12 = metadata !{i32 2, metadata !"Dwarf Version", i32 2} needed for dwarf-2 test

Or should I create a .cpp test instead of .ll test to avoid duplication of the test?

[dblaikie]

test/DebugInfo/incorrect-variable-debugloc1.ll

15

Yep - check the other test cases that have "-dwarf-version" in the llc command line.

[kromanova]

I have updated the patch based on Eric, David, Paul and Frédéric's reviews.

Using a small testcase, I have checked that LLDB works as expected (at least on Unix) with an object file compiled at -gdwarf-4( new way) and an object file compiled at -gdwarf-3, -gdwarf-2 (old way). Is there anything else I need to modify/check?

[friss]

This is mostly OK, just some small nits:

test/DebugInfo/incorrect-variable-debugloc1.ll

8

I'd say 'doesn't describe a constant value, but a value at a constant address'. AFAIK, the location is valid, it's just not what we want to describe :-)

28–29

We usually edit these out when the testcase is target independent (And otherwise it should go to a target specific directory)

[kromanova]

Addressed Fred's comments.

[dblaikie]

test/DebugInfo/incorrect-variable-debugloc1.ll

23

You probably need a {{$}} at the end of this line, otherwise it'll pass even if we emit the DWARF4 form here (test this by removing the conditional in your fix and observing that this test doesn't fail where it should)

[kromanova]

Please ignore the previous revision. I accidentally uploaded an old diff.

[kromanova]

Modified testcase based on David's comment.
Verified what Dave have asked; i.e that after removing the conditional in DwarfDebug.cpp (i.e causing it always generate DWARF-4) the test fails where it is expected to fail (in DWARF23: check).

[dblaikie]

Looks good, apart from the minor comment improvement.

test/DebugInfo/incorrect-variable-debugloc1.ll

7

Usually we write this as "PR21176" (rather than "BZ #21176").

[kromanova]

test/DebugInfo/incorrect-variable-debugloc1.ll

7

I will make this change in the comment before I commit. I guess, It might make the grep more consistent.
I don't see any benefit in uploading one more review for this reason only.

Is it OK to commit or should I wait to make sure that Fred and Eric don't have any additional comments?
Katya.

23

You are right. I'll fix it.

[dblaikie]

• msg-497-82.txt157 BDownload

[kromanova]

Buildbots spotted a failure for PowerPC platform for the incorrect-variable-debugloc1.ll test that I added.
Uploading a diff once again.

(1) Eric asked me to add a comment with the explanation in the source file (for the patch itself). Done.
(2) David asked me to change BZ 21776 into PR21776 in the testcase's comment. Done.
(3) I added XFAIL condition (and a comment) for PowerPC platform.

Does it look good?

[echristo]

Couple of comments inline, otherwise LGTM.

-eric

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1725

80-col?

test/DebugInfo/incorrect-variable-debugloc1.ll

4

"Temporarily marking this test as XFAIL..."

12

PR...

[kromanova]

Addressed Eric's comments.

[kromanova]

test/DebugInfo/incorrect-variable-debugloc1.ll

4

Overlooked "marking" comment. If everything else is OK, I will change it before I commit and not going to upload one more review.

[echristo]

Yep. LGTM with the comment wording change.

-eric

[Eugene.Zelenko]

Looks like patch was not committed.

[kromanova]

It was committed in r224098. However, I don't see traces of it anymore. I need to track down if it was moved, deleted or rewritten.
Katya.

[kromanova]

This code was removed in 225734. I will subscribe Adrian since he did this change.

[kromanova]

I just looked around more carefully.
DwarfExpression::AddSignedConstant and DwarfExpression::AddUnsignedConstant call DwarfExpression::AddStackValue(), which adds "DW_OP_stack_value".
I think the logic is still there, but was just moved/re-factored.

The test was moved to llvm/test/DebugInfo/Generic/incorrect-variable-debugloc1.ll

[kromanova]

Closed by commit r224098.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

       Streamer.EmitInt8(dwarf::DW_OP_constu, "DW_OP_constu");
       Streamer.EmitULEB128(Value.getInt());
     }
+    // The proper way to describe a constant value is 
+    // DW_OP_constu <const>, DW_OP_stack_value. 
+    // Unfortunately, DW_OP_stack_value was not available until DWARF-4,
+    // so we will continue to generate DW_OP_constu <const> for DWARF-2
+    // and DWARF-3. Technically, this is incorrect since DW_OP_const <const>
+    // actually describes a value at a constant addess, not a constant value. 
+    // However, in the past there was no better way  to describe a constant 
+    // value, so the producers and consumers started to rely on heuristics 
+    // to disambiguate the value vs. location status of the expression. 
+    // See PR21176 for more details.
+    if (getDwarfVersion() >= 4)
+      Streamer.EmitInt8(dwarf::DW_OP_stack_value, "DW_OP_stack_value");
   } else if (Value.isLocation()) {
     MachineLocation Loc = Value.getLoc();
     DIExpression Expr = Value.getExpression();

test/DebugInfo/incorrect-variable-debugloc1.ll

+; REQUIRES: object-emission
+; This test is failing for powerpc64, because a location list for the
+; variable 'c' is not generated at all. Temporary making this test to XFAIL 
+; for powerpc, until PR21881 is fixed.
+; XFAIL: powerpc64
+
+; RUN: %llc_dwarf -O2  -dwarf-version 2 -filetype=obj < %s | llvm-dwarfdump - | FileCheck %s  --check-prefix=DWARF23
+; RUN: %llc_dwarf -O2  -dwarf-version 3 -filetype=obj < %s | llvm-dwarfdump - | FileCheck %s  --check-prefix=DWARF23
+; RUN: %llc_dwarf -O2  -dwarf-version 4 -filetype=obj < %s | llvm-dwarfdump - | FileCheck %s  --check-prefix=DWARF4
+
+; This is a test for PR21176.
+; DW_OP_const <const> doesn't describe a constant value, but a value at a constant address. 
+; The proper way to describe a constant value is DW_OP_constu <const>, DW_OP_stack_value.
+
+; Generated with clang -S -emit-llvm -g -O2 test.cpp
+
+; extern int func();
+; 
+; int main()
+; {
+;   volatile int c = 13;
+;   c = func();
+;   return c;
+; }
+
+; DWARF23: Location description: 10 0d {{$}}
+; DWARF4: Location description: 10 0d 9f
+
+; Function Attrs: uwtable
+define i32 @main() #0 {
+entry:
+  %c = alloca i32, align 4
+  tail call void @llvm.dbg.value(metadata !15, i64 0, metadata !10, metadata !16), !dbg !17
+  store volatile i32 13, i32* %c, align 4, !dbg !18
+  %call = tail call i32 @_Z4funcv(), !dbg !19
+  tail call void @llvm.dbg.value(metadata !{i32 %call}, i64 0, metadata !10, metadata !16), !dbg !17
+  store volatile i32 %call, i32* %c, align 4, !dbg !19
+  tail call void @llvm.dbg.value(metadata !{i32* %c}, i64 0, metadata !10, metadata !16), !dbg !17
+  %c.0.c.0. = load volatile i32* %c, align 4, !dbg !20
+  ret i32 %c.0.c.0., !dbg !20
+}
+
+declare i32 @_Z4funcv() #1
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #2
+
+attributes #0 = { uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind readnone }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!12, !13}
+!llvm.ident = !{!14}
+
+!0 = metadata !{metadata !"0x11\004\00clang version 3.6.0 (trunk 223522)\001\00\000\00\001", metadata !1, metadata !2, metadata !2, metadata !3, metadata !2, metadata !2} ; [ DW_TAG_compile_unit ] [/home/kromanova/ngh/ToT_latest/llvm/test/DebugInfo/test.cpp] [DW_LANG_C_plus_plus]
+!1 = metadata !{metadata !"test.cpp", metadata !"/home/kromanova/ngh/ToT_latest/llvm/test/DebugInfo"}
+!2 = metadata !{}
+!3 = metadata !{metadata !4}
+!4 = metadata !{metadata !"0x2e\00main\00main\00\003\000\001\000\000\00256\001\004", metadata !1, metadata !5, metadata !6, null, i32 ()* @main, null, null, metadata !9} ; [ DW_TAG_subprogram ] [line 3] [def] [scope 4] [main]
+!5 = metadata !{metadata !"0x29", metadata !1}    ; [ DW_TAG_file_type ] [/home/kromanova/ngh/ToT_latest/llvm/test/DebugInfo/test.cpp]
+!6 = metadata !{metadata !"0x15\00\000\000\000\000\000\000", null, null, null, metadata !7, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
+!7 = metadata !{metadata !8}
+!8 = metadata !{metadata !"0x24\00int\000\0032\0032\000\000\005", null, null} ; [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
+!9 = metadata !{metadata !10}
+!10 = metadata !{metadata !"0x100\00c\005\000", metadata !4, metadata !5, metadata !11} ; [ DW_TAG_auto_variable ] [c] [line 5]
+!11 = metadata !{metadata !"0x35\00\000\000\000\000\000", null, null, metadata !8} ; [ DW_TAG_volatile_type ] [line 0, size 0, align 0, offset 0] [from int]
+!12 = metadata !{i32 2, metadata !"Dwarf Version", i32 2}
+!13 = metadata !{i32 2, metadata !"Debug Info Version", i32 2}
+!14 = metadata !{metadata !"clang version 3.6.0 (trunk 223522)"}
+!15 = metadata !{i32 13}
+!16 = metadata !{metadata !"0x102"}               ; [ DW_TAG_expression ]
+!17 = metadata !{i32 5, i32 16, metadata !4, null}
+!18 = metadata !{i32 5, i32 3, metadata !4, null}
+!19 = metadata !{i32 6, i32 7, metadata !4, null}
+!20 = metadata !{i32 7, i32 3, metadata !4, null}
+