[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

TIL what gcore is. One can save a corefile from within lldb too, I assume 
you've checked what that does (and with this change, it wouldn't matter either 
way).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[Jason Molenda via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0ce7037b5edd: Clear non-addressable bits from pc/fp/sp in 
unwinds (authored by jasonmolenda).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

Files:
  lldb/source/Target/RegisterContextUnwind.cpp

Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -137,9 +137,8 @@
   // (which would be a no-op in frame 0 where we get it from the register set,
   // but still a good idea to make the call here for other ABIs that may
   // exist.)
-  ABI *abi = process->GetABI().get();
-  if (abi)
-current_pc = abi->FixCodeAddress(current_pc);
+  if (ABISP abi_sp = process->GetABI())
+current_pc = abi_sp->FixCodeAddress(current_pc);
 
   UnwindPlanSP lang_runtime_plan_sp = LanguageRuntime::GetRuntimeUnwindPlan(
   m_thread, this, m_behaves_like_zeroth_frame);
@@ -355,17 +354,23 @@
 
   // Let ABIs fixup code addresses to make sure they are valid. In ARM ABIs
   // this will strip bit zero in case we read a PC from memory or from the LR.
-  ABI *abi = process->GetABI().get();
-  if (abi)
-pc = abi->FixCodeAddress(pc);
+  ABISP abi_sp = process->GetABI();
+  if (abi_sp)
+pc = abi_sp->FixCodeAddress(pc);
 
   if (log) {
 UnwindLogMsg("pc = 0x%" PRIx64, pc);
 addr_t reg_val;
-if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val))
+if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val)) {
+  if (abi_sp)
+reg_val = abi_sp->FixDataAddress(reg_val);
   UnwindLogMsg("fp = 0x%" PRIx64, reg_val);
-if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val))
+}
+if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val)) {
+  if (abi_sp)
+reg_val = abi_sp->FixDataAddress(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
@@ -424,11 +429,11 @@
   }
 }
 
-if (abi) {
+if (abi_sp) {
   m_fast_unwind_plan_sp.reset();
   m_full_unwind_plan_sp =
   std::make_shared(lldb::eRegisterKindGeneric);
-  abi->CreateDefaultUnwindPlan(*m_full_unwind_plan_sp);
+  abi_sp->CreateDefaultUnwindPlan(*m_full_unwind_plan_sp);
   if (m_frame_type != eSkipFrame) // don't override eSkipFrame
   {
 m_frame_type = eNormalFrame;
@@ -1751,8 +1756,8 @@
   if (ReadRegisterValueFromRegisterLocation(regloc, reg_info, reg_value)) {
 old_caller_pc_value = reg_value.GetAsUInt64();
 if (ProcessSP process_sp = m_thread.GetProcess()) {
-  if (ABISP abi = process_sp->GetABI())
-old_caller_pc_value = abi->FixCodeAddress(old_caller_pc_value);
+  if (ABISP abi_sp = process_sp->GetABI())
+old_caller_pc_value = abi_sp->FixCodeAddress(old_caller_pc_value);
 }
   }
 }
@@ -1811,8 +1816,8 @@
   reg_value)) {
   new_caller_pc_value = reg_value.GetAsUInt64();
   if (ProcessSP process_sp = m_thread.GetProcess()) {
-if (ABISP abi = process_sp->GetABI())
-  new_caller_pc_value = abi->FixCodeAddress(new_caller_pc_value);
+if (ABISP abi_sp = process_sp->GetABI())
+  new_caller_pc_value = abi_sp->FixCodeAddress(new_caller_pc_value);
   }
 }
   }
@@ -1953,6 +1958,7 @@
 
   address = LLDB_INVALID_ADDRESS;
   addr_t cfa_reg_contents;
