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
296

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

312

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

825

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

998

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.

1000

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
451

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
451

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
aprantl added inline comments.Mon, Apr 22, 10:10 AM
lib/CodeGen/LiveDebugValues.cpp
213

Can you convert this into early-exit form?

if (Var != Other.Var)
  return Var < Other.Var;
...
433

The combination of TODO and also makes these sentences confusing to me.

457

factor out VarLocIDs[ID] (.Var.getVar()) for readability?

458
if (! ...)
  continue
471

auto &foo = **DMI;

485

Transfers.push_back({&MI, EntryValDbgMI});

djtodoro marked 5 inline comments as done.Wed, Apr 24, 4:42 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
213

Sure.

457

Sure.

djtodoro updated this revision to Diff 196431.Wed, Apr 24, 5:39 AM

-Addressing comments