[Lldb-commits] [lldb] ValueImpl::GetSP was calling GetError before acquiring the StopLocker. (PR #225976)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Sep 30 11:55:57 PDT 2026
https://github.com/jimingham updated https://github.com/llvm/llvm-project/pull/225976
>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 1/3] 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)
>From ef57901b8bd921f13dd3a81fc13e474971bd6c2a Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Wed, 23 Sep 2026 17:24:23 -0700
Subject: [PATCH 2/3] Formatting and a few comment fixes.
---
lldb/include/lldb/ValueObject/ValueObject.h | 2 +-
lldb/source/ValueObject/ValueObject.cpp | 7 ++-----
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/lldb/include/lldb/ValueObject/ValueObject.h b/lldb/include/lldb/ValueObject/ValueObject.h
index d8aefcb658aa5..1354ff8722eaf 100644
--- a/lldb/include/lldb/ValueObject/ValueObject.h
+++ b/lldb/include/lldb/ValueObject/ValueObject.h
@@ -480,7 +480,7 @@ 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
+ /// Updates the ValueObject's value if needed, and then returns 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();
diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp
index a55a01edfe6fb..a045c5dd9bf96 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -291,9 +291,7 @@ const Status &ValueObject::GetError() {
return m_error;
}
-const Status &ValueObject::PeekError() {
- return m_error;
-}
+const Status &ValueObject::PeekError() { return m_error; }
const char *ValueObject::GetLocationAsCStringImpl(const Value &value,
const DataExtractor &data) {
@@ -3899,7 +3897,7 @@ lldb::ValueObjectSP ValueImpl::GetSP(Process::StopLocker &stop_locker,
Target *target = value_sp->GetTargetSP().get();
if (!target) {
- // If this ValueObject holds an error, then it is valuable for that.
+ // If this ValueObject holds an error, then it is valuable for that.
if (value_sp->GetError().Fail())
return value_sp;
return ValueObjectSP();
@@ -3925,7 +3923,6 @@ lldb::ValueObjectSP ValueImpl::GetSP(Process::StopLocker &stop_locker,
// 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;
>From 3864b93dfc34e722c93ae9c28baf88dc9c80eebd Mon Sep 17 00:00:00 2001
From: Jim Ingham <jingham at apple.com>
Date: Wed, 30 Sep 2026 11:54:59 -0700
Subject: [PATCH 3/3] Review suggestions on the docs.
---
lldb/include/lldb/ValueObject/ValueObject.h | 8 ++++----
lldb/source/ValueObject/ValueObject.cpp | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/lldb/include/lldb/ValueObject/ValueObject.h b/lldb/include/lldb/ValueObject/ValueObject.h
index 1354ff8722eaf..da50131adc988 100644
--- a/lldb/include/lldb/ValueObject/ValueObject.h
+++ b/lldb/include/lldb/ValueObject/ValueObject.h
@@ -481,12 +481,12 @@ class ValueObject {
// The functions below should NOT be modified by subclasses
/// Updates the ValueObject's value if needed, and then returns 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.
+ /// current error state. This function should only be called if the stop locker
+ /// is held so the process can't resume while the update 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.
+ /// Return the current error without updating. Only use this function when you
+ /// need the error but it is not safe to update the value.
const Status &PeekError();
ConstString GetName() const { return m_name; }
diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp
index a045c5dd9bf96..2f62c221719da 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -3913,7 +3913,7 @@ lldb::ValueObjectSP ValueImpl::GetSP(Process::StopLocker &stop_locker,
// 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
+ // we can't call GetError here, because that would call UpdateValueIfNeeded
// which isn't safe to do without holding the stop locker.
if (value_sp->PeekError().Fail())
return value_sp;
More information about the lldb-commits
mailing list