Page MenuHomePhabricator

[DwarfDebug] Dump call site debug info into DWARF
Needs ReviewPublic

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

djtodoro added a project: debug-info.
djtodoro updated this revision to Diff 195361.Apr 16 2019, 5:49 AM

-Minor refactor

Only a few superficial comments inline, I'll need to read this more carefully.

test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir
97

We probably don't most (all) of these attributes. If we can remove them that would be better, since they are a maintenance nightmare.

tools/llvm-dwarfdump/Statistics.cpp
401

Looks like these are untested?

djtodoro marked 2 inline comments as done.Apr 17 2019, 3:27 AM
djtodoro added inline comments.
test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir
97

Oh, we didn't want to push this :)
This will be removed.

tools/llvm-dwarfdump/Statistics.cpp
401

We will add tests.
Thanks!

djtodoro updated this revision to Diff 195690.Apr 18 2019, 2:13 AM
djtodoro added a subscriber: petarj.
djtodoro updated this revision to Diff 195724.Apr 18 2019, 6:24 AM

-Rebase
-Update tests

ormris removed a subscriber: ormris.Mon, Apr 22, 9:29 AM
aprantl added inline comments.Mon, Apr 22, 10:30 AM
include/llvm/CodeGen/TargetInstrInfo.h
1688

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

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
519

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

580

clang-format

614

Process

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

@aprantl Thanks for your comments!

include/llvm/CodeGen/TargetInstrInfo.h
1688

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
519

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

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

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

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

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

lib/CodeGen/AsmPrinter/DwarfDebug.h
268

aren't registers usually unsigned?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
475

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

dstenb added a subscriber: dstenb.Thu, Apr 25, 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
1688

Yea. Sure. We will refactor it.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
475

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.Thu, Apr 25, 9:26 AM
lib/CodeGen/AsmPrinter/DwarfExpression.cpp
475

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.Thu, Apr 25, 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
937

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?

955

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

964

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

970

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)?

972

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.

978

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
232–233

Document CallReg?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
663–665

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

718

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.Thu, May 2, 5:46 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
579

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.Mon, May 13, 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
937

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.

955

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

964

The same as previous.

970

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.

978

Sure. We added that.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
579

Yes, thanks for this! We improved this!

718

Sure.

lib/CodeGen/AsmPrinter/DwarfDebug.h
268

Yes, thanks!

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
475

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.Mon, May 13, 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
4701

"a" physical register

4702

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

better be specific.

4703

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

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
236

tuning

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

@aprantl Thanks!

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

-Address suggestions

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

when trying to describe -> to describe

it better get it right :-)

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
927

opcode -> attribute

lib/IR/DebugInfoMetadata.cpp
854

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

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

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.Fri, May 17, 6:55 AM

-Address suggestions