Page MenuHomePhabricator

DWARFExpression: Resolve file addresses in the linked module
ClosedPublic

Authored by aprantl on Sep 28 2018, 5:43 PM.

Details

Summary

This is a follow-up to https://reviews.llvm.org/D46362.
When evaluating a complex expression in DWARFExpression::Evaluate, file addresses must be resolved to load addresses before we can perform operations such as DW_OP_deref on them.

For this the address goes through three steps

  1. Read the file address as stored in the DWARF
  2. Link/relocate the file address (when reading from a .dSYM, this is a no-op)
  3. Convert the file address to a load address.

D46362 implemented step (3) by resolving the file address using the Module that the original DWARF came from. In the case of a dSYM that is correct, but when reading from .o files, we need to look up relocated/linked addresses, so the right place to look them up is the current frame's module. This patch fixes that by getting the module from the ExecutionContext. PLease let me know if there is a better way to get from the .o-Module to the linked Module that contains that Module.

A word a bout the unorthodox testcase: The motivating testcase for this fix is in Swift, but I managed to hand-modify LLVM-IR for a trivial C program to exhibit the same problem, so we can fix this in llvm.org.

rdar://problem/44689915

Diff Detail

Repository
rLLDB LLDB

Event Timeline

aprantl created this revision.Sep 28 2018, 5:43 PM
aprantl edited subscribers, added: lldb-commits; removed: llvm-commits.Sep 28 2018, 5:43 PM
clayborg requested changes to this revision.Sep 30 2018, 11:37 AM

Any idea why the module isn't set correctly in certain dwarf expressions? Any variable that is created from debug info that pertains to a module should have its DWARFExpression created with the correct module. A DWARFExpression that has no module, yet it has file addresses with DW_OP_addr, was not created correctly. If you grab a global variable from module "a.out" and create a DWARFExpression that doesn't have its module set correctly, but we are stopped in libfoo.dylib in some frame inside there, and then evaluate the expression it will evaluate incorrectly or not at all if the file address doesn't match something in the current frame. The right fix is to ensure the DWARFExpression is created correctly with a module.

This revision now requires changes to proceed.Sep 30 2018, 11:37 AM

See SymbolFileDWARF::ParseVariableDIE. It has code that links up the DW_OP_addr:

if (is_static_lifetime) {
  if (is_external)
    scope = eValueTypeVariableGlobal;
  else
    scope = eValueTypeVariableStatic;

  if (debug_map_symfile) {
    // When leaving the DWARF in the .o files on darwin, when we have a
    // global variable that wasn't initialized, the .o file might not
    // have allocated a virtual address for the global variable. In
    // this case it will have created a symbol for the global variable
    // that is undefined/data and external and the value will be the
    // byte size of the variable. When we do the address map in
    // SymbolFileDWARFDebugMap we rely on having an address, we need to
    // do some magic here so we can get the correct address for our
    // global variable. The address for all of these entries will be
    // zero, and there will be an undefined symbol in this object file,
    // and the executable will have a matching symbol with a good
    // address. So here we dig up the correct address and replace it in
    // the location for the variable, and set the variable's symbol
    // context scope to be that of the main executable so the file
    // address will resolve correctly.

This is what needs to be fixed.

If this isn't fixing up cases where the global does have an address (not just set to zero), we need to fix the code so it relinks the global address correctly.

aprantl updated this revision to Diff 167827.Oct 1 2018, 2:01 PM

Thanks, that was excellent feedback! Updated the patch to set the expression's module instead.

The only thing that worries me about this fix is that it's addressing a problem that's a pretty easy mistake to make. Is there ever a case that you would want to resolve a DWARF expression in the context of a .o file module? If not, is there some way we can make this happen automatically. This fix seems a bit ad hoc.

I could move it further up in the same function if that is what you mean? Then it would also apply to (hypothetical local variables that refer to a DW_OP_addr). I could imagine that this might be useful in some languages that want to refer to some static type metadata.

clayborg accepted this revision.Oct 2 2018, 9:47 AM

Jim: we are already linking the address for the DW_OP_addr using the debug map and no .o files are currently expected to be able to link an unlinked address into a file address as nothing in the .o file knows about the existence of the debug map object file, so this is the right fix.

This revision is now accepted and ready to land.Oct 2 2018, 9:47 AM

I was thinking more like if it's possible to figure out that a given module has a debug_map parent, "SetModule" could automatically redirect to the that module. Then all clients have to do is SetModule with the module they have to hand and it would get the right thing. That's why I asked if there was ever a case where we'd want to run a DWARF expression using the .o file module when there's a parent debug_map.

But maybe that's overthinking it.

This revision was automatically updated to reflect the committed changes.
vsk added a subscriber: vsk.Oct 3 2018, 4:01 PM
vsk added inline comments.
packages/Python/lldbsuite/test/functionalities/target_var/globals.ll
1

Should we check in bitcode instead? That might make it easier to avoid breakage when the textual IR format changes.

davide added a subscriber: davide.Oct 3 2018, 4:26 PM
davide added inline comments.
packages/Python/lldbsuite/test/functionalities/target_var/globals.ll
1

But also will be more painful to regenerate in case, e.g. add a verifier check where this breaks.
I think it's a tradeoff anyway, I and Adrian discussed this briefly and agreed this is a decent middle ground (on one extreme, one might check ASM directly, but that seems even harder to handle).

This is honestly based on my experience of having to regenerate broken bitcode files, happy to be convinced otherwise if you have arguments :)

vsk added inline comments.Oct 3 2018, 5:22 PM
packages/Python/lldbsuite/test/functionalities/target_var/globals.ll
1

(per an off-list discussion) I'm slightly biased towards preventing build breaks, but this sounds reasonable. A third option might be to check in both the bitcode (which is compiled) and the IR (which serves as a reference for regenerating the bitcode); that could address all/most of the concerns.