https://github.com/llvmbot created https://github.com/llvm/llvm-project/pull/179500
Backport 58f623c Requested by: @medismailben >From acd5a3e90ddd51ba5ca08955c9db794b837e907a Mon Sep 17 00:00:00 2001 From: Aman LaChapelle <[email protected]> Date: Thu, 29 Jan 2026 11:12:20 -0800 Subject: [PATCH] [lldb] Move ValueImpl and ValueLocker to ValueObject, NFC. (#178573) This patch moves ValueImpl and ValueLocker to ValueObject.{h,cpp}. This follows the example set in TypeImpl/SBType, where we have something that SBType uses internally that needs to be exposed in the layer below. In this case, SBValue uses ValueImpl, which wraps ValueObject. The wrapper helps avoid bugs, so we want to keep it, but the script interpreter needs to use it and said interpreter is conceptually *below* the SB layer...which means we can't use methods on SBValue. This patch is purely the code motion part of that, future patches will actually make use of this moved code. (cherry picked from commit 58f623c504d14743e465f988848f9c16dc220a3b) --- lldb/include/lldb/API/SBValue.h | 7 +- lldb/include/lldb/ValueObject/ValueObject.h | 78 +++++++++ lldb/source/API/SBValue.cpp | 166 -------------------- lldb/source/ValueObject/ValueObject.cpp | 91 +++++++++++ 4 files changed, 172 insertions(+), 170 deletions(-) diff --git a/lldb/include/lldb/API/SBValue.h b/lldb/include/lldb/API/SBValue.h index dead11fba19fe..0583f2a4983f6 100644 --- a/lldb/include/lldb/API/SBValue.h +++ b/lldb/include/lldb/API/SBValue.h @@ -13,10 +13,9 @@ #include "lldb/API/SBDefines.h" #include "lldb/API/SBType.h" +namespace lldb_private { class ValueImpl; class ValueLocker; - -namespace lldb_private { namespace python { class SWIGBridge; } @@ -490,7 +489,7 @@ class LLDB_API SBValue { /// \return /// A ValueObjectSP of the best kind (static, dynamic or synthetic) we /// can cons up, in accordance with the SBValue's settings. - lldb::ValueObjectSP GetSP(ValueLocker &value_locker) const; + lldb::ValueObjectSP GetSP(lldb_private::ValueLocker &value_locker) const; // these calls do the right thing WRT adjusting their settings according to // the target's preferences @@ -507,7 +506,7 @@ class LLDB_API SBValue { bool use_synthetic, const char *name); private: - typedef std::shared_ptr<ValueImpl> ValueImplSP; + typedef std::shared_ptr<lldb_private::ValueImpl> ValueImplSP; ValueImplSP m_opaque_sp; void SetSP(ValueImplSP impl_sp); diff --git a/lldb/include/lldb/ValueObject/ValueObject.h b/lldb/include/lldb/ValueObject/ValueObject.h index 3f9f2b5de8dbe..8a528bad15f94 100644 --- a/lldb/include/lldb/ValueObject/ValueObject.h +++ b/lldb/include/lldb/ValueObject/ValueObject.h @@ -1121,6 +1121,84 @@ class ValueObject { const ValueObject &operator=(const ValueObject &) = delete; }; +// The two classes below are used by the public SBValue API implementation. This +// is useful here because we need them in order to access the underlying +// ValueObject from SBValue without introducing a back-dependency from the API +// library to the more core libs. + +class ValueImpl { +public: + ValueImpl() = default; + + ValueImpl(lldb::ValueObjectSP in_valobj_sp, + lldb::DynamicValueType use_dynamic, bool use_synthetic, + const char *name = nullptr); + + ValueImpl(const ValueImpl &rhs) = default; + + ValueImpl &operator=(const ValueImpl &rhs); + + bool IsValid(); + + lldb::ValueObjectSP GetRootSP() { return m_valobj_sp; } + + lldb::ValueObjectSP GetSP(Process::StopLocker &stop_locker, + std::unique_lock<std::recursive_mutex> &lock, + Status &error); + + void SetUseDynamic(lldb::DynamicValueType use_dynamic) { + m_use_dynamic = use_dynamic; + } + + void SetUseSynthetic(bool use_synthetic) { m_use_synthetic = use_synthetic; } + + lldb::DynamicValueType GetUseDynamic() { return m_use_dynamic; } + + bool GetUseSynthetic() { return m_use_synthetic; } + + // All the derived values that we would make from the m_valobj_sp will share + // the ExecutionContext with m_valobj_sp, so we don't need to do the + // calculations in GetSP to return the Target, Process, Thread or Frame. It + // is convenient to provide simple accessors for these, which I do here. + lldb::TargetSP GetTargetSP() { + return m_valobj_sp ? m_valobj_sp->GetTargetSP() : lldb::TargetSP{}; + } + + lldb::ProcessSP GetProcessSP() { + return m_valobj_sp ? m_valobj_sp->GetProcessSP() : lldb::ProcessSP{}; + } + + lldb::ThreadSP GetThreadSP() { + return m_valobj_sp ? m_valobj_sp->GetThreadSP() : lldb::ThreadSP{}; + } + + lldb::StackFrameSP GetFrameSP() { + return m_valobj_sp ? m_valobj_sp->GetFrameSP() : lldb::StackFrameSP{}; + } + +private: + lldb::ValueObjectSP m_valobj_sp; + lldb::DynamicValueType m_use_dynamic; + bool m_use_synthetic; + ConstString m_name; +}; + +class ValueLocker { +public: + ValueLocker() = default; + + lldb::ValueObjectSP GetLockedSP(ValueImpl &in_value) { + return in_value.GetSP(m_stop_locker, m_lock, m_lock_error); + } + + Status &GetError() { return m_lock_error; } + +private: + Process::StopLocker m_stop_locker; + std::unique_lock<std::recursive_mutex> m_lock; + Status m_lock_error; +}; + } // namespace lldb_private #endif // LLDB_VALUEOBJECT_VALUEOBJECT_H diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp index 5b67270859da2..adc03785602e1 100644 --- a/lldb/source/API/SBValue.cpp +++ b/lldb/source/API/SBValue.cpp @@ -52,172 +52,6 @@ using namespace lldb; using namespace lldb_private; -class ValueImpl { -public: - ValueImpl() = default; - - ValueImpl(lldb::ValueObjectSP in_valobj_sp, - lldb::DynamicValueType use_dynamic, bool use_synthetic, - const char *name = nullptr) - : m_use_dynamic(use_dynamic), m_use_synthetic(use_synthetic), - m_name(name) { - if (in_valobj_sp) { - if ((m_valobj_sp = in_valobj_sp->GetQualifiedRepresentationIfAvailable( - lldb::eNoDynamicValues, false))) { - if (!m_name.IsEmpty()) - m_valobj_sp->SetName(m_name); - } - } - } - - ValueImpl(const ValueImpl &rhs) = default; - - ValueImpl &operator=(const ValueImpl &rhs) { - if (this != &rhs) { - m_valobj_sp = rhs.m_valobj_sp; - m_use_dynamic = rhs.m_use_dynamic; - m_use_synthetic = rhs.m_use_synthetic; - m_name = rhs.m_name; - } - return *this; - } - - bool IsValid() { - if (m_valobj_sp.get() == nullptr) - return false; - else { - // FIXME: This check is necessary but not sufficient. We for sure don't - // want to touch SBValues whose owning - // targets have gone away. This check is a little weak in that it - // enforces that restriction when you call IsValid, but since IsValid - // doesn't lock the target, you have no guarantee that the SBValue won't - // go invalid after you call this... Also, an SBValue could depend on - // data from one of the modules in the target, and those could go away - // independently of the target, for instance if a module is unloaded. - // But right now, neither SBValues nor ValueObjects know which modules - // they depend on. So I have no good way to make that check without - // tracking that in all the ValueObject subclasses. - TargetSP target_sp = m_valobj_sp->GetTargetSP(); - return target_sp && target_sp->IsValid(); - } - } - - lldb::ValueObjectSP GetRootSP() { return m_valobj_sp; } - - lldb::ValueObjectSP GetSP(Process::StopLocker &stop_locker, - std::unique_lock<std::recursive_mutex> &lock, - Status &error) { - if (!m_valobj_sp) { - error = Status::FromErrorString("invalid value object"); - return m_valobj_sp; - } - - 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) - return ValueObjectSP(); - - lock = std::unique_lock<std::recursive_mutex>(target->GetAPIMutex()); - - ProcessSP process_sp(value_sp->GetProcessSP()); - if (process_sp && !stop_locker.TryLock(&process_sp->GetRunLock())) { - // We don't allow people to play around with ValueObject if the process - // is running. If you want to look at values, pause the process, then - // look. - error = Status::FromErrorString("process must be stopped."); - return ValueObjectSP(); - } - - if (m_use_dynamic != eNoDynamicValues) { - ValueObjectSP dynamic_sp = value_sp->GetDynamicValue(m_use_dynamic); - if (dynamic_sp) - value_sp = dynamic_sp; - } - - if (m_use_synthetic) { - ValueObjectSP synthetic_sp = value_sp->GetSyntheticValue(); - if (synthetic_sp) - value_sp = synthetic_sp; - } - - if (!value_sp) - error = Status::FromErrorString("invalid value object"); - if (!m_name.IsEmpty()) - value_sp->SetName(m_name); - - return value_sp; - } - - void SetUseDynamic(lldb::DynamicValueType use_dynamic) { - m_use_dynamic = use_dynamic; - } - - void SetUseSynthetic(bool use_synthetic) { m_use_synthetic = use_synthetic; } - - lldb::DynamicValueType GetUseDynamic() { return m_use_dynamic; } - - bool GetUseSynthetic() { return m_use_synthetic; } - - // All the derived values that we would make from the m_valobj_sp will share - // the ExecutionContext with m_valobj_sp, so we don't need to do the - // calculations in GetSP to return the Target, Process, Thread or Frame. It - // is convenient to provide simple accessors for these, which I do here. - TargetSP GetTargetSP() { - if (m_valobj_sp) - return m_valobj_sp->GetTargetSP(); - else - return TargetSP(); - } - - ProcessSP GetProcessSP() { - if (m_valobj_sp) - return m_valobj_sp->GetProcessSP(); - else - return ProcessSP(); - } - - ThreadSP GetThreadSP() { - if (m_valobj_sp) - return m_valobj_sp->GetThreadSP(); - else - return ThreadSP(); - } - - StackFrameSP GetFrameSP() { - if (m_valobj_sp) - return m_valobj_sp->GetFrameSP(); - else - return StackFrameSP(); - } - -private: - lldb::ValueObjectSP m_valobj_sp; - lldb::DynamicValueType m_use_dynamic; - bool m_use_synthetic; - ConstString m_name; -}; - -class ValueLocker { -public: - ValueLocker() = default; - - ValueObjectSP GetLockedSP(ValueImpl &in_value) { - return in_value.GetSP(m_stop_locker, m_lock, m_lock_error); - } - - Status &GetError() { return m_lock_error; } - -private: - Process::StopLocker m_stop_locker; - std::unique_lock<std::recursive_mutex> m_lock; - Status m_lock_error; -}; - SBValue::SBValue() { LLDB_INSTRUMENT_VA(this); } SBValue::SBValue(const lldb::ValueObjectSP &value_sp) { diff --git a/lldb/source/ValueObject/ValueObject.cpp b/lldb/source/ValueObject/ValueObject.cpp index 121054e3e92ed..5b7c65a102e5d 100644 --- a/lldb/source/ValueObject/ValueObject.cpp +++ b/lldb/source/ValueObject/ValueObject.cpp @@ -3768,3 +3768,94 @@ ValueObjectSP ValueObject::Persist() { lldb::ValueObjectSP ValueObject::GetVTable() { return ValueObjectVTable::Create(*this); } + +ValueImpl::ValueImpl(lldb::ValueObjectSP in_valobj_sp, + lldb::DynamicValueType use_dynamic, bool use_synthetic, + const char *name) + : m_use_dynamic(use_dynamic), m_use_synthetic(use_synthetic), m_name(name) { + if (in_valobj_sp) { + if ((m_valobj_sp = in_valobj_sp->GetQualifiedRepresentationIfAvailable( + lldb::eNoDynamicValues, false))) { + if (!m_name.IsEmpty()) + m_valobj_sp->SetName(m_name); + } + } +} + +ValueImpl &ValueImpl::operator=(const ValueImpl &rhs) { + if (this != &rhs) { + m_valobj_sp = rhs.m_valobj_sp; + m_use_dynamic = rhs.m_use_dynamic; + m_use_synthetic = rhs.m_use_synthetic; + m_name = rhs.m_name; + } + return *this; +} + +bool ValueImpl::IsValid() { + if (m_valobj_sp.get() == nullptr) + return false; + + // FIXME: This check is necessary but not sufficient. We for sure don't + // want to touch SBValues whose owning + // targets have gone away. This check is a little weak in that it + // enforces that restriction when you call IsValid, but since IsValid + // doesn't lock the target, you have no guarantee that the SBValue won't + // go invalid after you call this... Also, an SBValue could depend on + // data from one of the modules in the target, and those could go away + // independently of the target, for instance if a module is unloaded. + // But right now, neither SBValues nor ValueObjects know which modules + // they depend on. So I have no good way to make that check without + // tracking that in all the ValueObject subclasses. + TargetSP target_sp = m_valobj_sp->GetTargetSP(); + return target_sp && target_sp->IsValid(); +} + +lldb::ValueObjectSP +ValueImpl::GetSP(Process::StopLocker &stop_locker, + std::unique_lock<std::recursive_mutex> &lock, Status &error) { + if (!m_valobj_sp) { + error = Status::FromErrorString("invalid value object"); + return m_valobj_sp; + } + + 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) + return ValueObjectSP(); + + lock = std::unique_lock<std::recursive_mutex>(target->GetAPIMutex()); + + ProcessSP process_sp(value_sp->GetProcessSP()); + if (process_sp && !stop_locker.TryLock(&process_sp->GetRunLock())) { + // We don't allow people to play around with ValueObject if the process + // is running. If you want to look at values, pause the process, then + // look. + error = Status::FromErrorString("process must be stopped."); + return ValueObjectSP(); + } + + if (m_use_dynamic != eNoDynamicValues) { + ValueObjectSP dynamic_sp = value_sp->GetDynamicValue(m_use_dynamic); + if (dynamic_sp) + value_sp = dynamic_sp; + } + + if (m_use_synthetic) { + ValueObjectSP synthetic_sp = value_sp->GetSyntheticValue(); + if (synthetic_sp) + value_sp = synthetic_sp; + } + + if (!value_sp) + error = Status::FromErrorString("invalid value object"); + if (!m_name.IsEmpty()) + value_sp->SetName(m_name); + + return value_sp; +} _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
