[lldb/Interpreter] Fix deep copying for OptionValue classes

Some implementations of the DeepCopy function called the copy constructor that copied m_parent member instead of setting a new parent. Others just leaved the base class's members (m_parent, m_callback, m_was_set) empty.
One more problem is that not all classes override this function, e.g. OptionValueArgs::DeepCopy produces OptionValueArray instance, and Target[Process/Thread]ValueProperty::DeepCopy produces OptionValueProperty. This makes downcasting via static_cast invalid.

The patch implements idiom "virtual constructor" to fix these issues.
Add a test that checks DeepCopy for correct copying/setting all data members of the base class.

Differential Revision: https://reviews.llvm.org/D96952

lldb/include/lldb/Interpreter/OptionValue.h

@@ -10,6 +10,7 @@
 #define LLDB_INTERPRETER_OPTIONVALUE_H
 
 #include "lldb/Core/FormatEntity.h"
+#include "lldb/Utility/Cloneable.h"
 #include "lldb/Utility/CompletionRequest.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
@@ -87,7 +88,8 @@ public:
 
   virtual void Clear() = 0;
 
-  virtual lldb::OptionValueSP DeepCopy() const = 0;
+  virtual lldb::OptionValueSP
+  DeepCopy(const lldb::OptionValueSP &new_parent) const;
 
   virtual void AutoComplete(CommandInterpreter &interpreter,
                             CompletionRequest &request);
@@ -306,6 +308,8 @@ public:
     m_parent_wp = parent_sp;
   }
 
+  lldb::OptionValueSP GetParent() const { return m_parent_wp.lock(); }
+
   void SetValueChangedCallback(std::function<void()> callback) {
     assert(!m_callback);
     m_callback = std::move(callback);
@@ -317,6 +321,12 @@ public:
   }
 
 protected:
+  using TopmostBase = OptionValue;
+
+  // Must be overriden by a derived class for correct downcasting the result of
+  // DeepCopy to it. Inherit from Cloneable to avoid doing this manually.
+  virtual lldb::OptionValueSP Clone() const = 0;
+
   lldb::OptionValueWP m_parent_wp;
   std::function<void()> m_callback;
   bool m_value_was_set; // This can be used to see if a value has been set

lldb/include/lldb/Interpreter/OptionValueArch.h