+  ABISP abi_sp = m_thread.GetProcess()->GetABI();
 
   switch (fa.GetValueType()) {
   case UnwindPlan::Row::FAValue::isRegisterDereferenced: {
@@ -1963,11 +1969,13 @@
   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 (ABISP abi_sp = m_thread.GetProcess()->GetABI())
+  if (abi_sp)
 address = abi_sp->FixCodeAddress(address);
   UnwindLogMsg(
   "CFA value via dereferencing reg %s (%d): reg has val 0x%" PRIx64
@@ -1989,6 +1997,8 @@
 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) {
 

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D152861#4420223 , @DavidSpickett 
wrote:

> I'm curious how you would end up with a signed PC value, but given this is 
> unwind it could be a value from a previous frame that was signed when stored 
> to the stack.

The darwin kernel signs sp/pc (and maybe fp too) when they're at rest inside 
the kernel, I think.  When we fetch the values for these with thread_get_state, 
they need to be run through and auth-and-clear before the values are sent to 
lldb (in debugserver aka lldb-server).   gcore isn't stripping the auth bits 
when it fetches the register contexts from the kernel, and is putting those 
values as-is in corefiles.

You're right that this shouldn't happen in a real process.  We already strip 
auth bits from $lr and spilled $lr's on the stack, where the code actually does 
sign it.  This is purely addressing an artifact of how the darwin kernel 
represents these internally.  gcore should really be clearing the auth bits 
from these register before putting them in a core file, but we need to work on 
core files that have already been created like this, so I'm starting with an 
lldb patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[David Spickett via Phabricator via lldb-commits]

DavidSpickett accepted this revision.
DavidSpickett added a comment.
This revision is now accepted and ready to land.

I'm curious how you would end up with a signed PC value, but given this is 
unwind it could be a value from a previous frame that was signed when stored to 
the stack.

LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda updated this revision to Diff 531092.
jasonmolenda added a comment.

Address Alex's feedback of not extracting the ABI pointer out of the shared 
pointer; simply use the shared pointer.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

Files:
  lldb/source/Target/RegisterContextUnwind.cpp

Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -137,9 +137,8 @@
   // (which would be a no-op in frame 0 where we get it from the register set,
   // but still a good idea to make the call here for other ABIs that may
   // exist.)
-  ABI *abi = process->GetABI().get();
-  if (abi)
-current_pc = abi->FixCodeAddress(current_pc);
+  if (ABISP abi_sp = process->GetABI())
+current_pc = abi_sp->FixCodeAddress(current_pc);
 
   UnwindPlanSP lang_runtime_plan_sp = LanguageRuntime::GetRuntimeUnwindPlan(
   m_thread, this, m_behaves_like_zeroth_frame);
@@ -355,17 +354,23 @@
 
   // Let ABIs fixup code addresses to make sure they are valid. In ARM ABIs
   // this will strip bit zero in case we read a PC from memory or from the LR.
-  ABI *abi = process->GetABI().get();
-  if (abi)
-pc = abi->FixCodeAddress(pc);
+  ABISP abi_sp = process->GetABI();
+  if (abi_sp)
+pc = abi_sp->FixCodeAddress(pc);
 
   if (log) {
 UnwindLogMsg("pc = 0x%" PRIx64, pc);
 addr_t reg_val;
-if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val))
+if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val)) {
+  if (abi_sp)
+reg_val = abi_sp->FixDataAddress(reg_val);
   UnwindLogMsg("fp = 0x%" PRIx64, reg_val);
-if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val))
+}
+if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val)) {
+  if (abi_sp)
+reg_val = abi_sp->FixDataAddress(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
@@ -424,11 +429,11 @@
   }
 }
 
