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

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

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

1117

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?

3003

See above, we can set error_ptr here.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDefines.cpp
62

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

DW_TAG_call_site

3752

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

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

Updated the fixme.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3752

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

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

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

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

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
249

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.

258

Can you document this decision up there?

lldb/source/Expression/DWARFExpression.cpp
1136

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

2742

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
249

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

258

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.

lldb/source/Expression/DWARFExpression.cpp
1136

Yes, we should detect this and fail early.

2742

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

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

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
249

Let's do that in a separate NFC patch!

258

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

lldb/source/Expression/DWARFExpression.cpp
1199

std::find_if or something?

1236

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

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

Oh, yes.

lldb/source/Expression/DWARFExpression.cpp
1199

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

1236

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.