[lldb] Remove unnecessary calls to Fix{Code,Data}Address

[felipepiovezan]

LLDB has since moved to a model where addresses are fixed when they actually need to be used.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

LLDB has since moved to a model where addresses are fixed when they actually need to be used.

---

Full diff: https://github.com/llvm/llvm-project/pull/150537.diff

2 Files Affected:

• (modified) lldb/source/Expression/DWARFExpression.cpp (-2)
• (modified) lldb/source/Target/RegisterContextUnwind.cpp (-9)

diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp
index 79bc6c87fa9c5..c07eba048b988 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -908,8 +908,6 @@ static llvm::Error Evaluate_DW_OP_deref(DWARFExpression::Stack &stack,
               " for DW_OP_deref",
               pointer_addr),
           error.takeError());
-    if (ABISP abi_sp = process->GetABI())
-      pointer_value = abi_sp->FixCodeAddress(pointer_value);
     stack.back().GetScalar() = pointer_value;
     stack.back().ClearContext();
   } break;
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp
index 880300d0637fb..edacdea7c4dec 100644
--- a/lldb/source/Target/RegisterContextUnwind.cpp
+++ b/lldb/source/Target/RegisterContextUnwind.cpp
@@ -1966,14 +1966,10 @@ bool RegisterContextUnwind::ReadFrameAddress(
           GetRegisterInfoAtIndex(cfa_reg.GetAsKind(eRegisterKindLLDB));
       RegisterValue reg_value;
       if (reg_info) {
-        if (abi_sp)
-          cfa_reg_contents = abi_sp->FixDataAddress(cfa_reg_contents);
         Status error = ReadRegisterValueFromMemory(
             reg_info, cfa_reg_contents, reg_info->byte_size, reg_value);
         if (error.Success()) {
           address = reg_value.GetAsUInt64();
-          if (abi_sp)
-            address = abi_sp->FixCodeAddress(address);
           UnwindLogMsg(
               "CFA value via dereferencing reg %s (%d): reg has val 0x%" PRIx64
               ", CFA value is 0x%" PRIx64,
@@ -1994,8 +1990,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
     RegisterNumber cfa_reg(m_thread, row_register_kind,
                            fa.GetRegisterNumber());
     if (ReadGPRValue(cfa_reg, cfa_reg_contents)) {
-      if (abi_sp)
-        cfa_reg_contents = abi_sp->FixDataAddress(cfa_reg_contents);
       if (cfa_reg_contents == LLDB_INVALID_ADDRESS || cfa_reg_contents == 0 ||
           cfa_reg_contents == 1) {
         UnwindLogMsg(
@@ -2030,8 +2024,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
         dwarfexpr.Evaluate(&exe_ctx, this, 0, nullptr, nullptr);
     if (result) {
       address = result->GetScalar().ULongLong();
-      if (ABISP abi_sp = m_thread.GetProcess()->GetABI())
-        address = abi_sp->FixCodeAddress(address);
 
       UnwindLogMsg("CFA value set by DWARF expression is 0x%" PRIx64,
                    address);
@@ -2072,7 +2064,6 @@ bool RegisterContextUnwind::ReadFrameAddress(
   }
   case UnwindPlan::Row::FAValue::isConstant: {
     address = fa.GetConstant();
-    address = m_thread.GetProcess()->FixDataAddress(address);
     UnwindLogMsg("CFA value set by constant is 0x%" PRIx64, address);
     return true;
   }

[jasonmolenda]

Felipe and I were discussing this earlier. We started (I started) with the model of removing metadata bits -- e.g. Pointer Authentication, or Top Byte Ignore -- from addresses as early as possible in the lldb codebase. We would pass around "Fixed" or sanitized or stripped address which only had the addressable bits internally. For user visible values, e.g. a pointer authenticated pointer value, we print the value with metadata bits, and then we print it separately with only the addressable bits, if it points to a symbol so we can be sure it's an address.

c. January 2022 ([lldb] Remove non-address bits from read/write addresses in lldb), we started removing metadata bits from addr_t's before we ReadMemory or WriteMemory the addresses. So it's possible/safe for us to preserve metadata bits a lot further through lldb's handling of the address, e.g. if they might be used in an expression running in the inferior, while still being able to read or write data at that address. So a lot of our initial address clearing is no longer needed, and may cause problems when those metadata bits need preserving for correct processing.

[jasonmolenda]

I believe this change is safe and correct, but I think Felipe is going to kick off a testsuite run with arch arm64e (where pointer authentication is enabled on Darwin) to try to stress these codepaths and confirm that our understanding is correct.

[DavidSpickett]

Your idea sounds fine, no idea if any of these are load bearing but test suites are the only way to find out. If Mac is fine go ahead and land this, the AArch64 Linux bot has top byte ignore and pointer authentication available.

[felipepiovezan]

Your idea sounds fine, no idea if any of these are load bearing but test suites are the only way to find out. If Mac is fine go ahead and land this, the AArch64 Linux bot has top byte ignore and pointer authentication available.

Somehow I missed this comment, otherwise I would have replied earlier, apologies!
When testing arm64e, we have indeed found two "sinks" (i.e. consumers of pointers) where LLDB still needs to clean up the pointers: in the StackID and in expression evaluation.

#152796
#152798

[felipepiovezan]

Now that the existing patches are merged, I'll do some more local testing before going ahead with this one (I had already tested them all together, but it's been a while)

[felipepiovezan]

This has been implemented over many other prs