-if (abi) {
+if (abi_sp) {
   m_fast_unwind_plan_sp.reset();
   m_full_unwind_plan_sp =
   std::make_shared(lldb::eRegisterKindGeneric);
-  abi->CreateDefaultUnwindPlan(*m_full_unwind_plan_sp);
+  abi_sp->CreateDefaultUnwindPlan(*m_full_unwind_plan_sp);
   if (m_frame_type != eSkipFrame) // don't override eSkipFrame
   {
 m_frame_type = eNormalFrame;
@@ -1751,8 +1756,8 @@
   if (ReadRegisterValueFromRegisterLocation(regloc, reg_info, reg_value)) {
 old_caller_pc_value = reg_value.GetAsUInt64();
 if (ProcessSP process_sp = m_thread.GetProcess()) {
-  if (ABISP abi = process_sp->GetABI())
-old_caller_pc_value = abi->FixCodeAddress(old_caller_pc_value);
+  if (ABISP abi_sp = process_sp->GetABI())
+old_caller_pc_value = abi_sp->FixCodeAddress(old_caller_pc_value);
 }
   }
 }
@@ -1811,8 +1816,8 @@
   reg_value)) {
   new_caller_pc_value = reg_value.GetAsUInt64();
   if (ProcessSP process_sp = m_thread.GetProcess()) {
-if (ABISP abi = process_sp->GetABI())
-  new_caller_pc_value = abi->FixCodeAddress(new_caller_pc_value);
+if (ABISP abi_sp = process_sp->GetABI())
+  new_caller_pc_value = abi_sp->FixCodeAddress(new_caller_pc_value);
   }
 }
   }
@@ -1953,6 +1958,7 @@
 
   address = LLDB_INVALID_ADDRESS;
   addr_t cfa_reg_contents;
