DWARFExpression: Convert file addresses to load addresses early on

[aprantl]

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

[labath]

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]

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.

[aprantl]

Thanks for the excellent feedback! Addressed comments.

[labath]

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?

[aprantl]

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

[aprantl]

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

lldb/trunk/include/lldb/Core/Value.h

public:
   Status GetValueAsData(ExecutionContext *exe_ctx, DataExtractor &data,
                         uint32_t data_offset,
                         Module *module); // Can be nullptr
 
   static const char *GetValueTypeAsCString(ValueType context_type);
 
   static const char *GetContextTypeAsCString(ContextType context_type);
 
+  /// Convert this value's file address to a load address, if possible.
+  void ConvertToLoadAddress(SymbolContext sc);
+
   bool GetData(DataExtractor &data);
 
   void Clear();
 
 protected:
   Scalar m_value;
   Vector m_vector;
   CompilerType m_compiler_type;

lldb/trunk/source/Core/Value.cpp

const char *Value::GetContextTypeAsCString(ContextType context_type) {
   case eContextTypeLLDBType:
     return "Type *";
   case eContextTypeVariable:
     return "Variable *";
   };
   return "???";
 }
 
+void Value::ConvertToLoadAddress(SymbolContext sc) {
+  if (GetValueType() != eValueTypeFileAddress)
+    return;
+
+  if (!sc.module_sp)
+    return;
+
+  lldb::addr_t file_addr = GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
+  if (file_addr == LLDB_INVALID_ADDRESS)
+    return;
+
+  Address so_addr;
+  if (!sc.module_sp->ResolveFileAddress(file_addr, so_addr))
+    return;
+  lldb::addr_t load_addr = so_addr.GetLoadAddress(sc.target_sp.get());
+  if (load_addr == LLDB_INVALID_ADDRESS)
+    return;
+
+  SetValueType(Value::eValueTypeLoadAddress);
+  GetScalar() = load_addr;
+}
+
 ValueList::ValueList(const ValueList &rhs) { m_values = rhs.m_values; }
 
 const ValueList &ValueList::operator=(const ValueList &rhs) {
   m_values = rhs.m_values;
   return *this;
 }
 
 void ValueList::PushValue(const Value &value) { m_values.push_back(value); }

lldb/trunk/source/Core/ValueObjectVariable.cpp

if (expr.Evaluate(&exe_ctx, nullptr, loclist_base_load_addr, nullptr,
       case Value::eValueTypeHostAddress:
         // The DWARF expression result was an address in the inferior process.
         // If this variable is an aggregate type, we just need the address as
         // the main value as all child variable objects will rely upon this
         // location and add an offset and then read their own values as needed.
         // If this variable is a simple type, we read all data for it into
         // m_data. Make sure this type has a value before we try and read it
 
-        // If we have a file address, convert it to a load address if we can.
-        if (value_type == Value::eValueTypeFileAddress && process_is_alive) {
-          lldb::addr_t file_addr =
-              m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
-          if (file_addr != LLDB_INVALID_ADDRESS) {
-            SymbolContext var_sc;
-            variable->CalculateSymbolContext(&var_sc);
-            if (var_sc.module_sp) {
-              ObjectFile *objfile = var_sc.module_sp->GetObjectFile();
-              if (objfile) {
-                Address so_addr(file_addr, objfile->GetSectionList());
-                lldb::addr_t load_addr = so_addr.GetLoadAddress(target);
-                if (load_addr != LLDB_INVALID_ADDRESS) {
-                  m_value.SetValueType(Value::eValueTypeLoadAddress);
-                  m_value.GetScalar() = load_addr;
-                }
-              }
-            }
-          }
-        }
+        // If we have a file address, convert it to a load address if we can.
+        m_value.ConvertToLoadAddress(var_sc);
 
         if (!CanProvideValue()) {
           // this value object represents an aggregate type whose children have
           // values, but this object does not. So we say we are changed if our
           // location has changed.
           SetValueDidChange(value_type != old_value.GetValueType() ||
                             m_value.GetScalar() != old_value.GetScalar());
         } else {

lldb/trunk/source/Expression/DWARFExpression.cpp

if (log && log->GetVerbose()) {
       }
       log->Printf("0x%8.8" PRIx64 ": %s", op_offset, DW_OP_value_to_name(op));
     }
     switch (op) {
     //----------------------------------------------------------------------
     // The DW_OP_addr operation has a single operand that encodes a machine
     // address and whose size is the size of an address on the target machine.
     //----------------------------------------------------------------------
-    case DW_OP_addr:
+    case DW_OP_addr: {
       stack.push_back(Scalar(opcodes.GetAddress(&offset)));
       stack.back().SetValueType(Value::eValueTypeFileAddress);
+      auto sc = frame->GetSymbolContext(eSymbolContextFunction);
+      stack.back().ConvertToLoadAddress(sc);
       break;
+    }
 
     //----------------------------------------------------------------------
     // The DW_OP_addr_sect_offset4 is used for any location expressions in
     // shared libraries that have a location like:
     //  DW_OP_addr(0x1000)
     // If this address resides in a shared library, then this virtual address
     // won't make sense when it is evaluated in the context of a running
     // process where shared libraries have been slid. To account for this, this