Page MenuHomePhabricator

[DwarfDebug] Dump call site debug info into DWARF
AcceptedPublic

Authored by djtodoro on Apr 15 2019, 8:57 AM.

Details

Summary

Dump DWARF information about call sites and call site parameters into debug info sections.

The patch also provides an interface for interpretation of instructions that could load values of call site parameters in order to generate DWARF info about call site parameters.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aprantl added inline comments.Apr 22 2019, 10:30 AM
include/llvm/CodeGen/TargetInstrInfo.h
1698

Should this return an optional DIExpression? Is Op also an out parameter?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
560

Could we include what is being analyzed in the function name?

621

clang-format

655

Process

djtodoro marked 4 inline comments as done.Apr 24 2019, 4:47 AM

@aprantl Thanks for your comments!

include/llvm/CodeGen/TargetInstrInfo.h
1698

Yes, Op is also an out parameter and that is why we didn't use an optional DIExpression as return value here.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
560

collectCallSiteParameters is maybe more appropriate name since we are collecting parameters info here.

djtodoro updated this revision to Diff 196432.Apr 24 2019, 5:42 AM

-Rebase
-Addressing comments
-Rename interpretationAnalysis()--> collectCallSiteParameters()
-Refactor the code

aprantl added inline comments.Apr 24 2019, 1:25 PM
include/llvm/CodeGen/TargetInstrInfo.h
1698

So why not return Optional<std::pair<const MachineOperand *, DIExpression *>> or is that worse?

lib/CodeGen/AsmPrinter/DwarfDebug.h
260

aren't registers usually unsigned?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
469

I'm confused by this. What's happening here?

dstenb added a subscriber: dstenb.Apr 25 2019, 1:54 AM
dstenb added inline comments.
test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir
56

Would it be possible to split the test into two separate basic blocks (or perhaps even different functions) for the different calls to foo? I think that might help the readability a bit.

90

Do you need the lifetime intrinsics for the test, or can they be removed?

96

Can be removed.

128–133

I think that most of the attributes that are default-valued can be removed. In addition to these, also hasWinCFI, registers, constants, machineFunctionInfo, and perhaps much in frameInfo.

@dstenb Thank you for your comments! We will address them!

include/llvm/CodeGen/TargetInstrInfo.h
1698

Yea. Sure. We will refactor it.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
469

This should be:

emitBRreg(Op->getArg(1), 0);

For call site parameter value we need to fetch the content of a register.
This additional DW_OP_reg is used because describeLoadedValue can produce expression that includes 2 registers (needed to support LEA instructions). We should probably add assertion here since this is only used for parameter location description and delete else.
We will make comment for this.

aprantl added inline comments.Apr 25 2019, 9:26 AM
lib/CodeGen/AsmPrinter/DwarfExpression.cpp
469

IIUC, the Verifier should reject any DIExpressions containing DW_OP_entry_values, because it doesn't know about DW_OP_reg operations. That means you are not calling the verifier on any of the expressions generated by LiveDebugValues. Could we add an assert(DIExpr->isValid()) somewhere there? Or alternatively shouldn't the machine verifier also call DIExpression::isValid on all DBG_VALUEs? The latter would be even better.

vsk added a comment.Apr 25 2019, 4:15 PM

Thanks for working on this. I haven't gotten to 'collectCallSiteParameters' or the x86-specific bits yet, but have some initial feedback which I hope is useful --

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
895

Does this switch the Dwarf4 + tuning=LLDB over to using GNU attributes? If so, can we either avoid that, or teach LLDB about the new attributes?

920

IIRC Dwarf5 specifies DW_AT_call_origin here. Is the GNU spec different? If so, why?

962

IIRC Dwarf5 allows DW_AT_call_return_pc here. Same question -- does the GNU spec disallow this, and if so, why?

968

This encodes the exact same information as DW_AT_call_return_pc above, just expressed as an offset from the binary load address as opposed to an offset from the start of the function. Why include both (specifically in the !ApplyGNUAttrs case)?

970

