DWARFExpression: Resolve file addresses in the linked module

[aprantl]

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

[clayborg]

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.

[clayborg]

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.

[clayborg]

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]

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

[jingham]

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.

[aprantl]

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]

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.

[jingham]

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.

[vsk]

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]

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]

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.

include/lldb/Expression/DWARFExpression.h

public:
   ///
   /// @return
   ///     LLDB_INVALID_ADDRESS if the location doesn't contain a
   ///     DW_OP_addr for \a op_addr_idx, otherwise a valid file address
   //------------------------------------------------------------------
   lldb::addr_t GetLocation_DW_OP_addr(uint32_t op_addr_idx, bool &error) const;
 
   bool Update_DW_OP_addr(lldb::addr_t file_addr);
   
+  void SetModule(const lldb::ModuleSP &module) { m_module_wp = module; }
+
   bool ContainsThreadLocalStorage() const;
 
   bool LinkThreadLocalStorage(
       lldb::ModuleSP new_module_sp,
       std::function<lldb::addr_t(lldb::addr_t file_addr)> const
           &link_address_callback);
 
   //------------------------------------------------------------------

packages/Python/lldbsuite/test/functionalities/target_var/Makefile

+LEVEL = ../../make
+
+include $(LEVEL)/Makefile.rules
+
+a.out: globals.ll
+  $(CC) $(CFLAGS) -g -c $^ -o globals.o
+  $(LD) $(LDFLAGS) -g globals.o -o $@
+
+clean::
+  rm -rf globals.o a.out *.dSYM

packages/Python/lldbsuite/test/functionalities/target_var/TestTargetVar.py

+"""
+Test that target var can resolve complex DWARF expressions.
+"""
+
+import lldb
+import sys
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class targetCommandTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipUnlessDarwin
+    @skipIfDarwinEmbedded           # needs x86_64
+    @skipIf(debug_info="gmodules")  # not relevant
+    def testTargetVarExpr(self):
+        self.build()
+        lldbutil.run_to_name_breakpoint(self, 'main')
+        self.expect("target variable i", substrs=['i', '42'])

packages/Python/lldbsuite/test/functionalities/target_var/globals.c

+int i = 42;
+int *p = &i;
+
+int main() {
+  return *p;
+}

packages/Python/lldbsuite/test/functionalities/target_var/globals.ll

+source_filename = "globals.c"
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.14.0"
+
+@i = global i32 42, align 4
+@p = global i32* @i, align 8, !dbg !0, !dbg !6
+
+; Function Attrs: noinline nounwind optnone ssp uwtable
+define i32 @main() #0 !dbg !15 {
+entry:
+  %retval = alloca i32, align 4
+  store i32 0, i32* %retval, align 4
+  %0 = load i32*, i32** @p, align 8, !dbg !18
+  %1 = load i32, i32* %0, align 4, !dbg !18
+  ret i32 %1, !dbg !18
+}
+
+attributes #0 = { noinline nounwind optnone ssp uwtable }
+
+!llvm.dbg.cu = !{!2}
+!llvm.module.flags = !{!10, !11, !12, !13}
+!llvm.ident = !{!14}
+
+!0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression(DW_OP_deref))
+!1 = distinct !DIGlobalVariable(name: "i", scope: !2, file: !3, line: 1, type: !9, isLocal: false, isDefinition: true)
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, emissionKind: FullDebug, globals: !5, nameTableKind: None)
+!3 = !DIFile(filename: "globals.c", directory: "/")
+!4 = !{}
+!5 = !{!0, !6}
+!6 = !DIGlobalVariableExpression(var: !7, expr: !DIExpression())
+!7 = distinct !DIGlobalVariable(name: "p", scope: !2, file: !3, line: 2, type: !8, isLocal: false, isDefinition: true)
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !9, size: 64)
+!9 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!10 = !{i32 2, !"Dwarf Version", i32 4}
+!11 = !{i32 2, !"Debug Info Version", i32 3}
+!12 = !{i32 1, !"wchar_size", i32 4}
+!13 = !{i32 7, !"PIC Level", i32 2}
+!14 = !{!"clang version 8.0.0 (trunk 340838) (llvm/trunk 340843)"}
+!15 = distinct !DISubprogram(name: "main", scope: !3, file: !3, line: 4, type: !16, isLocal: false, isDefinition: true, scopeLine: 4, isOptimized: false, unit: !2, retainedNodes: !4)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!9}
+!18 = !DILocation(line: 5, scope: !15)

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

if (num_attributes > 0) {
             GetObjectFile()->GetModule()->ReportError(
                 "0x%8.8x: %s has an invalid location: %s", die.GetOffset(),
                 die.GetTagAsCString(), strm.GetData());
           }
           if (location_DW_OP_addr != LLDB_INVALID_ADDRESS)
             is_static_lifetime = true;
         }
         SymbolFileDWARFDebugMap *debug_map_symfile = GetDebugMapSymfile();
+        if (debug_map_symfile)
+          // Set the module of the expression to the linked module
+          // instead of the oject file so the relocated address can be
+          // found there.
+          location.SetModule(debug_map_symfile->GetObjectFile()->GetModule());
 
         if (is_static_lifetime) {
           if (is_external)
             scope = eValueTypeVariableGlobal;
           else
             scope = eValueTypeVariableStatic;
 
           if (debug_map_symfile) {