Page MenuHomePhabricator

[LiveDebugValues] Emit parameter's entry value
Needs ReviewPublic

Authored by djtodoro on Feb 11 2019, 3:50 AM.

Details

Summary

Job of this pass to broadcast DBG_VALUES to the all successive blocks where its preserved location is valid. As a natural way it should emit replacements for clobbered parameters location.

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

Diff Detail

Event Timeline

djtodoro created this revision.Feb 11 2019, 3:50 AM
djtodoro added a project: debug-info.
djtodoro added a subscriber: petarj.
wolfgangp added inline comments.Feb 11 2019, 6:16 PM
lib/CodeGen/LiveDebugValues.cpp
302

I would prefer either the name "ParameterEntryVals" or "ParamEntyrVals", throughout the file, instead of "ParametrEntryVals".

314

This type is used in several locations. Perhaps we could have a using declaration
for it and use a more compact name?

821

This is invariant in the for-loop, no? Perhaps it should be pulled ahead of it?

945

I think this comment needs perhaps some rephrasing. Something like:
Enter DEBUG_VALUE instructions that track parameters into ParameterEntryVals. When we encounter a variable that we have already entered, we assume that we have found all parameter
entry locations and stop.

947

some spelling errors: wheter -> whether, vraible->variable, it->is

Suggested wording:
We check whether MI is inlined in order to deduce whether the variable that it tracks
comes from a different function. If that is the case we can't track its entry value.

@wolfgangp Thanks a lot for your comments! It has been addressed.

djtodoro updated this revision to Diff 186452.Feb 12 2019, 6:40 AM
djtodoro updated this revision to Diff 186613.Feb 13 2019, 2:45 AM
  • Rename: VariableNotChanged ===> ArgumentNotModified
  • Refactor a test case
dstenb added a subscriber: dstenb.Feb 13 2019, 1:19 PM
dstenb added inline comments.
lib/CodeGen/LiveDebugValues.cpp
439–441

Should this use Expr->isEntryValue()? However, the implementation of that function in D58041 only checks if the first element is the operation:

bool isEntryValue() const {
    return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_GNU_entry_value;
}

Is that correct, or should it look through all elements like is done here? Or is there some reason for why they should be disjunct? If so, could you add a comment here?

NikolaPrica added inline comments.Feb 14 2019, 3:05 AM
lib/CodeGen/LiveDebugValues.cpp
439–441

You are right! It should be isEntryValue()! Thanks!

djtodoro updated this revision to Diff 187190.Feb 17 2019, 10:41 PM
  • Use isEntryValue() from DIExpression
dstenb added inline comments.Feb 18 2019, 5:06 AM
lib/CodeGen/LiveDebugValues.cpp
439–441

Okay, thanks for the clarification. So DW_OP_GNU_entry_value will only be located in the first element then?

djtodoro marked an inline comment as done.Feb 19 2019, 12:14 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
439–441

In the current design, yes.
DIExpression will always have entry value DWARF operand as first element.

djtodoro updated this revision to Diff 195197.Mon, Apr 15, 8:50 AM
djtodoro edited the summary of this revision. (Show Details)

-Rebase
-Follow-up with the new approach

aprantl added inline comments.Mon, Apr 15, 8:54 PM
include/llvm/IR/DebugInfoMetadata.h
2529 ↗(On Diff #195197)

The more flags we add these bool parameters get less and less safe, since the order isn't typesafe. We should probably think about also using an unsigned Flag parameter that is WithDeref | WithEntryValue.

2600 ↗(On Diff #195197)

In LLVM IR I'd prefer using the DWARF 5 DW_OP_entry_value (Or a new LLVM one if we change the semantics). We can still emit the GNU variant, if requested.

According to the DWARF spec an entry value can appear anywhere in the dwarf expression, right? If we only accept one as the first element, we also need a verifier that enforces this.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
333 ↗(On Diff #195197)

This should be DW_OP_entry_value if DWARF 5 is requested (or a perhaps GDB tuning)

lib/CodeGen/LiveDebugValues.cpp
450

Would it be possible to split this part into a separate patch?

djtodoro marked 4 inline comments as done.Mon, Apr 15, 11:59 PM
djtodoro added inline comments.
include/llvm/IR/DebugInfoMetadata.h
2529 ↗(On Diff #195197)

I agree with this.

2600 ↗(On Diff #195197)

In LLVM IR I'd prefer using the DWARF 5 DW_OP_entry_value (Or a new LLVM one if we change the semantics). We can still emit the GNU variant, if requested.

We will make it.

According to the DWARF spec an entry value can appear anywhere in the dwarf expression, right?

Yes, but for now we support just basic entry values expressions.

If we only accept one as the first element, we also need a verifier that enforces this.

Sure.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
333 ↗(On Diff #195197)

Sure.

lib/CodeGen/LiveDebugValues.cpp
450

IIUC, splitting introduction and production of entry_values makes sense.

djtodoro updated this revision to Diff 195722.Thu, Apr 18, 6:14 AM

-Use DW_OP_entry_value from DWARF 5
-Split up introduction and production of entry values

ormris removed a subscriber: ormris.Thu, Apr 18, 9:27 AM