Page MenuHomePhabricator

PR32382: Emit complex DWARF expressions with the correct location description kind
ClosedPublic

Authored by aprantl on Mar 28 2017, 2:09 PM.

Details

Summary

This fixes most of PR32382. It does not fix the DWARF 2 expressions yet; that part can be done as a separate, simpler follow-up commit.

The DWARF specification knows 3 kinds of non-empty simple location descriptions:

  1. Register location descriptions
    • describe a variable in a register
    • consist of only a DW_OP_reg
  2. Memory location descriptions
    • describe the address of a variable
  3. Implicit location descriptions
    • describe the value of a variable
    • end with DW_OP_stack_value & friends

The existing DwarfExpression code is pretty much ignorant of these restrictions. This used to not matter because we only emitted very short expressions that we happened to get right by accident. This patch makes DwarfExpression aware of the rules defined by the DWARF standard and now chooses the right kind of location description for each expression being emitted.

There are two major changes in this patch:

  • I had to fix the semantics of dbg.declare for describing function arguments. After this patch a dbg.declare always takes the *address* of a variable as the first argument, even if the argument is not an alloca.
  • When lowering a DBG_VALUE, the decision of whether to emit a register location description or a memory location description depends on the MachineLocation — register machine locations may get promoted to memory locations based on their DIExpression. (Future) optimization passes that want to salvage implicit debug location for variables may do so by appending a DW_OP_stack_value. For example: DBG_VALUE, [RBP-8] --> DW_OP_fbreg -8 DBG_VALUE, RAX --> DW_OP_reg0 +0 DBG_VALUE, RAX, DIExpression(DW_OP_deref) --> DW_OP_reg0 +0

All testcases that were modified were regenerated from clang. I also added source-based testcases for each of these to the debuginfo-tests repository over the last week to make sure that no synchronized bugs slip in. The debuginfo-tests compile from source and run the debugger.

https://bugs.llvm.org/show_bug.cgi?id=32382
rdar://problem/31205000

Diff Detail

Repository
rL LLVM

Event Timeline

dblaikie edited edge metadata.Mar 29 2017, 10:57 AM

There are a lot of small, careful changes in here - I might need a bit of a high level walkthrough of what the changes are and how they achieve the goal. More detail on how this changes behavior would be good. "makes LLVM aware of these DWARF restrictions" is a bit too high level.

