[Lldb-commits] [lldb] ValueImpl::GetSP was calling GetError before acquiring the StopLocker. (PR #225976)

[via lldb-commits]

https://github.com/jimingham created https://github.com/llvm/llvm-project/pull/225976

GetError calls UpdateValueIfNeeded, and if you do that without keeping the process stopped, Resume could invalidate structures needed by UpdateValueIfNeeded, leading to crashes.

This was the cause of a small trickle of crashes we've been seeing when lldb is used under a GUI (in this case Xcode) that has more concurrency that a command-line lldb session.  The typical example showed the internal-state-thread and the gdb-remote async thread either resuming the target or waiting for it to stop again, and then another thread was servicing an SBValue::IsInScope() or SBValue::GetType() or some other such API.  That called ValueImpl::GetSP->ValueObject::UpdateValueIfNeeded and then it crashed fetching registers from the ValueObject's StackFrame or some other part of the stopped process state.

So in this patch, I added a PeekError that only fetches the current error but doesn't update, and use that in the one case where we're returning the ValueObjectSP held by the SBValue but can't get the stop locker.

I couldn't figure out how to craft a test case that failed the way our reported crashes did.  You have to be pretty unlucky to hit this crash.

>From 17841d06b9d9448fbbb2c2a940f5e120d01fa714 Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Wed, 23 Sep 2026 16:53:20 -0700
Subject: [PATCH] ValueImpl::GetSP was calling GetError before acquiring the
 StopLocker. GetError calls UpdateValueIfNeeded, and if you do that without
 keeping the process stopped, Resume could invalidate structures needed by
 UpdateValueIfNeeded, leading to crashes.

This was the cause of a small trickle of crashes we've been seeing when
lldb is used under a GUI (in this case Xcode) that has more concurrency
that a command-line lldb session.

I couldn't figure out how to craft a test case that failed the way our
reported crashes did.  You have to be pretty unlucky to hit this crash.
---
 lldb/include/lldb/ValueObject/ValueObject.h |  7 ++++++
 lldb/source/ValueObject/ValueObject.cpp     | 25 +++++++++++++++++----
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/lldb/include/lldb/ValueObject/ValueObject.h b/lldb/include/lldb/ValueObject/ValueObject.h
index c336213c84e25..d8aefcb658aa5 100644
--- a/lldb/include/lldb/ValueObject/ValueObject.h
+++ b/lldb/include/lldb/ValueObject/ValueObject.h
@@ -480,8 +480,15 @@ class ValueObject {
   virtual bool GetDeclaration(Declaration &decl);
 
   // The functions below should NOT be modified by subclasses
+  /// Updates the ValueObject's value if needed, and then return the
+  /// current error state.  This should only be called if the stop locker is
+  /// held so the process can't resume while this is in flight.
   const Status &GetError();
 
+  /// Return the current error without updating.  Only use this when you need
+  /// the error but it is not safe to update the value.
+  const Status &PeekError();
+
   ConstString GetName() const { return m_name; }
 
   /// Returns a unique id for this ValueObject.
diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp
index e7d3b7ce5be6a..430d7639b1c23 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -291,6 +291,10 @@ const Status &ValueObject::GetError() {
   return m_error;
 }
 
+const Status &ValueObject::PeekError() {
+  return m_error;
+}
+
 const char *ValueObject::GetLocationAsCStringImpl(const Value &value,
                                                   const DataExtractor &data) {
   if (UpdateValueIfNeeded(false)) {
@@ -3893,12 +3897,13 @@ lldb::ValueObjectSP ValueImpl::GetSP(Process::StopLocker &stop_locker,
   lldb::ValueObjectSP value_sp = m_valobj_sp;
 
   Target *target = value_sp->GetTargetSP().get();
-  // If this ValueObject holds an error, then it is valuable for that.
-  if (value_sp->GetError().Fail())
-    return value_sp;
 
-  if (!target)
+  if (!target) {
+  // If this ValueObject holds an error, then it is valuable for that.
+    if (value_sp->GetError().Fail())
+      return value_sp;
     return ValueObjectSP();
+  }
 
   api_mutex = target->GetAPIMutex();
   lock = std::unique_lock<TargetAPIMutex>(api_mutex);
@@ -3909,9 +3914,21 @@ lldb::ValueObjectSP ValueImpl::GetSP(Process::StopLocker &stop_locker,
     // is running. If you want to look at values, pause the process, then
     // look.
     error = Status::FromErrorString("process must be stopped.");
+    // We still want to return a value object if it was in an error state, but
+    // we can't call GetError here, since that would call UpdateValueIfNeeded
+    // which isn't safe to do without holding the stop locker.
+    if (value_sp->PeekError().Fail())
+      return value_sp;
+
     return ValueObjectSP();
   }
 
+  // Now we can safely get the ValueObject to update itself and if that results
+  // in an error, return this ValueObject since it holds the error:
+  // If this ValueObject holds an error, then it is valuable for that.
+  if (value_sp->GetError().Fail())
+    return value_sp;
+
   if (m_use_dynamic != eNoDynamicValues) {
     ValueObjectSP dynamic_sp = value_sp->GetDynamicValue(m_use_dynamic);
     if (dynamic_sp)