+  ABISP abi_sp = m_thread.GetProcess()->GetABI();
 
   switch (fa.GetValueType()) {
   case UnwindPlan::Row::FAValue::isRegisterDereferenced: {
@@ -1963,11 +1969,13 @@
   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 (ABISP abi_sp = m_thread.GetProcess()->GetABI())
+  if (abi_sp)
 address = abi_sp->FixCodeAddress(address);
   UnwindLogMsg(
   "CFA value via dereferencing reg %s (%d): reg has val 0x%" PRIx64
@@ -1989,6 +1997,8 @@
 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) {

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

In D152861#4419197 , @bulbazord wrote:

> I noticed you're pulling a lot of raw pointers out of shared pointers. Is 
> there a particular reason you're doing that? `operator bool` on 
> `std::shared_ptr` returns true if the held pointer is non-null.

Nah, I didn't care much in these contexts whether I was holding a shared 
pointer from the lifetime point of view.  You're right it's not going to be 
more efficient to do it this way, I might as well use the shared pointers I got 
back instead of extracting the pointer out of the SP.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

I noticed you're pulling a lot of raw pointers out of shared pointers. Is there 
a particular reason you're doing that? `operator bool` on `std::shared_ptr` 
returns true if the held pointer is non-null.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152861/new/

https://reviews.llvm.org/D152861

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D152861: Clear non-addressable bits from fp/sp/lr/pc values in RegisterContextUnwind

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: JDevlieghere.
jasonmolenda added a project: LLDB.
Herald added a subscriber: kristof.beyls.
Herald added a reviewer: a.sidorin.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

The darwin kernel may have authentication bits on fields like $pc or $sp when 
the register state is thread_get_state'd with macOS Sonoma.  debugserver clears 
these bits before handing the values to lldb  arm64 corefiles created by gcore 
on MacOS Sonoma (macOS 14) will include these signed value as-is.

This patch changes RegisterContextUnwind to clear the unaddressable bits from 
sp/pc/fp/lr -- these must point to stack or code in memory.

We already clear the bits from spilled lr's because that's frequently signed 
with an ABI using ARMv8.3 ptrauth.  This patch extends that same behavior to sp 
and fp.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152861

Files:
  lldb/source/Target/RegisterContextUnwind.cpp

Index: lldb/source/Target/RegisterContextUnwind.cpp
===
--- lldb/source/Target/RegisterContextUnwind.cpp
+++ lldb/source/Target/RegisterContextUnwind.cpp
@@ -362,10 +362,16 @@
   if (log) {
 UnwindLogMsg("pc = 0x%" PRIx64, pc);
 addr_t reg_val;
-if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val))
+if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_FP, reg_val)) {
+  if (abi)
+reg_val = abi->FixDataAddress(reg_val);
   UnwindLogMsg("fp = 0x%" PRIx64, reg_val);
-if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val))
+}
+if (ReadGPRValue(eRegisterKindGeneric, LLDB_REGNUM_GENERIC_SP, reg_val)) {
+  if (abi)
+reg_val = abi->FixDataAddress(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
@@ -1953,6 +1959,7 @@
 
   address = LLDB_INVALID_ADDRESS;
   addr_t cfa_reg_contents;
+  ABI *abi = m_thread.GetProcess()->GetABI().get();
 
   switch (fa.GetValueType()) {
   case UnwindPlan::Row::FAValue::isRegisterDereferenced: {
@@ -1963,12 +1970,14 @@
   GetRegisterInfoAtIndex(cfa_reg.GetAsKind(eRegisterKindLLDB));
   RegisterValue reg_value;
   if (reg_info) {
+if (abi)
+  cfa_reg_contents = abi->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 (ABISP abi_sp = m_thread.GetProcess()->GetABI())
-address = abi_sp->FixCodeAddress(address);
+  if (abi)
+address = abi->FixCodeAddress(address);
   UnwindLogMsg(
   "CFA value via dereferencing reg %s (%d): reg has val 0x%" PRIx64
   ", CFA value is 0x%" PRIx64,
@@ -1989,6 +1998,8 @@
 RegisterNumber cfa_reg(m_thread, row_register_kind,
fa.GetRegisterNumber());
 if (ReadGPRValue(cfa_reg, cfa_reg_contents)) {
+  if (abi)
+cfa_reg_contents = abi->FixDataAddress(cfa_reg_contents);
   if (cfa_reg_contents == LLDB_INVALID_ADDRESS || cfa_reg_contents == 0 ||
   cfa_reg_contents == 1) {
 UnwindLogMsg(
@@ -2024,8 +2035,8 @@
 if (dwarfexpr.Evaluate(_ctx, this, 0, nullptr, nullptr, result,
)) {
   address = result.GetScalar().ULongLong();
-  if (ABISP abi_sp = m_thread.GetProcess()->GetABI())
-address = abi_sp->FixCodeAddress(address);
+  if (ABI *abi = m_thread.GetProcess()->GetABI().get())
+address = abi->FixCodeAddress(address);
 
   UnwindLogMsg("CFA value set by DWARF expression is 0x%" PRIx64,
address);
@@ -2076,6 +2087,8 @@
 return LLDB_INVALID_ADDRESS;
   if (!m_sym_ctx.module_sp || !m_sym_ctx.symbol)
 return LLDB_INVALID_ADDRESS;
+  if (ABI *abi = m_thread.GetProcess()->GetABI().get())
+hint = abi->FixCodeAddress(hint);
 
   hint += plan_offset;
 
@@ -2133,28 +2146,38 @@
 return false;
   }
 
+  uint32_t generic_regnum = LLDB_INVALID_REGNUM;
+  if (register_kind == eRegisterKindGeneric)
+generic_regnum = regnum;
+  else
+m_thread.GetRegisterContext()->ConvertBetweenRegisterKinds(
+register_kind, regnum, eRegisterKindGeneric, generic_regnum);
+  ABI *abi = m_thread.GetProcess()->GetABI().get();
+
   RegisterValue reg_value;
   // if this is frame 0 (currently executing frame), get the requested reg
   // contents from the actual thread registers
   if (IsFrameZero()) {
 if (m_thread.GetRegisterContext()->ReadRegister(reg_info, reg_value)) {
   value = reg_value.GetAsUInt64();
+  if (abi && generic_regnum != LLDB_INVALID_REGNUM) {
+if (generic_regnum == LLDB_REGNUM_GENERIC_PC ||
+