Skip to content

Conversation

@felipepiovezan
Copy link
Contributor

@felipepiovezan felipepiovezan commented Sep 18, 2025

The first call, in InitializeNonZerothFrame, is inside a logging branch.
For that, it's better to log the real value instead of the fixed one.

The second call, inside RegisterContextUnwind::ReadFrameAddress, is
computing an address which is then passed to
ReadRegisterValueFromMemory, which boils down to a Process::ReadMemory,
which fixes the address if it wants to. The current variable names are
misleading, making readers believe it is the cfa value itself that is
being passed to Fix*Address; that's not the case. This commit renames
the variable to make this abundantly clear.

@llvmbot
Copy link
Member

llvmbot commented Sep 18, 2025

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

The first call, in InitializeNonZerothFrame, is inside a logging branch. For that, it's better to log the real value instead of the fixed one.

The second call, inside RegisterContextUnwind::ReadFrameAddress, is computing an address which is then passed to
ReadRegisterValueFromMemory. The current variable names are misleading, making readers believe it is the cfa value itself that is being passed to Fix*Address; that's not the case. This commit renames the variable to make this abundantly clear.


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

1 Files Affected:

  • (modified) lldb/source/Target/RegisterContextUnwind.cpp (+14-19)
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp
index 3b018c09b8b72..7f6ead1e7ccab 100644
--- a/lldb/source/Target/RegisterContextUnwind.cpp
+++ b/lldb/source/Target/RegisterContextUnwind.cpp
@@ -361,16 +361,10 @@ void RegisterContextUnwind::InitializeNonZerothFrame() {
   if (log) {
     UnwindLogMsg("pc = 0x%" PRIx64, pc);
     addr_t reg_val;
-    if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val)) {
-      if (abi_sp)
-        reg_val = abi_sp->FixDataAddress(reg_val);
+    if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val))
       UnwindLogMsg("fp = 0x%" PRIx64, reg_val);
-    }
-    if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val)) {
-      if (abi_sp)
-        reg_val = abi_sp->FixDataAddress(reg_val);
+    if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val))
       UnwindLogMsg("sp = 0x%" PRIx64, reg_val);
-    }
   }
 
   // A pc of 0x0 means it's the end of the stack crawl unless we're above a trap
@@ -2026,30 +2020,31 @@ bool RegisterContextUnwind::ReadFrameAddress(
   switch (fa.GetValueType()) {
   case UnwindPlan::Row::FAValue::isRegisterDereferenced: {
     UnwindLogMsg("CFA value via dereferencing reg");
-    RegisterNumber cfa_reg(m_thread, row_register_kind,
-                           fa.GetRegisterNumber());
-    if (ReadGPRValue(cfa_reg, cfa_reg_contents)) {
+    RegisterNumber regnum_to_deref(m_thread, row_register_kind,
+                                   fa.GetRegisterNumber());
+    addr_t reg_to_deref_contents;
+    if (ReadGPRValue(regnum_to_deref, reg_to_deref_contents)) {
       const RegisterInfo *reg_info =
-          GetRegisterInfoAtIndex(cfa_reg.GetAsKind(eRegisterKindLLDB));
+          GetRegisterInfoAtIndex(regnum_to_deref.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);
+            reg_info, reg_to_deref_contents, reg_info->byte_size, reg_value);
         if (error.Success()) {
           address = reg_value.GetAsUInt64();
           UnwindLogMsg(
               "CFA value via dereferencing reg %s (%d): reg has val 0x%" PRIx64
               ", CFA value is 0x%" PRIx64,
-              cfa_reg.GetName(), cfa_reg.GetAsKind(eRegisterKindLLDB),
-              cfa_reg_contents, address);
+              regnum_to_deref.GetName(),
+              regnum_to_deref.GetAsKind(eRegisterKindLLDB),
+              reg_to_deref_contents, address);
           return true;
         } else {
           UnwindLogMsg("Tried to deref reg %s (%d) [0x%" PRIx64
                        "] but memory read failed.",
-                       cfa_reg.GetName(), cfa_reg.GetAsKind(eRegisterKindLLDB),
-                       cfa_reg_contents);
+                       regnum_to_deref.GetName(),
+                       regnum_to_deref.GetAsKind(eRegisterKindLLDB),
+                       reg_to_deref_contents);
         }
       }
     }

The first call, in InitializeNonZerothFrame, is inside a logging branch.
For that, it's better to log the real value instead of the fixed one.

The second call, inside RegisterContextUnwind::ReadFrameAddress, is
computing an address which is then passed to
ReadRegisterValueFromMemory, which boils down to a Process::ReadMemory,
which fixes the address if it wants to. The current variable names are
misleading, making readers believe it is the cfa value itself that is
being passed to Fix*Address; that's not the case. This commit renames
the variable to make this abundantly clear.
@felipepiovezan felipepiovezan force-pushed the felipe/noop_calls_to_fixaddr branch from f6f7a69 to f7f958a Compare September 18, 2025 14:39
Copy link
Collaborator

@jasonmolenda jasonmolenda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The early Fixing of the register value before doing a memory read is from early in our adoption of AArch64 PAC where the bits would not be cleared at use-time when we read/write memory.

@felipepiovezan felipepiovezan merged commit 113357f into llvm:main Sep 18, 2025
9 checks passed
@felipepiovezan felipepiovezan deleted the felipe/noop_calls_to_fixaddr branch September 18, 2025 17:33
felipepiovezan added a commit to felipepiovezan/llvm-project that referenced this pull request Sep 18, 2025
The first call, in InitializeNonZerothFrame, is inside a logging branch.
For that, it's better to log the real value instead of the fixed one.

The second call, inside RegisterContextUnwind::ReadFrameAddress, is
computing an address which is then passed to
ReadRegisterValueFromMemory, which boils down to a Process::ReadMemory,
which fixes the address if it wants to. The current variable names are
misleading, making readers believe it is the cfa value itself that is
being passed to Fix*Address; that's not the case. This commit renames
the variable to make this abundantly clear.

(cherry picked from commit 113357f)
felipepiovezan added a commit to felipepiovezan/llvm-project that referenced this pull request Sep 19, 2025
The first call, in InitializeNonZerothFrame, is inside a logging branch.
For that, it's better to log the real value instead of the fixed one.

The second call, inside RegisterContextUnwind::ReadFrameAddress, is
computing an address which is then passed to
ReadRegisterValueFromMemory, which boils down to a Process::ReadMemory,
which fixes the address if it wants to. The current variable names are
misleading, making readers believe it is the cfa value itself that is
being passed to Fix*Address; that's not the case. This commit renames
the variable to make this abundantly clear.

(cherry picked from commit 113357f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants