Page MenuHomePhabricator

DWARFExpression: Convert file addresses to load addresses early on
ClosedPublic

Authored by aprantl on May 2 2018, 11:52 AM.

Details

Summary

This is a change that only affects Swift and is NFC for the language plugins on llvm.org. In Swift, we can have global variables with a location such as DW_OP_addr <addr> DW_OP_deref. The DWARF expression evaluator doesn't know how to apply a DW_OP_deref to a file address, but at the very end we convert the file address into a load address.

This patch moves the file->load address conversion to right after the result of the DW_OP_addr is pushed onto the stack so that a subsequent DW_OP_deref (and potentially other operations) can be interpreted.

rdar://problem/39767528 LLDB can't deal with dwarf expressions on global variables

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.May 2 2018, 11:52 AM
labath added a subscriber: labath.May 3 2018, 2:26 AM

One day I'd like us to have a unit testing framework for the dwarf expression evaluator. The evaluator looks like a thing that is both complex enough (so it's worth doing it) and /ought to be/ self-contained enough (so it is doable). Then we could test these, and all other sorts of funny expressions without having to tickle a compiler into producing them.

source/Core/Value.cpp
672 ↗(On Diff #144910)

You should be able to get the target out of the symbol context.

683–685 ↗(On Diff #144910)

This is not what the original code was doing, but what do you think about getting the section list directly from the Module? The two lists aren't exactly the same, but this distinction should not matter here.
Besides being shorter, this would also make this code work slightly better with object-file-less Modules that Leonard is trying to introduce (D46292 et al.).

clayborg requested changes to this revision.May 3 2018, 7:30 AM

great fix. Just resolve the file address using the module function and this is good to go.

source/Core/Value.cpp
683–685 ↗(On Diff #144910)

Remove these 3 lines

687 ↗(On Diff #144910)

Let the module resolve the file address for you:

Address so_addr;
if (!sc.module_sp->ResolveFileAddress(file_addr, so_addr))
  return;
source/Expression/DWARFExpression.cpp
1385–1387 ↗(On Diff #144910)

Be sure there is a test that tests expressions on global variables before we run and before we have a section load list. The result of an expression for a global variable can end up using the file address and currently can read from the object file's data section. Just make sure this still works. Seems like it will.

This revision now requires changes to proceed.May 3 2018, 7:30 AM
aprantl updated this revision to Diff 145032.May 3 2018, 9:39 AM

Thanks for the excellent feedback! Addressed comments.

clayborg accepted this revision.May 3 2018, 9:47 AM
This revision is now accepted and ready to land.May 3 2018, 9:47 AM
This revision was automatically updated to reflect the committed changes.
labath added a comment.May 3 2018, 1:17 PM

It looks like this has caused a bunch of tests to crash on linux http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/22800. The crashes seem to be happening during printing of global variables ( HandleCommand(command = "target variable my_global_char") and such). Can you take a look?

Yes, I must have added a mistake in my last edit before landing this. I reverted that patch for now.

As Greg suspected, the problem was that target was null. I rewrote that patch to not use a SymbolContext in the first place.