[Lldb-commits] [lldb] [lldb] Restore register state if PrepareTrivialCall fails (PR #129038)

[David Spickett via lldb-commits]

https://github.com/DavidSpickett updated https://github.com/llvm/llvm-project/pull/129038

>From 67e0bd38889c1a62c9f457432f9e9d46c6ece8dc Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Wed, 26 Feb 2025 10:01:39 +0000
Subject: [PATCH 1/2] [lldb] Restore register state if PrepareTrivialCall fails

Fixes #124269

PrepareTrivalCall always had the possibility of failing, but
given that it only wrote to general purpose registers, if it
did, you had bigger problems.

When it failed, we did not mark the thread plan valid and
when it was torn down we didn't try to restore the register
state. This meant that if you tried to continue, the program
unlikely to work.

When I added GCS, I needed to handle the situation where the
GCS pointer points to unmapped memory and we fail to write
the extra entry we need. So I added code to restore the gcspr_el0
register specifically if this happened, and ordered the operations
such that we tried this first.

In this change I've made the teardown of an invalid thread plan
restore the register state if one was saved. It may be there isn't
one if ConstructorSetup fails, but this is ok because that function
does not modify anything.

Now that we're doing that, I don't need the GCS specific code
anymore, and all thread plans are protected from this in the rare
event something does fail.

Testing is done by the existing GCS test case that points
the gcspr into unmapped memory which causes PrepareTrivialCall
to fail.

I tried adding a simulated test using a mock gdb server. This
was not possible because they all use DynamicLoaderStatic which
disables all JIT features.
---
 lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp |  7 +------
 lldb/source/Target/ThreadPlanCallFunction.cpp     | 13 ++++++++++++-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
index 280ec5ba37100..4bca879143a33 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
@@ -102,12 +102,7 @@ static Status PushToLinuxGuardedControlStack(addr_t return_addr,
   size_t wrote = thread.GetProcess()->WriteMemory(gcspr_el0, &return_addr,
                                                   sizeof(return_addr), error);
   if ((wrote != sizeof(return_addr) || error.Fail())) {
-    // When PrepareTrivialCall fails, the register context is not restored,
-    // unlike when an expression fails to execute. This is arguably a bug,
-    // see https://github.com/llvm/llvm-project/issues/124269.
-    // For now we are handling this here specifically. We can assume this
-    // write will work as the one to decrement the register did.
-    reg_ctx->WriteRegisterFromUnsigned(gcspr_el0_info, gcspr_el0 + 8);
+    // gcspr_el0 will be restored by the ThreadPlan's DoTakedown.
     return Status("Failed to write new Guarded Control Stack entry.");
   }
 
diff --git a/lldb/source/Target/ThreadPlanCallFunction.cpp b/lldb/source/Target/ThreadPlanCallFunction.cpp
index 50dcb66b9719f..218111d4faf60 100644
--- a/lldb/source/Target/ThreadPlanCallFunction.cpp
+++ b/lldb/source/Target/ThreadPlanCallFunction.cpp
@@ -174,8 +174,20 @@ void ThreadPlanCallFunction::ReportRegisterState(const char *message) {
 
 void ThreadPlanCallFunction::DoTakedown(bool success) {
   Log *log = GetLog(LLDBLog::Step);
+  Thread &thread = GetThread();
 
   if (!m_valid) {
+    // If ConstructorSetup was succesfull but PrepareTrivialCall was not,
+    // we will have a saved register state and potentially modified registers.
+    // Restore those.
+    if (m_stored_thread_state.register_backup_sp)
+      if (!thread.RestoreRegisterStateFromCheckpoint(m_stored_thread_state))
+        LLDB_LOGF(
+            log,
+            "ThreadPlanCallFunction(%p): Failed to restore register state from "
+            "invalid plan that contained a saved register state.",
+            static_cast<void *>(this));
+
     // Don't call DoTakedown if we were never valid to begin with.
     LLDB_LOGF(log,
               "ThreadPlanCallFunction(%p): Log called on "
@@ -185,7 +197,6 @@ void ThreadPlanCallFunction::DoTakedown(bool success) {
   }
 
   if (!m_takedown_done) {
-    Thread &thread = GetThread();
     if (success) {
       SetReturnValue();
     }

>From 5f6bce5c8d6fffc84c1486c80e537dd4d4409c3d Mon Sep 17 00:00:00 2001
From: David Spickett <david.spickett at linaro.org>
Date: Fri, 28 Feb 2025 15:57:26 +0000
Subject: [PATCH 2/2] no longer relevant

---
 lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
index 4bca879143a33..25803c9799ce4 100644
--- a/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
+++ b/lldb/source/Plugins/ABI/AArch64/ABISysV_arm64.cpp
@@ -145,8 +145,6 @@ bool ABISysV_arm64::PrepareTrivialCall(Thread &thread, addr_t sp,
   if (args.size() > 8)
     return false;
 
-  // Do this first, as it's got the most chance of failing (though still very
-  // low).
   if (GetProcessSP()->GetTarget().GetArchitecture().GetTriple().isOSLinux()) {
     Status err = PushToLinuxGuardedControlStack(return_addr, reg_ctx, thread);
     // If we could not manage the GCS, the expression will certainly fail,