lib/Bitcode/Reader/MetadataLoader.cpp
1536–1540 ↗(On Diff #93285)

This seems a bit complicated - any way to simplify it/make it more obvious? (or at least add comments)

Some of it looks the same as the condition in the case 0 of this switch. Could it be factored into a common function wiht a name that might aid readability?

lib/Bitcode/Writer/BitcodeWriter.cpp
1759–1760 ↗(On Diff #93285)

Where did the HasOpFragmentFlag go? Or it morphed into a version flag stored above the first bit?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
177 ↗(On Diff #93285)

So is this another "not quite DWARF expression" contract it DIExpression? (like the LLVM_fragment?)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1517 ↗(On Diff #93285)

Remove commented-out code.

There are a lot of small, careful changes in here - I might need a bit of a high level walkthrough of what the changes are and how they achieve the goal. More detail on how this changes behavior would be good. "makes LLVM aware of these DWARF restrictions" is a bit too high level.

This was unfortunately the smallest I could break the patch up into.
The observable behavior changes only for DIExpressions that have a DW_OP_deref operator *and* additional operators. The primary change is that the old code treated DW_OP_breg as if it where dereferencing the contents of the register. This happened to work if no other operations followed (because with no DW_OP_stack_value present, debuggers treat the expression as a memory location description, i.e.: deref the expression).

lib/Bitcode/Reader/MetadataLoader.cpp
1536–1540 ↗(On Diff #93285)

It doesn't have enough in common to be factored out, but perhaps adding the comment

//[DW_OP_deref, ..., DW_OP_LLVM_fragment, x, y]
// -> [..., DW_OP_deref, DW_OP_LLVM_fragment, x, y]

makes it more obvious what is happening there.

lib/Bitcode/Writer/BitcodeWriter.cpp
1759–1760 ↗(On Diff #93285)

It's rolled into the Version. I have a laundry list of future improvements to DIExpression, that will involve revving the version and are easier to handle as incremental upgrades rather than individual "features".

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
177 ↗(On Diff #93285)

Yes, this is a bit obtuse. We could also add an addAddress() call to DwarfExpression analogous to addMachineReg() that then sets the location description kind inside of DwarfExpression. We could effectively move all the code inside if (Global) into DwarfExpression::addAddress().

aprantl updated this revision to Diff 93896.Apr 3 2017, 11:24 AM

Add DIEDwarfExpression::addAddress() and DIEDwarfExpression::addThreadLocalAddress().

dblaikie accepted this revision.Apr 3 2017, 2:35 PM

Couple of minor things. Not sure I fully understand everything here, but seems plausible.

include/llvm/IR/DebugInfoMetadata.h
2252–2257 ↗(On Diff #93896)

linear algorithm for testing the last element? that doesn't seem ideal (getElements().back() instead?) (conditional on !getElements().empty())

lib/Bitcode/Reader/MetadataLoader.cpp
1536–1540 ↗(On Diff #93285)

Maybe it'd be easier if it were a std::copy?

if (deref) {
  auto End = Elts.end();
  if (Elts.size() >= 3 && std::prev(End, 3) == fragment)
    std::prev(End, 3);
  std::move(std::next(Elts.begin()), End, Elts.begin());
  *std::prev(End) = fragment;
}

Something like that?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
228–230 ↗(On Diff #93896)

Should fixme be conditional on "when using DWARF N < M" (where M is whatever version that added stack_value). Would be good to mention that in the comment if that's the case.

This revision is now accepted and ready to land.Apr 3 2017, 2:35 PM
aprantl planned changes to this revision.Apr 6 2017, 11:24 AM

Thanks a lot for reviewing this, David! While I was preparing the next updated of the corresponding clang changes in https://reviews.llvm.org/D31440 I realized that there is one use-case that is not supported by this variant. To encode locations for by-ref captures clang wants to describe a location that is at an offset inside of a larger alloca. To support this use-case I need to remove the implicit DW_OP_deref that we are adding to dbg.declares everywhere and make it explicit instead.
This will be a good change:

  • it will make a future patch that replaces all instances of dbg.declare with dbg.value very simple
  • we won't need to rewrite DIExpressions when we lower dbg.declare -> dbg.value
  • it will make the DW_OP_xderef use-case that I complain about in DwarfCompileUnit.cpp:570 appear less out-of-place.

I'll be back here with an updated (albeit somewhat longer) patch soon. Meanwhile I'm also extending the debuginfo-tests a bit to make sure I'm not breaking the integration between llvm and clang.

aprantl updated this revision to Diff 95323.Apr 14 2017, 11:39 AM
aprantl edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 14 2017, 11:39 AM
aprantl updated this revision to Diff 95356.Apr 14 2017, 3:46 PM

Addressed David's review feedback for the DW_OP_deref bitcode upgrade.

aprantl updated this revision to Diff 95503.Apr 17 2017, 3:59 PM

Added test coverage for SelectionDAG in sret.ll and ensured it matches the FastISel behavior.

aprantl requested review of this revision.Apr 17 2017, 4:20 PM
aprantl edited edge metadata.

David, the review is still marked as accepted, but I did add a couple of changes since then, so I think I should be moving it back into "Request review".
Would you mind taking a second look?

As I mentioned in the description, all IR testcases that were modified were regenerated from clang. I also added source-based testcases for each of these to the debuginfo-tests repository over the last week to make sure that no synchronized bugs slip in. The debuginfo-tests compile from source and run the debugger and are exercised by the green dragon bots, so all these changes should be safe.
thanks!
adrian

dblaikie accepted this revision.Apr 17 2017, 4:24 PM

Phab's not helping me see what the delta since the last time I accepted this are, but taking a very rough glance it seems ok - carry on :)

This revision is now accepted and ready to land.Apr 17 2017, 4:24 PM
This revision was automatically updated to reflect the committed changes.
aaboud added a subscriber: aaboud.May 25 2017, 5:14 AM

This commit caused the following bug:
https://bugs.llvm.org/show_bug.cgi?id=33166

This commit caused the following bug:
https://bugs.llvm.org/show_bug.cgi?id=33166

Seems like the issue was fixed in r301210. If that is true, then sorry for the noise.
Adrain, can you please confirm that?

Thanks