This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Evaluate DW_OP_entry_value
ClosedPublic

Authored by vsk on Sep 9 2019, 3:32 PM.

Details

Summary

Add support for evaluating DW_OP_entry_value. This involves parsing
DW_TAG_call_site_parameter and wiring the information through to the expression
evaluator.

rdar://54496008

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Sep 9 2019, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 3:32 PM
vsk updated this revision to Diff 219442.Sep 9 2019, 3:35 PM
  • Clean up the test.

This is very exciting!

lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values/Makefile
4 ↗(On Diff #219442)

The -g should be redundant, and if we leave out -glldb (which should be the default for clang anyway) then this test in theory could also work with other compilers such as GCC (which I think implements the same option under the same -f option). I have no idea whether anyone runs such a bot, but it's still nice.

lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values/main.cpp
7 ↗(On Diff #219442)

I would just drop the header for testcases.

35 ↗(On Diff #219442)

Cool.

lldb/source/Expression/DWARFExpression.cpp
497 ↗(On Diff #219442)

We should probably just let llvm's libDebugInfo do the printing here, but this works fine for now.

1117 ↗(On Diff #219442)

The Evaluate function allows setting an Error with a message. Should we do the same thing here so we can return the error to the caller?

2856 ↗(On Diff #219442)

See above, we can set error_ptr here.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.cpp
62 ↗(On Diff #219442)

Don't spend too much time on that, we should just use the llvm DWARFExpression iterator instead.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3712 ↗(On Diff #219442)

DW_TAG_call_site

3752 ↗(On Diff #219442)

default?

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
795 ↗(On Diff #219442)

This should also be tested inside of LLVM itself and probably should be a separate commit.

vsk updated this revision to Diff 219462.Sep 9 2019, 6:12 PM
vsk marked 7 inline comments as done.
  • Partially address review feedback.
vsk planned changes to this revision.Sep 9 2019, 6:13 PM

TODO:

  • Split out llvm change.
  • Add a test to validate that lldb skips inline frames when evaluating DW_OP_entry_value.
lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values/Makefile
4 ↗(On Diff #219442)

llvm entry value support requires one of -glldb or -ggdb at the moment, but -g can be dropped.

lldb/source/Expression/DWARFExpression.cpp
1117 ↗(On Diff #219442)

I worry that these log messages are too specific to be meaningful to the caller. The immediate caller should be able to issue an error, however.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.cpp
62 ↗(On Diff #219442)

Updated the fixme.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3752 ↗(On Diff #219442)

I don't believe any action is needed in the default case: we do not want to log or report an error.

aprantl added inline comments.Sep 10 2019, 8:40 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3752 ↗(On Diff #219442)

Doesn't this trigger the switch-doesn't-cover-all-cases warning otherwise?

vsk updated this revision to Diff 219573.Sep 10 2019, 11:17 AM
vsk marked an inline comment as done.
vsk edited the summary of this revision. (Show Details)
  • Addressed review feedback, split out unrelated changes, and improved test coverage.
vsk marked an inline comment as done.Sep 10 2019, 11:23 AM
vsk added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3752 ↗(On Diff #219442)

No warning because dw_attr_t is a uint16_t, but it's probably simpler to avoid the switch here.

grandinj added inline comments.
lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

the way this is being used seems to indicate it can be

std::vector<CallSiteParameter>

no need for unique_ptr

vsk planned changes to this revision.Sep 10 2019, 3:33 PM
vsk marked an inline comment as done.

While tightening up the test case I think I found an issue with the way inlined frames are handled. I need to take a closer look.

lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

That's a totally fair point. The reason I've used unique_ptr here is to save space in CallEdge in the common case, where no call site information is loaded for the function. Call site info is lazily parsed, so we'd like to take a minimal memory hit for functions that aren't in a backtrace.

Also, note that using a pointer allows for a further PointerIntPair memory optimization mentioned below.

aprantl added inline comments.Sep 10 2019, 3:50 PM
lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

Can you document this decision up there?

249 ↗(On Diff #219573)

Out of curiosity: What's the effect of this line? It appears to have totally redundant information in it that Doxygen should already know about.

lldb/source/Expression/DWARFExpression.cpp
1136 ↗(On Diff #219573)

What does it mean if there is a null parent_frame and shouldn't we return false in that case?

2672 ↗(On Diff #219573)

because...?

vsk updated this revision to Diff 219646.Sep 10 2019, 6:14 PM
vsk marked 4 inline comments as done.
  • Fix return address lookup when the immediate parent frame is inlined.
  • Tighten the test so that it actually verifies that tail calls, inlining, etc. occur, instead of assuming :).
  • Add/move various TODOs as pointed out by reviewers.

I'm happy with the test coverage now, and think that I've addressed all the feedback. PTAL, thanks!

lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

Done. And, thanks @grandinj for pointing this out, I dug a bit more and found that we're *not* doing this in Function for the CallEdge vector, but probably should be. Added a TODO there.

249 ↗(On Diff #219573)

No clue. I saw it elsewhere in this file and wanted to stick to the established format. It could be worth simplifying later, though.

lldb/source/Expression/DWARFExpression.cpp
1136 ↗(On Diff #219573)

Yes, we should detect this and fail early.

2672 ↗(On Diff #219573)

Actually, rejecting DW_OP_push_object_address requires no special handling. Instead we need a TODO about actually supporting it: that belongs in the Evaluate_DW_OP_entry_value.

vsk updated this revision to Diff 219648.Sep 10 2019, 6:24 PM
vsk marked an inline comment as done.
vsk added inline comments.
lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

Actually, there's no need to do this in both CallEdge and Function: edges are parsed lazily, but parameters aren't. Let's just leave a note about this in Function.

labath added a subscriber: labath.Sep 11 2019, 5:55 AM
labath added inline comments.
lldb/source/Expression/DWARFExpression.cpp
497 ↗(On Diff #219442)

FYI: I have a patch which does just that. I just need to make libDebugInfo not crash so easily.

Looking great! I have one more question inline, but that is mostly about documenting the algorithm.

lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

Is a SmallVector<0> (16 bytes on x86_64) smaller than a libcxx std::vector<>?

249 ↗(On Diff #219573)

Let's do that in a separate NFC patch!

lldb/source/Expression/DWARFExpression.cpp
1199 ↗(On Diff #219648)

std::find_if or something?

1236 ↗(On Diff #219648)

Here the comments are not enough for me to follow why we are doing this? Could you explain it to me and then add that to the comment as well?

What would an example DW_OP_entry_value and matching call site parameter look like?

aprantl added inline comments.Sep 11 2019, 8:57 AM
lldb/source/Expression/DWARFExpression.cpp
497 ↗(On Diff #219442)

Oh thanks for mentioning that! I'll stop working on mine then :-)

vsk updated this revision to Diff 219779.Sep 11 2019, 1:07 PM
vsk marked 3 inline comments as done.
  • Add a working example and some additional comments in Evaluate_DW_OP_entry_value.
lldb/include/lldb/Symbol/Function.h
258 ↗(On Diff #219462)

Oh, yes.

lldb/source/Expression/DWARFExpression.cpp
1199 ↗(On Diff #219648)

I tried this, but the resulting code did not look clearer to me.

1236 ↗(On Diff #219648)

I added a worked-through example at the start of the function -- is that ok?

aprantl accepted this revision.Sep 11 2019, 1:16 PM

Thanks, that's exactly what I was looking for!

This revision is now accepted and ready to land.Sep 11 2019, 1:16 PM
This revision was automatically updated to reflect the committed changes.