Page MenuHomePhabricator

[lldb/DWARF] Add support for DW_OP_implicit_value
ClosedPublic

Authored by mib on Oct 20 2020, 7:19 PM.

Details

Summary

This patch completes https://reviews.llvm.org/D83560. Now that the
compiler can emit DW_OP_implicit_value into DWARF expressions, lldb
needed to learn reading these opcodes for variable inspection and
expression evaluation.

This implicit location descriptor specifies an immediate value with two
operands: the length (ULEB128) followed by a block representing the value
in the target memory representation.

rdar://67406091

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Oct 20 2020, 7:19 PM
mib requested review of this revision.Oct 20 2020, 7:19 PM
mib updated this revision to Diff 299539.Oct 20 2020, 7:22 PM
mib retitled this revision from [lldb/DWARF] Add support DW_OP_implicit_value to [lldb/DWARF] Add support for DW_OP_implicit_value.
mib edited the summary of this revision. (Show Details)
mib updated this revision to Diff 299540.Oct 20 2020, 7:24 PM

Add test.

Harbormaster completed remote builds in B75805: Diff 299539.

The code looks pretty straight-forward, but I believe the test should be asm-based with hardcoded dwarf (if you're feeling adventurous, you can also try yaml). My reasons for that are:

  • it makes it guarantees (and makes it explicit) the input that is being tested
  • the test can run on any platform (if done right -- no launching of processes)
  • it enables us to test the error scenario where we fail to read the DW_OP_implicit_value data.

I think the simplest way to produce such a test would be to take DW_TAG_variable-DW_AT_const_value.s and exchange DW_AT_const_value for DW_AT_location/DW_FORM_exprloc

lldb/source/Expression/DWARFExpression.cpp
856–867

I'm not sure this function is really complex enough to justify its existence. The actual code is pretty short and most of it is just argument lists and logging. I don't think the logging is very useful as the caller logs already, and the argument lists would go away if this were inlined.

+1 on everything Pavel said.

lldb/source/Expression/DWARFExpression.cpp
856–867

Agreed, plus half of the arguments are unused.

mib marked 2 inline comments as done.Oct 21 2020, 8:00 PM
mib updated this revision to Diff 299941.Oct 22 2020, 6:04 AM
mib edited the summary of this revision. (Show Details)

Change shell test for unit test.

This revision is now accepted and ready to land.Oct 22 2020, 8:37 AM
This revision was landed with ongoing or failed builds.Oct 22 2020, 9:03 AM
This revision was automatically updated to reflect the committed changes.