I alluded to this in a different comment -- and maybe this was already settled in which case I apologize for missing it -- but I'm not sure we should assume that Dwarf4 implies 'apply GNU attributes'. Assuming this means teaching LLDB about the GNU attributes, which I'd like to avoid doing unless it's important for LLDB to be able to debug GNU binaries.

976

I see the ApplyGNUAttrs ? A : B pattern repeated a lot. Maybe it'd be worthwhile to have a getDwarf5OrGNUAttribute helper which takes the official Dwarf5 attribute as an input, and returns either a) that same Dwarf5 attribute, or b) the appropriate GNU attribute.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
234–235

Document CallReg?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
704–706

This comment is now stale, as the logic no longer ignores indirect calls.

755

Could you split this up into multiple calls to dbgs() s.t there's just one copy of each debug string ('CallSiteEntry', 'IsTail', etc.)?

dstenb added inline comments.May 2 2019, 5:46 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
620

This avoid looking at the first instruction in the block, which could load an argument. Perhaps this should be a do-while loop instead?

I have not been able to land in such a case when compiling C code on x86-64 (I don't know if it is even possible), but I encountered such cases with our downstream target.

djtodoro marked 11 inline comments as done.May 13 2019, 2:45 AM

@aprantl, @vsk and @dstenb. Thanks for your comments!

Thanks for working on this. I haven't gotten to 'collectCallSiteParameters' or the x86-specific bits yet, but have some initial feedback which I hope is useful --

@vsk It is useful for sure! :)

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
895

Sure. We will avoid it. We see that LLDB uses DWARF 5 symbols even in DWARF 4 case.
We will generate GNU attributes only in GDB + DWARF 4 tune.

920

It is different. It generates DW_AT_abstract_origin in that case because DW_AT_call_origin was part of DWARF 5.

962

The same as previous.

968

I see. We knew it was redundant. We will leave in the case of DWARF 5 that info, but GDB needs absolute address in the case of DWARF 4. For representing absolute address of a call DW_AT_low_pc is used.

976

Sure. We added that.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
620

Yes, thanks for this! We improved this!

755

Sure.

lib/CodeGen/AsmPrinter/DwarfDebug.h
260

Yes, thanks!

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
469

