[lldb] Move ValueImpl and ValueLocker to ValueObject, NFC.

[bzcheeseman]

Stacked PRs:

• [lldb] Make print delegate to synthetic frames. #178602
• [lldb] Add support for ScriptedFrame to provide values/variables. #178575
• [lldb] Add conversions for SBValueList and SBValue to the python bridge. #178574
• ->[lldb] Move ValueImpl and ValueLocker to ValueObject, NFC. #178573

---

[lldb] Move ValueImpl and ValueLocker to ValueObject, NFC.

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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Aman LaChapelle (bzcheeseman)

Changes

Stacked PRs:

• #178575
• #178574
• ->#178573

---

[lldb] Move ValueImpl and ValueLocker to ValueObject, NFC.

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/178573.diff

4 Files Affected:

• (modified) lldb/include/lldb/API/SBValue.h (+6-4)
• (modified) lldb/include/lldb/ValueObject/ValueObject.h (+90)
• (modified) lldb/source/API/SBValue.cpp (-166)
• (modified) lldb/source/ValueObject/ValueObject.cpp (+92)

diff --git a/lldb/include/lldb/API/SBValue.h b/lldb/include/lldb/API/SBValue.h
index dead11fba19fe..d4cc2f05c39e3 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
@@ -506,8 +505,11 @@ class LLDB_API SBValue {
   void SetSP(const lldb::ValueObjectSP &sp, lldb::DynamicValueType use_dynamic,
              bool use_synthetic, const char *name);
 
+protected:
+  friend class lldb_private::ScriptInterpreter;
+
 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..8a6e5e623905f 100644
--- a/lldb/include/lldb/ValueObject/ValueObject.h
+++ b/lldb/include/lldb/ValueObject/ValueObject.h
@@ -1121,6 +1121,96 @@ 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() {
+    if (m_valobj_sp)
+      return m_valobj_sp->GetTargetSP();
+    else
+      return lldb::TargetSP();
+  }
+
+  lldb::ProcessSP GetProcessSP() {
+    if (m_valobj_sp)
+      return m_valobj_sp->GetProcessSP();
+    else
+      return lldb::ProcessSP();
+  }
+
+  lldb::ThreadSP GetThreadSP() {
+    if (m_valobj_sp)
+      return m_valobj_sp->GetThreadSP();
+    else
+      return lldb::ThreadSP();
+  }
+
+  lldb::StackFrameSP GetFrameSP() {
+    if (m_valobj_sp)
+      return m_valobj_sp->GetFrameSP();
+    else
+      return 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..ee553b2b3925c 100644
--- a/lldb/source/ValueObject/ValueObject.cpp
+++ b/lldb/source/ValueObject/ValueObject.cpp
@@ -3768,3 +3768,95 @@ 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;
+  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
+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;
+}

[JDevlieghere]

As a reminder, the SB API provides ABI stability. Unless I'm forgetting about something, this transformation is ABI safe, because the type being moved is:

1. Not part of the lldb namespace,
2. Only used by private and protected methods of SBValue,
3. Doesn't change the layout/size of SBValue.

So that looks fine. Since you're already touching this, please consider removing some of the else-after-returns that are prevalent in the old code. Otherwise LGTM.

[medismailben]

LGTM :)

[medismailben]

/cherry-pick 58f623c

[llvmbot]

/pull-request #179500