@@ -15,7 +15,7 @@
 
 namespace lldb_private {
 
-class OptionValueArch : public OptionValue {
+class OptionValueArch : public Cloneable<OptionValueArch, OptionValue> {
 public:
   OptionValueArch() = default;
 
@@ -47,8 +47,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   void AutoComplete(CommandInterpreter &interpreter,
                     lldb_private::CompletionRequest &request) override;
 

lldb/include/lldb/Interpreter/OptionValueArgs.h

@@ -13,11 +13,10 @@
 
 namespace lldb_private {
 
-class OptionValueArgs : public OptionValueArray {
+class OptionValueArgs : public Cloneable<OptionValueArgs, OptionValueArray> {
 public:
   OptionValueArgs()
-      : OptionValueArray(
-            OptionValue::ConvertTypeToMask(OptionValue::eTypeString)) {}
+      : Cloneable(OptionValue::ConvertTypeToMask(OptionValue::eTypeString)) {}
 
   ~OptionValueArgs() override = default;
 

lldb/include/lldb/Interpreter/OptionValueArray.h

@@ -15,7 +15,7 @@
 
 namespace lldb_private {
 
-class OptionValueArray : public OptionValue {
+class OptionValueArray : public Cloneable<OptionValueArray, OptionValue> {
 public:
   OptionValueArray(uint32_t type_mask = UINT32_MAX, bool raw_value_dump = false)
       : m_type_mask(type_mask), m_values(), m_raw_value_dump(raw_value_dump) {}
@@ -38,7 +38,8 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
+  lldb::OptionValueSP
+  DeepCopy(const lldb::OptionValueSP &new_parent) const override;
 
   bool IsAggregateValue() const override { return true; }
 

lldb/include/lldb/Interpreter/OptionValueBoolean.h

@@ -13,7 +13,7 @@
 
 namespace lldb_private {
 
-class OptionValueBoolean : public OptionValue {
+class OptionValueBoolean : public Cloneable<OptionValueBoolean, OptionValue> {
 public:
   OptionValueBoolean(bool value)
       : m_current_value(value), m_default_value(value) {}
@@ -71,8 +71,6 @@ public:
 
   void SetDefaultValue(bool value) { m_default_value = value; }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
 protected:
   bool m_current_value;
   bool m_default_value;

lldb/include/lldb/Interpreter/OptionValueChar.h

@@ -13,7 +13,7 @@
 
 namespace lldb_private {
 
-class OptionValueChar : public OptionValue {
+class OptionValueChar : public Cloneable<OptionValueChar, OptionValue> {
 public:
   OptionValueChar(char value)
       : m_current_value(value), m_default_value(value) {}
@@ -54,8 +54,6 @@ public:
 
   void SetDefaultValue(char value) { m_default_value = value; }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
 protected:
   char m_current_value;
   char m_default_value;

lldb/include/lldb/Interpreter/OptionValueDictionary.h

@@ -15,7 +15,8 @@
 
 namespace lldb_private {
 
-class OptionValueDictionary : public OptionValue {
+class OptionValueDictionary
+    : public Cloneable<OptionValueDictionary, OptionValue> {
 public:
   OptionValueDictionary(uint32_t type_mask = UINT32_MAX,
                         bool raw_value_dump = true)
@@ -39,7 +40,8 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
+  lldb::OptionValueSP
+  DeepCopy(const lldb::OptionValueSP &new_parent) const override;
 
   bool IsAggregateValue() const override { return true; }
 

lldb/include/lldb/Interpreter/OptionValueEnumeration.h

@@ -19,7 +19,8 @@
 
 namespace lldb_private {
 
-class OptionValueEnumeration : public OptionValue {
+class OptionValueEnumeration
+    : public Cloneable<OptionValueEnumeration, OptionValue> {
 public:
   typedef int64_t enum_type;
   struct EnumeratorInfo {
@@ -49,8 +50,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   void AutoComplete(CommandInterpreter &interpreter,
                     CompletionRequest &request) override;
 

lldb/include/lldb/Interpreter/OptionValueFileColonLine.h

@@ -16,7 +16,8 @@
 
 namespace lldb_private {
 
-class OptionValueFileColonLine : public OptionValue {
+class OptionValueFileColonLine :
+    public Cloneable<OptionValueFileColonLine, OptionValue> {
 public:
   OptionValueFileColonLine();
   OptionValueFileColonLine(const llvm::StringRef input);
@@ -38,8 +39,6 @@ public:
     m_column_number = LLDB_INVALID_COLUMN_NUMBER;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   void AutoComplete(CommandInterpreter &interpreter,
                     CompletionRequest &request) override;
 

lldb/include/lldb/Interpreter/OptionValueFileSpec.h

@@ -16,7 +16,7 @@
 
 namespace lldb_private {
 
-class OptionValueFileSpec : public OptionValue {
+class OptionValueFileSpec : public Cloneable<OptionValueFileSpec, OptionValue> {
 public:
   OptionValueFileSpec(bool resolve = true);
 
@@ -45,8 +45,6 @@ public:
     m_data_mod_time = llvm::sys::TimePoint<>();
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   void AutoComplete(CommandInterpreter &interpreter,
                     CompletionRequest &request) override;
 

lldb/include/lldb/Interpreter/OptionValueFileSpecList.h

@@ -16,12 +16,13 @@
 
 namespace lldb_private {
 
-class OptionValueFileSpecList : public OptionValue {
+class OptionValueFileSpecList
+    : public Cloneable<OptionValueFileSpecList, OptionValue> {
 public:
   OptionValueFileSpecList() = default;
 
-  OptionValueFileSpecList(const FileSpecList &current_value)
-      : m_current_value(current_value) {}
+  OptionValueFileSpecList(const OptionValueFileSpecList &other)
+      : Cloneable(other), m_current_value(other.GetCurrentValue()) {}
 
   ~OptionValueFileSpecList() override = default;
 
@@ -42,8 +43,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   bool IsAggregateValue() const override { return true; }
 
   // Subclass specific functions
@@ -64,6 +63,8 @@ public:
   }
 
 protected:
+  lldb::OptionValueSP Clone() const override;
+
   mutable std::recursive_mutex m_mutex;
   FileSpecList m_current_value;
 };

lldb/include/lldb/Interpreter/OptionValueFormat.h

@@ -13,7 +13,8 @@
 
 namespace lldb_private {
 
-class OptionValueFormat : public OptionValue {
+class OptionValueFormat
+    : public Cloneable<OptionValueFormat, OptionValue> {
 public:
   OptionValueFormat(lldb::Format value)
       : m_current_value(value), m_default_value(value) {}
@@ -39,8 +40,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
 
   lldb::Format GetCurrentValue() const { return m_current_value; }

lldb/include/lldb/Interpreter/OptionValueFormatEntity.h

@@ -14,7 +14,8 @@
 
 namespace lldb_private {
 
-class OptionValueFormatEntity : public OptionValue {
+class OptionValueFormatEntity
+    : public Cloneable<OptionValueFormatEntity, OptionValue> {
 public:
   OptionValueFormatEntity(const char *default_format);
 
@@ -33,8 +34,6 @@ public:
 
   void Clear() override;
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   void AutoComplete(CommandInterpreter &interpreter,
                     CompletionRequest &request) override;
 

lldb/include/lldb/Interpreter/OptionValueLanguage.h

@@ -15,7 +15,7 @@
 
 namespace lldb_private {
 
-class OptionValueLanguage : public OptionValue {
+class OptionValueLanguage : public Cloneable<OptionValueLanguage, OptionValue> {
 public:
   OptionValueLanguage(lldb::LanguageType value)
       : m_current_value(value), m_default_value(value) {}
@@ -42,8 +42,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
 
   lldb::LanguageType GetCurrentValue() const { return m_current_value; }

lldb/include/lldb/Interpreter/OptionValuePathMappings.h

@@ -14,7 +14,8 @@
 
 namespace lldb_private {
 
-class OptionValuePathMappings : public OptionValue {
+class OptionValuePathMappings
+    : public Cloneable<OptionValuePathMappings, OptionValue> {
 public:
   OptionValuePathMappings(bool notify_changes)
       : m_notify_changes(notify_changes) {}
@@ -37,8 +38,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   bool IsAggregateValue() const override { return true; }
 
   // Subclass specific functions

lldb/include/lldb/Interpreter/OptionValueProperties.h

@@ -18,24 +18,27 @@
 #include "lldb/Utility/ConstString.h"
 
 namespace lldb_private {
+class Properties;
 
 class OptionValueProperties
-    : public OptionValue,
+    : public Cloneable<OptionValueProperties, OptionValue>,
       public std::enable_shared_from_this<OptionValueProperties> {
 public:
   OptionValueProperties() = default;
 
   OptionValueProperties(ConstString name);
 
-  OptionValueProperties(const OptionValueProperties &global_properties);
-
   ~OptionValueProperties() override = default;
 
   Type GetType() const override { return eTypeProperties; }
 
   void Clear() override;
 
-  lldb::OptionValueSP DeepCopy() const override;
+  static lldb::OptionValuePropertiesSP
+  CreateLocalCopy(const Properties &global_properties);
+
+  lldb::OptionValueSP
+  DeepCopy(const lldb::OptionValueSP &new_parent) const override;
 
   Status
   SetValueFromString(llvm::StringRef value,

lldb/include/lldb/Interpreter/OptionValueRegex.h

@@ -14,7 +14,7 @@
 
 namespace lldb_private {
 
-class OptionValueRegex : public OptionValue {
+class OptionValueRegex : public Cloneable<OptionValueRegex, OptionValue> {
 public:
   OptionValueRegex(const char *value = nullptr)
       : m_regex(llvm::StringRef::withNullAsEmpty(value)),
@@ -38,8 +38,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
   const RegularExpression *GetCurrentValue() const {
     return (m_regex.IsValid() ? &m_regex : nullptr);

lldb/include/lldb/Interpreter/OptionValueSInt64.h

@@ -14,7 +14,7 @@
 
 namespace lldb_private {
 
-class OptionValueSInt64 : public OptionValue {
+class OptionValueSInt64 : public Cloneable<OptionValueSInt64, OptionValue> {
 public:
   OptionValueSInt64() = default;
 
@@ -44,8 +44,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
 
   const int64_t &operator=(int64_t value) {

lldb/include/lldb/Interpreter/OptionValueString.h

@@ -17,7 +17,7 @@
 
 namespace lldb_private {
 
-class OptionValueString : public OptionValue {
+class OptionValueString : public Cloneable<OptionValueString, OptionValue> {
 public:
   typedef Status (*ValidatorCallback)(const char *string, void *baton);
 
@@ -78,8 +78,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
 
   Flags &GetOptions() { return m_options; }

lldb/include/lldb/Interpreter/OptionValueUInt64.h

@@ -14,7 +14,7 @@
 
 namespace lldb_private {
 
-class OptionValueUInt64 : public OptionValue {
+class OptionValueUInt64 : public Cloneable<OptionValueUInt64, OptionValue> {
 public:
   OptionValueUInt64() = default;
 
@@ -47,8 +47,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
 
   const uint64_t &operator=(uint64_t value) {

lldb/include/lldb/Interpreter/OptionValueUUID.h

@@ -14,7 +14,7 @@
 
 namespace lldb_private {
 
-class OptionValueUUID : public OptionValue {
+class OptionValueUUID : public Cloneable<OptionValueUUID, OptionValue> {
 public:
   OptionValueUUID() = default;
 
@@ -38,8 +38,6 @@ public:
     m_value_was_set = false;
   }
 
-  lldb::OptionValueSP DeepCopy() const override;
-
   // Subclass specific functions
 
   UUID &GetCurrentValue() { return m_uuid; }

lldb/include/lldb/Utility/Cloneable.h

@@ -0,0 +1,55 @@
+//===-- Cloneable.h ---------------------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_UTILITY_CLONEABLE_H
+#define LLDB_UTILITY_CLONEABLE_H
+
+#include <type_traits>
+
+namespace lldb_private {
+
+/// \class Cloneable Cloneable.h "lldb/Utility/Cloneable.h"
+/// A class that implements CRTP-based "virtual constructor" idiom.
+///
+/// Example:
+/// @code
+/// class Base {
+///   using TopmostBase = Base;
+/// public:
+///   virtual std::shared_ptr<Base> Clone() const = 0;
+/// };
+/// @endcode
+///
+/// To define a class derived from the Base with overridden Clone:
+/// @code
+/// class Intermediate : public Cloneable<Intermediate, Base> {};
+/// @endcode
+///
+/// To define a class at the next level of inheritance with overridden Clone:
+/// @code
+/// class Derived : public Cloneable<Derived, Intermediate> {};
+/// @endcode
+
+template <typename Derived, typename Base>
+class Cloneable : public Base {
+public:
+  using Base::Base;
+
+  std::shared_ptr<typename Base::TopmostBase> Clone() const override {
+    // std::is_base_of requires derived type to be complete, that's why class
+    // scope static_assert cannot be used.
+    static_assert(std::is_base_of<Cloneable, Derived>::value,
+                  "Derived class must be derived from this.");
+
+    return std::make_shared<Derived>(static_cast<const Derived &>(*this));
+  }
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_UTILITY_CLONEABLE_H

lldb/source/Interpreter/OptionValue.cpp

@@ -568,6 +568,12 @@ bool OptionValue::DumpQualifiedName(Stream &strm) const {
   return dumped_something;
 }
 
+OptionValueSP OptionValue::DeepCopy(const OptionValueSP &new_parent) const {
+  auto &&clone = Clone();
+  clone->SetParent(new_parent);
+  return clone;
+}
+
 void OptionValue::AutoComplete(CommandInterpreter &interpreter,
                                CompletionRequest &request) {}
 

lldb/source/Interpreter/OptionValueArch.cpp

@@ -64,10 +64,6 @@ Status OptionValueArch::SetValueFromString(llvm::StringRef value,
   return error;
 }
 
-lldb::OptionValueSP OptionValueArch::DeepCopy() const {
-  return OptionValueSP(new OptionValueArch(*this));
-}
-
 void OptionValueArch::AutoComplete(CommandInterpreter &interpreter,
                                    CompletionRequest &request) {
   CommandCompletions::InvokeCommonCompletionCallbacks(

lldb/source/Interpreter/OptionValueArray.cpp

@@ -303,15 +303,16 @@ Status OptionValueArray::SetArgs(const Args &args, VarSetOperationType op) {
   return error;
 }
 
-lldb::OptionValueSP OptionValueArray::DeepCopy() const {
-  OptionValueArray *copied_array =
-      new OptionValueArray(m_type_mask, m_raw_value_dump);
-  lldb::OptionValueSP copied_value_sp(copied_array);
-  *static_cast<OptionValue *>(copied_array) = *this;
-  copied_array->m_callback = m_callback;
-  const uint32_t size = m_values.size();
-  for (uint32_t i = 0; i < size; ++i) {
-    copied_array->AppendValue(m_values[i]->DeepCopy());
-  }
-  return copied_value_sp;
+OptionValueSP
+OptionValueArray::DeepCopy(const OptionValueSP &new_parent) const {
+  auto copy_sp = OptionValue::DeepCopy(new_parent);
+  // copy_sp->GetAsArray cannot be used here as it doesn't work for derived
+  // types that override GetType returning a different value.
+  auto *array_value_ptr = static_cast<OptionValueArray *>(copy_sp.get());
+  lldbassert(array_value_ptr);
+
+  for (auto &value : array_value_ptr->m_values)
+    value = value->DeepCopy(copy_sp);
+
+  return copy_sp;
 }

lldb/source/Interpreter/OptionValueBoolean.cpp

@@ -67,10 +67,6 @@ Status OptionValueBoolean::SetValueFromString(llvm::StringRef value_str,
   return error;
 }
 
-lldb::OptionValueSP OptionValueBoolean::DeepCopy() const {
-  return OptionValueSP(new OptionValueBoolean(*this));
-}
-
 void OptionValueBoolean::AutoComplete(CommandInterpreter &interpreter,
                                       CompletionRequest &request) {
   llvm::StringRef autocomplete_entries[] = {"true", "false", "on", "off",

lldb/source/Interpreter/OptionValueChar.cpp

@@ -57,7 +57,3 @@ Status OptionValueChar::SetValueFromString(llvm::StringRef value,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValueChar::DeepCopy() const {
-  return OptionValueSP(new OptionValueChar(*this));
-}

lldb/source/Interpreter/OptionValueDictionary.cpp

@@ -311,15 +311,16 @@ bool OptionValueDictionary::DeleteValueForKey(ConstString key) {
   return false;
 }
 
-lldb::OptionValueSP OptionValueDictionary::DeepCopy() const {
-  OptionValueDictionary *copied_dict =
-      new OptionValueDictionary(m_type_mask, m_raw_value_dump);
-  lldb::OptionValueSP copied_value_sp(copied_dict);
-  collection::const_iterator pos, end = m_values.end();
-  for (pos = m_values.begin(); pos != end; ++pos) {
-    StreamString strm;
-    strm.Printf("%s=", pos->first.GetCString());
-    copied_dict->SetValueForKey(pos->first, pos->second->DeepCopy(), true);
-  }
-  return copied_value_sp;
+OptionValueSP
+OptionValueDictionary::DeepCopy(const OptionValueSP &new_parent) const {
+  auto copy_sp = OptionValue::DeepCopy(new_parent);
+  // copy_sp->GetAsDictionary cannot be used here as it doesn't work for derived
+  // types that override GetType returning a different value.
+  auto *dict_value_ptr = static_cast<OptionValueDictionary *>(copy_sp.get());
+  lldbassert(dict_value_ptr);
+
+  for (auto &value : dict_value_ptr->m_values)
+    value.second = value.second->DeepCopy(copy_sp);
+
+  return copy_sp;
 }

lldb/source/Interpreter/OptionValueEnumeration.cpp

@@ -95,10 +95,6 @@ void OptionValueEnumeration::SetEnumerations(
   m_enumerations.Sort();
 }
 
-lldb::OptionValueSP OptionValueEnumeration::DeepCopy() const {
-  return OptionValueSP(new OptionValueEnumeration(*this));
-}
-
 void OptionValueEnumeration::AutoComplete(CommandInterpreter &interpreter,
                                           CompletionRequest &request) {
   const uint32_t num_enumerators = m_enumerations.GetSize();

lldb/source/Interpreter/OptionValueFileColonLine.cpp

@@ -134,10 +134,6 @@ Status OptionValueFileColonLine::SetValueFromString(llvm::StringRef value,
   return error;
 }
 
-lldb::OptionValueSP OptionValueFileColonLine::DeepCopy() const {
-  return OptionValueSP(new OptionValueFileColonLine(*this));
-}
-
 void OptionValueFileColonLine::AutoComplete(CommandInterpreter &interpreter,
                                             CompletionRequest &request) {
   CommandCompletions::InvokeCommonCompletionCallbacks(

lldb/source/Interpreter/OptionValueFileSpec.cpp

@@ -84,10 +84,6 @@ Status OptionValueFileSpec::SetValueFromString(llvm::StringRef value,
   return error;
 }
 
-lldb::OptionValueSP OptionValueFileSpec::DeepCopy() const {
-  return OptionValueSP(new OptionValueFileSpec(*this));
-}
-
 void OptionValueFileSpec::AutoComplete(CommandInterpreter &interpreter,
                                        CompletionRequest &request) {
   CommandCompletions::InvokeCommonCompletionCallbacks(

lldb/source/Interpreter/OptionValueFileSpecList.cpp

@@ -164,7 +164,7 @@ Status OptionValueFileSpecList::SetValueFromString(llvm::StringRef value,
   return error;
 }
 
-lldb::OptionValueSP OptionValueFileSpecList::DeepCopy() const {
+OptionValueSP OptionValueFileSpecList::Clone() const {
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
-  return OptionValueSP(new OptionValueFileSpecList(m_current_value));
+  return Cloneable::Clone();
 }

lldb/source/Interpreter/OptionValueFormat.cpp

@@ -56,7 +56,3 @@ Status OptionValueFormat::SetValueFromString(llvm::StringRef value,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValueFormat::DeepCopy() const {
-  return OptionValueSP(new OptionValueFormat(*this));
-}

lldb/source/Interpreter/OptionValueFormatEntity.cpp

@@ -109,10 +109,6 @@ Status OptionValueFormatEntity::SetValueFromString(llvm::StringRef value_str,
   return error;
 }
 
-lldb::OptionValueSP OptionValueFormatEntity::DeepCopy() const {
-  return OptionValueSP(new OptionValueFormatEntity(*this));
-}
-
 void OptionValueFormatEntity::AutoComplete(CommandInterpreter &interpreter,
                                            CompletionRequest &request) {
   FormatEntity::AutoComplete(request);

lldb/source/Interpreter/OptionValueLanguage.cpp

@@ -69,7 +69,3 @@ Status OptionValueLanguage::SetValueFromString(llvm::StringRef value,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValueLanguage::DeepCopy() const {
-  return OptionValueSP(new OptionValueLanguage(*this));
-}

lldb/source/Interpreter/OptionValuePathMappings.cpp

@@ -196,7 +196,3 @@ Status OptionValuePathMappings::SetValueFromString(llvm::StringRef value,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValuePathMappings::DeepCopy() const {
-  return OptionValueSP(new OptionValuePathMappings(*this));
-}

lldb/source/Interpreter/OptionValueProperties.cpp

@@ -22,26 +22,6 @@ using namespace lldb_private;
 
 OptionValueProperties::OptionValueProperties(ConstString name) : m_name(name) {}
 
-OptionValueProperties::OptionValueProperties(
-    const OptionValueProperties &global_properties)
-    : OptionValue(global_properties),
-      m_name(global_properties.m_name),
-      m_properties(global_properties.m_properties),
-      m_name_to_index(global_properties.m_name_to_index) {
-  // We now have an exact copy of "global_properties". We need to now find all
-  // non-global settings and copy the property values so that all non-global
-  // settings get new OptionValue instances created for them.
-  const size_t num_properties = m_properties.size();
-  for (size_t i = 0; i < num_properties; ++i) {
-    // Duplicate any values that are not global when constructing properties
-    // from a global copy
-    if (!m_properties[i].IsGlobal()) {
-      lldb::OptionValueSP new_value_sp(m_properties[i].GetValue()->DeepCopy());
-      m_properties[i].SetOptionValue(new_value_sp);
-    }
-  }
-}
-
 size_t OptionValueProperties::GetNumProperties() const {
   return m_properties.size();
 }
@@ -562,8 +542,32 @@ Status OptionValueProperties::DumpPropertyValue(const ExecutionContext *exe_ctx,
   return error;
 }
 
-lldb::OptionValueSP OptionValueProperties::DeepCopy() const {
-  llvm_unreachable("this shouldn't happen");
+OptionValuePropertiesSP
+OptionValueProperties::CreateLocalCopy(const Properties &global_properties) {
+  auto global_props_sp = global_properties.GetValueProperties();
+  lldbassert(global_props_sp);
+
+  auto copy_sp = global_props_sp->DeepCopy(global_props_sp->GetParent());
+  return std::static_pointer_cast<OptionValueProperties>(copy_sp);
+}
+
+OptionValueSP
+OptionValueProperties::DeepCopy(const OptionValueSP &new_parent) const {
+  auto copy_sp = OptionValue::DeepCopy(new_parent);
+  // copy_sp->GetAsProperties cannot be used here as it doesn't work for derived
+  // types that override GetType returning a different value.
+  auto *props_value_ptr = static_cast<OptionValueProperties *>(copy_sp.get());
+  lldbassert(props_value_ptr);
+
+  for (auto &property : props_value_ptr->m_properties) {
+    // Duplicate any values that are not global when constructing properties
+    // from a global copy.
+    if (!property.IsGlobal()) {
+      auto value_sp = property.GetValue()->DeepCopy(copy_sp);
+      property.SetOptionValue(value_sp);
+    }
+  }
+  return copy_sp;
 }
 
 const Property *OptionValueProperties::GetPropertyAtPath(

lldb/source/Interpreter/OptionValueRegex.cpp

@@ -59,7 +59,3 @@ Status OptionValueRegex::SetValueFromString(llvm::StringRef value,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValueRegex::DeepCopy() const {
-  return OptionValueSP(new OptionValueRegex(m_regex.GetText().str().c_str()));
-}

lldb/source/Interpreter/OptionValueSInt64.cpp

@@ -70,7 +70,3 @@ Status OptionValueSInt64::SetValueFromString(llvm::StringRef value_ref,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValueSInt64::DeepCopy() const {
-  return OptionValueSP(new OptionValueSInt64(*this));
-}

lldb/source/Interpreter/OptionValueString.cpp

@@ -117,10 +117,6 @@ Status OptionValueString::SetValueFromString(llvm::StringRef value,
   return error;
 }
 
-lldb::OptionValueSP OptionValueString::DeepCopy() const {
-  return OptionValueSP(new OptionValueString(*this));
-}
-
 Status OptionValueString::SetCurrentValue(llvm::StringRef value) {
   if (m_validator) {
     Status error(m_validator(value.str().c_str(), m_validator_baton));

lldb/source/Interpreter/OptionValueUInt64.cpp

@@ -68,7 +68,3 @@ Status OptionValueUInt64::SetValueFromString(llvm::StringRef value_ref,
   }
   return error;
 }
-
-lldb::OptionValueSP OptionValueUInt64::DeepCopy() const {
-  return OptionValueSP(new OptionValueUInt64(*this));
-}

lldb/source/Interpreter/OptionValueUUID.cpp

@@ -58,10 +58,6 @@ Status OptionValueUUID::SetValueFromString(llvm::StringRef value,
   return error;
 }
 
-lldb::OptionValueSP OptionValueUUID::DeepCopy() const {
-  return OptionValueSP(new OptionValueUUID(*this));
-}
-
 void OptionValueUUID::AutoComplete(CommandInterpreter &interpreter,
                                    CompletionRequest &request) {
   ExecutionContext exe_ctx(interpreter.GetExecutionContext());

lldb/source/Target/Process.cpp

@@ -83,16 +83,10 @@ using namespace std::chrono;
 #define DISABLE_MEM_CACHE_DEFAULT true
 #endif
 
-class ProcessOptionValueProperties : public OptionValueProperties {
+class ProcessOptionValueProperties
+    : public Cloneable<ProcessOptionValueProperties, OptionValueProperties> {
 public:
-  ProcessOptionValueProperties(ConstString name)
-      : OptionValueProperties(name) {}
-
-  // This constructor is used when creating ProcessOptionValueProperties when
-  // it is part of a new lldb_private::Process instance. It will copy all
-  // current global property values as needed
-  ProcessOptionValueProperties(ProcessProperties *global_properties)
-      : OptionValueProperties(*global_properties->GetValueProperties()) {}
+  ProcessOptionValueProperties(ConstString name) : Cloneable(name) {}
 
   const Property *GetPropertyAtIndex(const ExecutionContext *exe_ctx,
                                      bool will_modify,
@@ -131,10 +125,12 @@ enum {
 #include "TargetPropertiesEnum.inc"
 };
 
-class ProcessExperimentalOptionValueProperties : public OptionValueProperties {
+class ProcessExperimentalOptionValueProperties
+    : public Cloneable<ProcessExperimentalOptionValueProperties,
+                       OptionValueProperties> {
 public:
   ProcessExperimentalOptionValueProperties()
-      : OptionValueProperties(
+      : Cloneable(
             ConstString(Properties::GetExperimentalSettingsName())) {}
 };
 
@@ -157,8 +153,8 @@ ProcessProperties::ProcessProperties(lldb_private::Process *process)
         ConstString("thread"), ConstString("Settings specific to threads."),
         true, Thread::GetGlobalProperties()->GetValueProperties());
   } else {
-    m_collection_sp = std::make_shared<ProcessOptionValueProperties>(
-        Process::GetGlobalProperties().get());
+    m_collection_sp =
+        OptionValueProperties::CreateLocalCopy(*Process::GetGlobalProperties());
     m_collection_sp->SetValueChangedCallback(
         ePropertyPythonOSPluginPath,
         [this] { m_process->LoadOperatingSystemPlugin(true); });

lldb/source/Target/Target.cpp

@@ -3615,15 +3615,10 @@ enum {
   ePropertyExperimental,
 };
 
-class TargetOptionValueProperties : public OptionValueProperties {
+class TargetOptionValueProperties
+    : public Cloneable<TargetOptionValueProperties, OptionValueProperties> {
 public:
-  TargetOptionValueProperties(ConstString name) : OptionValueProperties(name) {}
-
-  // This constructor is used when creating TargetOptionValueProperties when it
-  // is part of a new lldb_private::Target instance. It will copy all current
-  // global property values as needed
-  TargetOptionValueProperties(const TargetPropertiesSP &target_properties_sp)
-      : OptionValueProperties(*target_properties_sp->GetValueProperties()) {}
+  TargetOptionValueProperties(ConstString name) : Cloneable(name) {}
 
   const Property *GetPropertyAtIndex(const ExecutionContext *exe_ctx,
                                      bool will_modify,
@@ -3654,11 +3649,12 @@ enum {
 #include "TargetPropertiesEnum.inc"
 };
 
-class TargetExperimentalOptionValueProperties : public OptionValueProperties {
+class TargetExperimentalOptionValueProperties
+    : public Cloneable<TargetExperimentalOptionValueProperties,
+                       OptionValueProperties> {
 public:
   TargetExperimentalOptionValueProperties()
-      : OptionValueProperties(
-            ConstString(Properties::GetExperimentalSettingsName())) {}
+      : Cloneable(ConstString(Properties::GetExperimentalSettingsName())) {}
 };
 
 TargetExperimentalProperties::TargetExperimentalProperties()
@@ -3671,8 +3667,8 @@ TargetExperimentalProperties::TargetExperimentalProperties()
 TargetProperties::TargetProperties(Target *target)
     : Properties(), m_launch_info(), m_target(target) {
   if (target) {
-    m_collection_sp = std::make_shared<TargetOptionValueProperties>(
-        Target::GetGlobalProperties());
+    m_collection_sp =
+        OptionValueProperties::CreateLocalCopy(*Target::GetGlobalProperties());
 
     // Set callbacks to update launch_info whenever "settins set" updated any
     // of these properties

lldb/source/Target/Thread.cpp

@@ -71,16 +71,10 @@ enum {
 #include "TargetPropertiesEnum.inc"
 };
 
-class ThreadOptionValueProperties : public OptionValueProperties {
+class ThreadOptionValueProperties
+    : public Cloneable<ThreadOptionValueProperties, OptionValueProperties> {
 public:
-  ThreadOptionValueProperties(ConstString name)
-      : OptionValueProperties(name) {}
-
-  // This constructor is used when creating ThreadOptionValueProperties when it
-  // is part of a new lldb_private::Thread instance. It will copy all current
-  // global property values as needed
-  ThreadOptionValueProperties(ThreadProperties *global_properties)
-      : OptionValueProperties(*global_properties->GetValueProperties()) {}
+  ThreadOptionValueProperties(ConstString name) : Cloneable(name) {}
 
   const Property *GetPropertyAtIndex(const ExecutionContext *exe_ctx,
                                      bool will_modify,
@@ -108,8 +102,8 @@ ThreadProperties::ThreadProperties(bool is_global) : Properties() {
         std::make_shared<ThreadOptionValueProperties>(ConstString("thread"));
     m_collection_sp->Initialize(g_thread_properties);
   } else
-    m_collection_sp = std::make_shared<ThreadOptionValueProperties>(
-        Thread::GetGlobalProperties().get());
+    m_collection_sp =
+        OptionValueProperties::CreateLocalCopy(*Thread::GetGlobalProperties());
 }
 
 ThreadProperties::~ThreadProperties() = default;

lldb/test/API/commands/settings/TestSettings.py

@@ -238,6 +238,11 @@ class SettingsCommandTestCase(TestBase):
         self.assertTrue(found_env_var,
                         "MY_ENV_VAR was not set in LunchInfo object")
 
+        self.assertEqual(launch_info.GetNumArguments(), 3)
+        self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")
+        self.assertEqual(launch_info.GetArgumentAtIndex(1), "B")
+        self.assertEqual(launch_info.GetArgumentAtIndex(2), "C")
+        
         self.expect(
             'target show-launch-environment',
             substrs=["MY_ENV_VAR=YES"])

lldb/unittests/Interpreter/CMakeLists.txt

@@ -1,6 +1,7 @@
 add_lldb_unittest(InterpreterTests
   TestCompletion.cpp
   TestOptionArgParser.cpp
+  TestOptionValue.cpp
   TestOptionValueFileColonLine.cpp
 
   LINK_LIBS

lldb/unittests/Interpreter/TestOptionValue.cpp

@@ -0,0 +1,173 @@
+//===-- TestOptionValue.cpp --------        -------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Interpreter/OptionValues.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+
+class Callback {
+public:
+  virtual void Invoke() const {}
+  void operator()() const { Invoke(); }
+};
+
+class MockCallback : public Callback {
+public:
+  MOCK_CONST_METHOD0(Invoke, void());
+};
+
+// Test a single-value class.
+TEST(OptionValueString, DeepCopy) {
+  OptionValueString str;
+  str.SetValueFromString("ab");
+
+  MockCallback callback;
+  str.SetValueChangedCallback([&callback] { callback(); });
+  EXPECT_CALL(callback, Invoke());
+
+  auto copy_sp = str.DeepCopy(nullptr);
+
+  // Test that the base class data members are copied/set correctly.
+  ASSERT_TRUE(copy_sp);
+  ASSERT_EQ(copy_sp->GetParent().get(), nullptr);
+  ASSERT_TRUE(copy_sp->OptionWasSet());
+  ASSERT_EQ(copy_sp->GetStringValue(), "ab");
+
+  // Trigger the callback.
+  copy_sp->SetValueFromString("c", eVarSetOperationAppend);
+  ASSERT_EQ(copy_sp->GetStringValue(), "abc");
+}
+
+// Test an aggregate class.
+TEST(OptionValueArgs, DeepCopy) {
+  OptionValueArgs args;
+  args.SetValueFromString("A B");
+
+  MockCallback callback;
+  args.SetValueChangedCallback([&callback] { callback(); });
+  EXPECT_CALL(callback, Invoke());
+
+  auto copy_sp = args.DeepCopy(nullptr);
+
+  // Test that the base class data members are copied/set correctly.
+  ASSERT_TRUE(copy_sp);
+  ASSERT_EQ(copy_sp->GetParent(), nullptr);
+  ASSERT_TRUE(copy_sp->OptionWasSet());
+
+  auto *args_copy_ptr = copy_sp->GetAsArgs();
+  ASSERT_EQ(args_copy_ptr->GetSize(), 2U);
+  ASSERT_EQ((*args_copy_ptr)[0]->GetParent(), copy_sp);
+  ASSERT_EQ((*args_copy_ptr)[0]->GetStringValue(), "A");
+  ASSERT_EQ((*args_copy_ptr)[1]->GetParent(), copy_sp);
+  ASSERT_EQ((*args_copy_ptr)[1]->GetStringValue(), "B");
+
+  // Trigger the callback.
+  copy_sp->SetValueFromString("C", eVarSetOperationAppend);
+  ASSERT_TRUE(args_copy_ptr);
+  ASSERT_EQ(args_copy_ptr->GetSize(), 3U);
+  ASSERT_EQ((*args_copy_ptr)[2]->GetStringValue(), "C");
+}
+
+class TestProperties : public OptionValueProperties {
+public:
+  static std::shared_ptr<TestProperties> CreateGlobal() {
+    auto props_sp = std::make_shared<TestProperties>();
+    const bool is_global = false;
+
+    auto dict_sp = std::make_shared<OptionValueDictionary>(1 << eTypeUInt64);
+    props_sp->AppendProperty(ConstString("dict"), ConstString(), is_global,
+                             dict_sp);
+
+    auto file_list_sp = std::make_shared<OptionValueFileSpecList>();
+    props_sp->AppendProperty(ConstString("file-list"), ConstString(), is_global,
+                             file_list_sp);
+    return props_sp;
+  }
+
+  void SetDictionaryChangedCallback(const MockCallback &callback) {
+    SetValueChangedCallback(m_dict_index, [&callback] { callback(); });
+  }
+
+  void SetFileListChangedCallback(const MockCallback &callback) {
+    SetValueChangedCallback(m_file_list_index, [&callback] { callback(); });
+  }
+
+  OptionValueDictionary *GetDictionary() {
+    return GetPropertyAtIndexAsOptionValueDictionary(nullptr, m_dict_index);
+  }
+
+  OptionValueFileSpecList *GetFileList() {
+    return GetPropertyAtIndexAsOptionValueFileSpecList(nullptr, true,
+                                                       m_file_list_index);
+  }
+
+private:
+  lldb::OptionValueSP Clone() const {
+    return std::make_shared<TestProperties>(*this);
+  }
+
+  uint32_t m_dict_index = 0;
+  uint32_t m_file_list_index = 1;
+};
+
+// Test a user-defined propery class.
+TEST(TestProperties, DeepCopy) {
+  auto props_sp = TestProperties::CreateGlobal();
+  props_sp->GetDictionary()->SetValueFromString("A=1 B=2");
+  props_sp->GetFileList()->SetValueFromString("path/to/file");
+
+  MockCallback callback;
+  props_sp->SetDictionaryChangedCallback(callback);
+  props_sp->SetFileListChangedCallback(callback);
+  EXPECT_CALL(callback, Invoke()).Times(2);
+
+  auto copy_sp = props_sp->DeepCopy(nullptr);
+
+  // Test that the base class data members are copied/set correctly.
+  ASSERT_TRUE(copy_sp);
+  ASSERT_EQ(copy_sp->GetParent(), nullptr);
+
+  // This cast is safe only if the class overrides Clone().
+  auto *props_copy_ptr = static_cast<TestProperties *>(copy_sp.get());
+  ASSERT_TRUE(props_copy_ptr);
+
+  // Test the first child.
+  auto dict_copy_ptr = props_copy_ptr->GetDictionary();
+  ASSERT_TRUE(dict_copy_ptr);
+  ASSERT_EQ(dict_copy_ptr->GetParent(), copy_sp);
+  ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
+  ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
+
+  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  ASSERT_TRUE(value_ptr);
+  ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
+  ASSERT_EQ(value_ptr->GetUInt64Value(), 1U);
+
+  value_ptr = dict_copy_ptr->GetValueForKey(ConstString("B"));
+  ASSERT_TRUE(value_ptr);
+  ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
+  ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);
+
+  // Test the second child.
+  auto file_list_copy_ptr = props_copy_ptr->GetFileList();
+  ASSERT_TRUE(file_list_copy_ptr);
+  ASSERT_EQ(file_list_copy_ptr->GetParent(), copy_sp);
+  ASSERT_TRUE(file_list_copy_ptr->OptionWasSet());
+
+  auto file_list_copy = file_list_copy_ptr->GetCurrentValue();
+  ASSERT_EQ(file_list_copy.GetSize(), 1U);
+  ASSERT_EQ(file_list_copy.GetFileSpecAtIndex(0), FileSpec("path/to/file"));
+
+  // Trigger the callback first time.
+  dict_copy_ptr->SetValueFromString("C=3", eVarSetOperationAppend);
+
+  // Trigger the callback second time.
+  file_list_copy_ptr->SetValueFromString("0 another/path", eVarSetOperationReplace);
+}