DW_OP_reg is used only in situations when we want to describe an expression over two registers (example: Loading a parameter's value with LEA instruction. Please see X86InstrInfo::describeLoadedValue()). We can discuss about potential alternatives for that.

DW_OP_entry_values uses advantages from @entry values (read from DW_TAG_call_site_parameter info) when representing normal values. In debugger we would have a new feature for printing of new kind of value (i.e. print val@entry). GDB already has that support.
Without the part for producing DW_OP_entry_values (in LiveDebugValues) we would see just those val@entry and there won't be cases of improving normal values of a parameters.
For now, we generate DW_OP_entry_values with a simple register location, but the register is read from DBG_VALUE operand (We don't need DW_OP_reg in DIExpression for that). DIExpression has only entry value DWARF operand and the size of following expression (for now it is always 1, because the rest of expression will be only register from DBG_VALUE).

The conclusion is that there are two separate parts here:

  1. Introduction debug info for call site parameters (DW_TAG_call_site_parameter) that is used for printing @entry value within debugger
  2. Introduction of new kind of DWARF operand (DW_OP_entry_value) that can use benefits of @entry values when representing normal values of function parameters
djtodoro updated this revision to Diff 199224.May 13 2019, 2:47 AM

-Refactor the code
-Trim existing tests
-Improve production of call site info
-Rebase

I think we're getting there!

docs/LangRef.rst
4762

"a" physical register

4763

... after AsmPrinter to describe a call site ...

better be specific.

4764

This opcode is only generated by the ... pass.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
238

tuning

djtodoro marked 4 inline comments as done.May 16 2019, 5:15 AM

@aprantl Thanks!

djtodoro updated this revision to Diff 199795.May 16 2019, 5:16 AM

-Address suggestions

aprantl added inline comments.May 16 2019, 3:45 PM
docs/LangRef.rst
4769

when trying to describe -> to describe

it better get it right :-)

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
921

opcode -> attribute

lib/IR/DebugInfoMetadata.cpp
856

I think this must also check that we are inside an entry value.

djtodoro marked an inline comment as done.May 17 2019, 6:54 AM
djtodoro added inline comments.
lib/IR/DebugInfoMetadata.cpp
856

DW_OP_reg is used only in situations when we want to describe an expression over two registers (example: Loading a parameter's value with LEA instruction. Please see X86InstrInfo::describeLoadedValue()). We can discuss about potential alternatives for that.

DW_OP_entry_values uses advantages from @entry values (read from DW_TAG_call_site_parameter info) when representing normal values. In debugger we would have a new feature for printing of new kind of value (i.e. print val@entry). GDB already has that support.
Without the part for producing DW_OP_entry_values (in LiveDebugValues) we would see just those val@entry and there won't be cases of improving normal values of a parameters.
For now, we generate DW_OP_entry_values with a simple register location, but the register is read from DBG_VALUE operand (We don't need DW_OP_reg in DIExpression for that). DIExpression has only entry value DWARF operand and the size of following expression (for now it is always 1, because the rest of expression will be only register from DBG_VALUE).

The conclusion is that there are two separate parts here:

Introduction debug info for call site parameters (DW_TAG_call_site_parameter) that is used for printing @entry value within debugger
Introduction of new kind of DWARF operand (DW_OP_entry_value) that can use benefits of @entry values when representing normal values of function parameters

According to this comment this new DW_OP_reg operand will only be used in the situation when an instruction, that loads a value into parameter forwarding register, is dealing with multiple registers (please see test case test/DebugInfo/MIR/X86/dbgcall-site-lea-interpretation.mir). The DIExpression that contains the DW_OP_reg will be used only for printing the DW_AT_GNU_call_site_value and if we remove this part the only thing that will happen is that we will have less number of call site parameters generated (because without the DW_AT_GNU_call_site_value a DW_TAG_GNU_call_site_parameter is useless and if don't manage to describe loaded value we are avoiding the production of call site parameters debug info).

djtodoro updated this revision to Diff 200037.May 17 2019, 6:55 AM

-Address suggestions

djtodoro updated this revision to Diff 200959.May 23 2019, 6:30 AM

-Add check for a frame reg when describing a parameter value

aprantl accepted this revision.May 23 2019, 9:36 AM
This revision is now accepted and ready to land.May 23 2019, 9:36 AM
djtodoro updated this revision to Diff 201514.May 27 2019, 5:21 AM

-Refactor the code a bit
-Rebase

dstenb added inline comments.May 27 2019, 5:50 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
593

Nit: interpret

lib/Target/X86/X86InstrInfo.cpp
7362

If neither this nor the isFI() condition are true then Op will be used uninitialized on line 7135 and onward. (I got a -Wsometimes-unintialized warning for this.)

@dstenb Thanks a lot for your comments!

djtodoro updated this revision to Diff 201522.May 27 2019, 6:46 AM

-Addressing comments

djtodoro updated this revision to Diff 202420.May 31 2019, 5:09 AM

-Rebase tests

Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 6:37 AM
dstenb added inline comments.Thu, Jul 4, 7:09 AM
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
473 ↗(On Diff #205799)

Is the fallthrough intentional here, or a merge artifact?

djtodoro marked an inline comment as done.Thu, Jul 4, 7:57 AM
djtodoro added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp
473 ↗(On Diff #205799)

We forgot to update this on to Phabricator. Thanks!

djtodoro updated this revision to Diff 208045.Thu, Jul 4, 7:59 AM

-Fixing unintentional code during the rebase process

This revision was automatically updated to reflect the committed changes.

Thanks a lot for your work with this patch series! It seems very useful.

Sorry for coming with this post-merge question, but I encountered it first now when trying to adapt this to our downstream target; what limitations are imposed on the MachineOperand pointer that the describeLoadedValue returns? For example, in our downstream target we materialize 0 with a sub $reg, $reg, so we'd have to return a pointer to a MachineOperand that is not part of the instruction. Is that allowed? I guess x86 has the same issue as it materializes 0 with xor.

Perhaps it would be possible to make the hook return a MachineOperand object instead? It could simplify the code a bit in such cases. Or would that be too expensive?

@dstenb Thanks a lot!

That seems like a good catch and we should support such scenario.

I think that it will be expensive to return a MachineOperand object. Could you please provide us with more info about the case? Can we describe such situation through the DIExpression instead?

This commit broke codegen on SystemZ, we're now failing both the test-suite and the multistage bootstrap. Unfortunately this wasn't detected earlier since the SystemZ build bot was down.

The regressions go away when I revert the change to TargetRegisterInfo::isCallerPreservedPhysReg, i.e. change it back to always returning false. Note that this change is the only modification in this patch that has any effect on actual code generation (as opposed to debug info generation) -- was this really intentional?

I have not yet debugged the failure in detail, but I believe the isCallerPreservedPhysReg change is in fact incorrect -- this function was not intended to just mirror getCallPreservedMask (which simply describes the function call ABI), but rather to handle special cases like the PowerPC TOC register, which is not just simply preserved across calls, but rather is invariant throughout the whole function. This is why this check is used to eliminate invariant stores in MachineLICM. The change in this patch breaks that semantics.

This really needs to be fixed before the LLVM 9 release.

djtodoro reopened this revision.Fri, Jul 12, 4:31 AM

@uweigand Thanks for that! The commit is reverted with the rL365886.

We didn't want to change the generated code, so we just made a quick fix for this. This should solve the problem.

This revision is now accepted and ready to land.Fri, Jul 12, 4:31 AM
djtodoro updated this revision to Diff 209459.Fri, Jul 12, 4:35 AM

-Make a wrapper around the getCallPreservedMask() and use it in order to avoid changes in generated code

@dstenb Thanks a lot!

That seems like a good catch and we should support such scenario.

I think that it will be expensive to return a MachineOperand object.

The size of MachineOperand is 32 bytes on x86-64, and the class only contains POD fields, so I guess it should be fairly cheap to copy such objects, or initialize them with compile-time known values? Though, perhaps that would still cause a noticeable increase in compilation time.

I think it would be interesting to benchmark that at least.

Could you please provide us with more info about the case?

Here is an x86-64 reproducer.

extern void callee(int);
void caller() { callee(0); }

When compiled using -O2 -g -Xclang -femit-debug-entry-values it gives the following instruction to try to describe the parameter with:

$edi = XOR32rr undef $edi(tied-def 0), undef $edi, implicit-def dead $eflags

Can we describe such situation through the DIExpression instead?

Oh, maybe! Do you have some suggestions for how that could be done?

@dstenb Thanks a lot, that seems useful!

The size of MachineOperand is 32 bytes on x86-64, and the class only contains POD fields, so I guess it should be fairly cheap to copy such objects, or initialize them with compile-time known values? Though, perhaps that would still cause a noticeable increase in compilation time.

I think it would be interesting to benchmark that at least

Yes, I agree that we should definitely experiment with that.

Here is an x86-64 reproducer.

extern void callee(int);
void caller() { callee(0); }

When compiled using -O2 -g -Xclang -femit-debug-entry-values it gives the following instruction to try to describe the parameter with:

$edi = XOR32rr undef $edi(tied-def 0), undef $edi, implicit-def dead $eflags

Thanks for the test case, I also came up with some "real world" cases with situation like this.

Can we describe such situation through the DIExpression instead?

Oh, maybe! Do you have some suggestions for how that could be done?

I think we could make a DIExpression with the DW_OP_const* , 0 (within ParamLoadedValue) and then handle it properly in collectCallSiteParameters(). WDYT?
In addition, do you think that we could treat that situation like an improvement and create a separate thread for that (so we can re-land this with the latest fix)?

Thanks for the quick revert! The new patch looks reasonable to me.

@uweigand Thanks for the note and comment!