[lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

This rewrites DW_OP_addrx inside DWARFExpression as an DW_OP_addr so
that we can update addresses that are originally located in the
debug_addr section.

The full discussion behind this can be found in
https://discourse.llvm.org/t/dwarfexpression-and-dw-op-addrx/71627/12, but a
summary follows.

When SymbolFileDWARF::ParseVariableDIE creates DWARFExpressions for
variables whose location is an OP_addr, it knows how to remap
addresses appropriately in the DebugMap case. It then calls
DWARFExpression::Update_DW_OP_addr, which updates the value associated
with OP_addr.

However, when we have an OP_addrx, the update function does
nothing. This makes sense, as the DWARFExpression does not have a
mutable view of the debug_addr section. In non-DebugMap flows this is
not an issue, as the debug_addr contains the correct addresses of
variables. In DebugMap flows, this is problematic because the work
done by RelinkOSOAddress is lost. By updating the OP to OP_addr, we
can also update the address as required,

We also explored the alternative of relinking the debug_addr section
when we are initializing OSOs (InitOSO). However, this creates an
inconsistent story for users of
DWARFExpression::GetLocation_DW_OP_addr. This function returns an
address without telling callers whether that address came from an
OP_addr or an OP_addrx. If we preemptively relink OP_addrx results
without doing the same for OP_addr results, then callers can’t know
whether the address they got was an executable address or an object
file address. In other words, they can’t know whether they need to
call LinkOSOFileAddress on those results or not.

This patch addresses the majority of test failures when enabling DWARF
5 for MachO (over 200 test failures).

Differential Revision: https://reviews.llvm.org/D155198

Author: felipepiovezan

lldb/source/Expression/DWARFExpression.cpp

@@ -408,13 +408,33 @@ bool DWARFExpression::Update_DW_OP_addr(const DWARFUnit *dwarf_cu,
       // the heap data so "m_data" will now correctly manage the heap data.
       m_data.SetData(encoder.GetDataBuffer());
       return true;
-    } else {
-      const offset_t op_arg_size =
-          GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
-      if (op_arg_size == LLDB_INVALID_OFFSET)
-        break;
-      offset += op_arg_size;
     }
+    if (op == DW_OP_addrx) {
+      // Replace DW_OP_addrx with DW_OP_addr, since we can't modify the
+      // read-only debug_addr table.
+      // Subtract one to account for the opcode.
+      llvm::ArrayRef data_before_op = m_data.GetData().take_front(offset - 1);
+
+      // Read the addrx index to determine how many bytes it needs.
+      const lldb::offset_t old_offset = offset;
+      m_data.GetULEB128(&offset);
+      if (old_offset == offset)
+        return false;
+      llvm::ArrayRef data_after_op = m_data.GetData().drop_front(offset);
+
+      DataEncoder encoder(m_data.GetByteOrder(), m_data.GetAddressByteSize());
+      encoder.AppendData(data_before_op);
+      encoder.AppendU8(DW_OP_addr);
+      encoder.AppendAddress(file_addr);
+      encoder.AppendData(data_after_op);
+      m_data.SetData(encoder.GetDataBuffer());
+      return true;
+    }
+    const offset_t op_arg_size =
+        GetOpcodeDataSize(m_data, offset, op, dwarf_cu);
+    if (op_arg_size == LLDB_INVALID_OFFSET)
+      break;
+    offset += op_arg_size;
   }
   return false;
 }

lldb/unittests/Expression/DWARFExpressionTest.cpp

@@ -522,6 +522,11 @@ TEST_F(DWARFExpressionMockProcessTest, WASM_DW_OP_addr_index) {
   ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
   ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
   ASSERT_EQ(result.GetScalar().UInt(), 0x5678u);
+
+  ASSERT_TRUE(expr.Update_DW_OP_addr(dwarf_cu, 0xdeadbeef));
+  ASSERT_TRUE(evaluate(expr, status, result)) << status.ToError();
+  ASSERT_EQ(result.GetValueType(), Value::ValueType::LoadAddress);
+  ASSERT_EQ(result.GetScalar().UInt(), 0xdeadbeefu);
 }
 
 class CustomSymbolFileDWARF : public SymbolFileDWARF {