This is an archive of the discontinued LLVM Phabricator instance.

[DwarfDebug] Dump call site debug info into DWARF
ClosedPublic

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
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.Apr 22 2019, 9:29 AM
aprantl added inline comments.Apr 22 2019, 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.Apr 24 2019, 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.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
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.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
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.Apr 25 2019, 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.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
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.May 2 2019, 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.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
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.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
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.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
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.May 17 2019, 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.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
552

Nit: interpret

lib/Target/X86/X86InstrInfo.cpp
7129

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.Jul 4 2019, 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.Jul 4 2019, 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.Jul 4 2019, 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.Jul 12 2019, 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.Jul 12 2019, 4:31 AM
djtodoro updated this revision to Diff 209459.Jul 12 2019, 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!

vsk added a comment.Jul 25 2019, 6:52 PM

This has been sitting on my review queue for too long, I'm really sorry for the delay here. I've still only gotten 2/3 of the way through this patch but will make time to finish the rest soon. Thanks for working on this!

I have a correctness concern in 'shouldInterpret'. My other concerns are about documentation/naming.

include/llvm/CodeGen/TargetInstrInfo.h
1686

Could you clarify what RHS mean, here?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
972

const auto& Param?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
228

Please document these methods and, if it's not too much trouble, mark them const.

238

As the descriptions of the parameters here is fairly complex, could you split the discussion of each parameter into a separate paragraph?

Also, is the parenthetical "(in the case of non-gdb tuning)" in the PCOffset description redundant?

238

Does CallReg=0 for direct calls?

242

Please document constructCallSiteParmEntryDIEs. Specifically, it would help to have a \ref to the function that creates the param array, and to have a note about whether or not the order of the parameters matters.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
539

Can we assert that this insertion into ArgsRegsForProcess occurs? I'm assuming that it's a bug if more than one argument is forward in the same register.

Super nit-pick, but, I don't think having 'ForProcess' in the name is very clear. Something like 'ForwardedRegWorklist' would be more in keeping with the naming in the rest of the compiler.

544

Could you add a comment explaining what RegsForEntryValues is before explaining ShouldTryEmitEntryVals?

548

The comment for 'shouldInterpret' should be updated to reflect the fact that it returns a register, not a bool. Also, consider using a more specific name, like 'getForwardingRegDefinedByMI'.

556

Is the first call to MO.getReg() just checking for undef? I wonder why TRI->isPhysicalRegister() does not make it redundant.

558

Can more than one Def be attached to an MI? 'MachineInstr::getNumDefs()' suggests it is possible. If so, then it seems like there is a bug here when more than one forwarding reg is clobbered by MI?

566

Why are DbgLocVal and Reg passed in by reference?

569

Consider using RegsForEntryValues.find() to avoid doing two lookups.

576

Consider using a for loop here. It will guarantee iterator increment and make it easier to continue early.

586

To simplify this, I suggest continuing early when Reg is undef.

589

These declarations for Op and Expr don't seem necessary.

603

Please use /*IsIndirect=*/IsSPorFP when calling the MachineLocation constructor.

606

I wonder whether two passes through the block are needed, as ArgsRegsForProcess may not be complete when it is referenced in shouldInterpret.

619

Not sure what '1' is for in '{dwarf::DW_OP_entry_value, 1}', perhaps a comment would help?

621

RegsForEntryValues.find to avoid two lookups?

lib/CodeGen/AsmPrinter/DwarfDebug.h
269

Consider adding some detail, like:

unsigned Register; ///< Parameter register at the callee entry point.
DbgValueLoc; ///< Corresponding location for the parameter value at the call site.
272

Assert that Reg isn't undef here?

275

Please mark the DbgCallSiteParam methods const.

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

Consider using 'FileCheck -implicit-check-not=DW_TAG_GNU_call_site_parameter %s' to catch all of them.

djtodoro marked 36 inline comments as done.Jul 29 2019, 5:57 AM

@vsk Thanks a lot for such nice review!

include/llvm/CodeGen/TargetInstrInfo.h
1686

Sure.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
238

As the descriptions of the parameters here is fairly complex, could you split the discussion of each parameter into a separate paragraph?

Sure.

