Page MenuHomePhabricator

[DebugInfo] Lower dbg.declare to DBG_VALUE with DW_OP_deref
AbandonedPublic

Authored by rnk on Aug 30 2017, 2:22 PM.

Details

Reviewers
aprantl
inglorion
Summary

This is a step towards eliminating the vestigial second operand of
DBG_VALUE, which is always an immediate zero or noreg and only indicates
if the debug value is indirect in memory. Today, the offset is carried
in the DIExpression, and soon the indirection will live there too.

Many of the test case updates remove DW_OP_deref to better match the IR
that clang generates after r300523, which removed DW_OP_deref from VLAs
and indirect parameters.

Event Timeline

rnk created this revision.Aug 30 2017, 2:22 PM
aprantl edited edge metadata.Aug 30 2017, 2:51 PM

As discussed on IRC, I think that this is *generally* a good idea, but I'm skeptical about a few issues with the concrete implementation that I commented on inline.
These changes have to be made very carefully. Have you tried running the debuginfo-tests with your patch? To do this you need to checkout the debuginfo-tests repository into the clang/test directory and have either gdb or lldb installed and working (yes it's magical and works with both!).

We need to make sure that any change to the IR testcases can actually be reproduced by recompiling the original source with clang.

thanks for working on this!

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5138

Can you explain why the DW_OP_deref needs to go at the end of the expression rather than the beginning?

llvm/test/DebugInfo/X86/dbg-declare-arg.ll
12

Shouldn't this be an NFC commit?

llvm/test/DebugInfo/X86/safestack-byval.ll
18

That's a *very* different expression. Why is this correct?

88

Why the change?

llvm/test/DebugInfo/X86/vla.ll
36

I don't see a change in this patch that would have this effect.

aprantl requested changes to this revision.Aug 30 2017, 2:52 PM
This revision now requires changes to proceed.Aug 30 2017, 2:52 PM
rnk added a comment.Aug 30 2017, 5:27 PM

Thanks for the review, it seems like this is headed in the right direction. I haven't tried debuginfo-tests yet, but I'll do that and get back.

BTW, we were thinking about adding tests for cdb to debuginfo-tests. cdb is the Windows command line debugger. I doubt the existing perl script will run on Windows out of the box, but we'll figure something out.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5138

We have existing IR that looks like this:

dbg.declare(%block_params, !Var("foo"), !DIExpression(DW_OP_constu, 4, DW_OP_minus))

I *think* this means: take the pointer value in %block_params, subtract 4 from it, and look in memory to find the value.

I would expect this to be equivalent to:

dbg.value(%block_params, !Var("foo"), !DIExpression(DW_OP_constu, 4, DW_OP_minus, DW_OP_deref))

Meaning, the DIExpression is applied to the IR value, which ultimately becomes a virtual register, and then a register or spilled stack slot. We apply the DIExpression of a dbg.declare to that value to get the address of the variable, and *then* we deref once more to get the value.

Does that makes sense? This isn't an area that I'm familiar with yet, but that's what I'm thinking here.

llvm/test/DebugInfo/X86/dbg-declare-arg.ll
12

It isn't. I think we were getting wrong DBG_VALUEs in these cases. In this case, %agg.result starts in RDI and is spilled to RSP+8. Before this change, SDISel would make an indirect DBG_VALUE of a vreg copy of RDI. LiveDebugVariables doesn't do anything different if the original DBG_VALUE instruction was indirect and the vreg was spilled, it just makes an instruction like DBG_VALUE(<fi#N>, 0, ...) without adding a deref to the expression. It only propagates IsIndirect if the MachineOperand remained a register after register allocation.

llvm/test/DebugInfo/X86/safestack-byval.ll
18

It's a different expr, but I added the dwarfdump test to confirm that we emit the same final DWARF before and after this change. We still emit the location of [RBX-400].

88

I regenerated the IR with clang, and it doesn't come out with DW_OP_deref anymore. I didn't refresh the whole test case, but I could if you like. This is how I got it:

$ clang -g -S -emit-llvm -x c++ ss.c -fsanitize=safe-stack --target=x86_64-unknown-linux -O1 -o - | opt -S -safe-stack -o ss.ll
llvm/test/DebugInfo/X86/vla.ll
36

Clang no longer emits this DW_OP_deref after r300523. I renamed the old op_deref.ll test to vla2.ll, since it was really only using op_deref because clang used to use it for VLAs. vla2.ll is actually a much better test, it has dwarfdump checks, and I think those dwarf expressions are "more right" now.

Okay, thanks for the explanation. In order to make this change our highest priority should be that the contract between Clang and LLVM still works.
Secondarily, we should make an effort to ensure that LLVM IR testcases that include source code actually reflect the contract and can be reproduced by recompiling the original source with clang. If the IR changes after a patch, it may make sense to clone the test so we get both coverage of the original codepath in the backend and coverage/documentation of how frontends are expected to behave.

The contract still works if the same source code compiled with a patched clang produces the same DWARF/codeview as an unmodified clang. The debuginfo-tests are a good indicator that we're heading in the right direction. Or in other words, every time I changed the code relating to indirectness I broke one of those tests on my first attempt :-)

rnk added a comment.Aug 31 2017, 2:18 PM

Running debuginfo-tests was enlightening. My understanding of these opcodes is a lot worse than I thought it was. :( They are really impossible to understand without staring at the LLDB or GDB expression evaluation code. I'll have to think about this some more.

bjope added a subscriber: bjope.Sep 5 2017, 12:28 AM
rnk abandoned this revision.Sep 15 2017, 3:31 PM

Closing this, I now realize this was a step in the wrong direction.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2021, 4:56 AM
Herald added a subscriber: dexonsmith. · View Herald Transcript