Page MenuHomePhabricator

[Salvage] Change salvage debug info implementation to use new DW_OP_LLVM_convert where needed
AcceptedPublic

Authored by StephenTozer on Apr 26 2019, 5:40 AM.

Details

Summary

Fixes issue: https://bugs.llvm.org/show_bug.cgi?id=40645

Previously, LLVM had no functional way of performing casts inside of a DIExpression(), which made salvaging cast instructions other than Noop casts impossible. With the recent addition of DW_OP_LLVM_convert this salvaging is now possible, and so can be used to fix the attached bug as well as any cases where SExt instruction results are lost in the debugging metadata. This patch introduces this fix by expanding the salvage debug info method to cover these cases using the new operator.

Diff Detail

Event Timeline

StephenTozer created this revision.Apr 26 2019, 5:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2019, 5:40 AM
dstenb added a subscriber: dstenb.Apr 26 2019, 5:51 AM
dstenb added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1674

There is a early bail-out for ZExtInst above:

if (CI->isNoopCast(DL) || isa<ZExtInst>(&I))
  return SrcDIExpr;

so that condition in this if statement appears to be dead code as it is now.

I assume that we no longer should bail out for zexts (?).

As per the previous comment, ZExt is equivalent to a Noop on the DWARF expression stack, so can be caught by the same early exit.

StephenTozer edited the summary of this revision. (Show Details)Apr 26 2019, 6:12 AM

Have you verified that this results in the desired DW_OPs being emitted in the end and that the (or a) debugger makes sense out of it?

Since the expression stack is a bit sloppy about types it defaults to the generic type (size of address on the target machine), so without having previously set the type to something else a single DW_OP_LLVM_convert does not mean much. This is the reason that llvm::replaceAllDbgUsesWith in lib/Transforms/Utils/Local.cpp inserts a pair (the first one is simply used to set the type of the current stack element and the second one performs the actual conversion).

For now inserting a pair would probably be the easiest way forward. A bit prettier would be to introduce a DW_OP_LLVM_reinterpret op to do that job. Longer term it would be helpful if all values had a specified type immediately when they are put on the stack.

dstenb added inline comments.Apr 26 2019, 6:29 AM
llvm/lib/Transforms/Utils/Local.cpp
1674

Also, perhaps this should be inverted to an early exit instead?

A bit prettier would be to introduce a DW_OP_LLVM_reinterpret op to do that job.

I am preparing a patch for that right now. Regardless of this review it should make the existing DW_OP_LLVM_convert stuff a bit cleaner.

A bit prettier would be to introduce a DW_OP_LLVM_reinterpret op to do that job.

I am preparing a patch for that right now. Regardless of this review it should make the existing DW_OP_LLVM_convert stuff a bit cleaner.

Actually forget all I said about DW_OP_reinterpret, it turns out that I did not read the spec properly and did not pay attention to the last sentence saying that: The type of the operand and result type must have the same size in bits.

So clearly it cannot be used like I suggested.

What I said about using two DW_OP_LLVM_convert in sequence still holds though.

Updated to use convert OP correctly as suggested by markus.

aprantl added inline comments.May 2 2019, 11:26 AM
llvm/lib/Transforms/Utils/Local.cpp
1688

clang-format?

Remove incorrectly included lines.

The DW_OP_LLVM_convert stuff looks-good-to-me but I don't feel that I have authority to approve for the rest so please don't wait for any input from my part.

probinson added inline comments.May 13 2019, 10:35 AM
llvm/lib/Transforms/Utils/Local.cpp
1671

Looks to me like this 'if' does more than check for vectors?

Update comment

probinson accepted this revision.May 14 2019, 6:49 AM

LGTM after you add the full-stop noted below.

llvm/lib/Transforms/Utils/Local.cpp
1671

Needs a full-stop at the end of the sentence.

This revision is now accepted and ready to land.May 14 2019, 6:49 AM
This revision was automatically updated to reflect the committed changes.

This broke compilation of libtiff for arm, see https://bugs.llvm.org/show_bug.cgi?id=41931 for details.

Thanks for working on this, it seems very useful! Any updates?

StephenTozer reopened this revision.Aug 16 2019, 9:35 AM

Reopening, as the merged patch caused a regression issue and needed to be reverted; this update should fix said regression issue.

This revision is now accepted and ready to land.Aug 16 2019, 9:35 AM

What was the fix for the regression and is it also tested?

The regression issue is documented in the bug above: https://bugs.llvm.org/show_bug.cgi?id=41931
The main issue can be summarised briefly as "splitting a fragment DIExpression across multiple registers is currently broken", which is what I've submitted a fix for (see comments in SelectionDAGBuilder.cpp); the test for the new behaviour is in fragmented-args-multiple-regs.ll

Thanks for this (a few comments added).

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

I guess this is unwanted.

llvm/test/CodeGen/ARM/fragmented-args-multiple-regs.ll
34

We can delete this.

35

And this as well.

41

We can also update the version to 10.0.0.

48

Here as well.

llvm/test/DebugInfo/salvage-cast-debug-info.ll
21

Here as well.