Also, is the parenthetical "(in the case of non-gdb tuning)" in the PCOffset description redundant?

The comment is updated in order to avoid the ambiguity. GDB uses PC info even for tail calls.

Does CallReg=0 for direct calls?

Yes.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
539

This only adds all the regs into the working list by iterating through the CallSiteInfo.

Super nit-pick, but, I don't think having 'ForProcess' in the name is very clear. Something like 'ForwardedRegWorklist' would be more in keeping with the naming in the rest of the compiler.

Thanks!!

544

Sure.

548

Thanks for this!

556

Yes..Thanks!

558

Hmm...Yes, we have not consider such situation. Thanks!

566

We can pass them as values, and let the compiler to optimize it for us.

606

The working list is complete at the moment. But, we are inserting new registers in order to try generating call site parameters values as entry values (after the loop ends). We are doing that only for the entry MBB (the simplest case), but since the GCC generates the call site values expression as entry values even for the MBBs other then the entry one, we have left the TODO comment for that.

619

Sure.

lib/CodeGen/AsmPrinter/DwarfDebug.h
269

Thanks!

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

We just wanted to emphasizes that we did not describe a call site parameter. Since all of this is controlled by an option, the implicit-check-not will require not to use the option on the same test case. But we put an error alerting that the CallSiteInfo (the MF attribute callSites) is provided but not used, so it will be difficult to use the implicit-check-not here.

djtodoro updated this revision to Diff 212158.Jul 29 2019, 5:58 AM
djtodoro marked 12 inline comments as done.

-Addressing comments

vsk added inline comments.Jul 29 2019, 1:56 PM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.h
238

nit, tuning

241

nit, matter, and its -> the

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
539

I see. I had in mind:

bool InsertedReg = ForwardedRegWorklist.insert(ArgReg.Reg).second;
assert(InsertedReg && "Single register used to forward two arguments?");

If this is already checked elsewhere, there's no need for a duplicate assert.

568

After FwdReg from MO is pushed into Defs, shouldn't this loop should break? IIUC no other registers should be equal-to/alias FwdReg.

590

Should the basic block scan stop at branches in addition to calls to reduce compile time (MI::isCall() || MI::isBranch())?

591

If the scan reaches a call or a branch, shouldn't it break instead of returning? That way, the ShouldTryEmitEntryVals case can be finished. Perhaps there should be a test?

642

nit, extra indentation here?

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
478

I don't understand the lowering of DW_OP_regx within addMachineRegExpression. Why should the lowering be different within/without parameter values? It seems like the DIExpression itself should be formed with DW_OP_bregx, if/when needed.

lib/CodeGen/TargetInstrInfo.cpp
1142

I do not think that casting the offset to unsigned is safe due to loss of precision. Also, note that the expression generated for the Offset=0 case is inefficient. Is there some way to reuse the code in DIExpression::appendOffset for this?

lib/IR/DebugInfoMetadata.cpp
854

@aprantl would you mind re-stating your concern? The dwarf standard says that DW_OP_reg<n> describes an object in a register. That seems like it would be valid outside of a DW_OP_entry_value block. Is it not?

lib/Target/X86/X86InstrInfo.cpp
7175

We should be able to factor out the logic in DIExpression::{append,prepend}Offset to simplify this.

djtodoro marked 12 inline comments as done.Jul 31 2019, 4:26 AM

@vsk Thanks for the comments!

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
539

I see.. That makes sense.

568

Yes. Thanks!

590

Actually, that is not necessary, since this will never encounter a branch.

591

If it encounters a call that means we can not consider the 'DW_OP_entry_value' as valid expression with in a call site parameter value, since the call may clobber that, and we stop the analysis as is.

There is a test for that part within the test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
478

Yes, thanks! That is better way for sure.

lib/CodeGen/TargetInstrInfo.cpp
1142

I agree, we can use the DIExpression::appendOffset.

lib/Target/X86/X86InstrInfo.cpp
7175

We can use that.

djtodoro updated this revision to Diff 212551.Jul 31 2019, 4:28 AM
djtodoro marked 2 inline comments as done.

-Addressing comments

vsk accepted this revision.Jul 31 2019, 8:20 AM

Thanks, lgtm.

This revision was automatically updated to reflect the committed changes.