[lldb] Protect OptionValue accesses from data races

[augusto2112]

Summary

Thread sanitizer is catching data races in OptionValue, protect accesses
to OptionValue with a mutex.

[bulbazord]

A few questions:

• Do all of these need to be protected with a mutex? In your description you're saying TSan is detecting data races. What piece of data are you observing the data race on?
• Do we need a recursive mutex? I assume that these operations might call into each other, but if not it would be nice to just have it be a std::mutex.

I think your implementation is already correct, these questions are more geared towards performance (i.e. not locking more than we need to, using a more efficient mutex type, etc.)

[augusto2112]

Do all of these need to be protected with a mutex? In your description you're saying TSan is detecting data races. What piece of data are you observing the data race on?

Only SetAsBoolean/GetAsBoolean are being caught when running our test suite at the moment.

Do we need a recursive mutex? I assume that these operations might call into each other, but if not it would be nice to just have it be a std::mutex.

If we want to protect all accesses then yes, since these functions call each other. It we decide to only lock against what's being caught by tsan then we can get by with a regular mutex.

[kastiglione]

lldb/source/Interpreter/OptionValue.cpp

40

this one doesn't need a lock

353–354

this one doesn't have a lock, should it?

[bulbazord]

In D157041#4559255, @augusto2112 wrote:

Do all of these need to be protected with a mutex? In your description you're saying TSan is detecting data races. What piece of data are you observing the data race on?

Only SetAsBoolean/GetAsBoolean are being caught when running our test suite at the moment.

Do we need a recursive mutex? I assume that these operations might call into each other, but if not it would be nice to just have it be a std::mutex.

If we want to protect all accesses then yes, since these functions call each other. It we decide to only lock against what's being caught by tsan then we can get by with a regular mutex.

Only SetAsBoolean and GetAsBoolean are being caught right now, I see. The implementation of those methods really isn't any different (AFAICT) from the other methods so I would assume we would need to protect all the other Get and Set methods as well then, makes sense. I'd probably keep the recursive mutex for now then.

[kastiglione]

lldb/source/Interpreter/OptionValue.cpp

46

do these GetAsX functions need to lock?

[augusto2112]

lldb/source/Interpreter/OptionValue.cpp

46

Hmm, I think we can get away with just locking GetXValue and SetXValue, and replace the lock with a non recursive one.

[aprantl]

lldb/source/Interpreter/OptionValue.cpp

52

If you are following @kastiglione 's suggestion from above (I'd be fine with choosing symmetry over performance) then this method (and the ones below) also doesn't need any locks, since it just calls other thread-safe methods.

[JDevlieghere]

Despite their prevalence in LLDB, I still consider recursive_mutex a potential code smell. So +1 on avoiding the need for a recursive mutex.

[augusto2112]

Addressed comments

[JDevlieghere]

LGTM. If it's straightforward, I think would be nice to have a unit test with two threads modifying the same OptionValue. That way a TSan run would catch this issue. If that's more work than expected then this is fine as is.

[augusto2112]

Remove lock from DeepCopy

[aprantl]

If it's straightforward, I think would be nice to have a unit test with two threads modifying the same OptionValue. That way a TSan run would catch this issue. If that's more work than expected then this is fine as is.

We might just want to set up a bot that runs the entire test suite under TSan, assuming that we can get it clean?

[JDevlieghere]

In D157041#4562033, @aprantl wrote:

If it's straightforward, I think would be nice to have a unit test with two threads modifying the same OptionValue. That way a TSan run would catch this issue. If that's more work than expected then this is fine as is.

We might just want to set up a bot that runs the entire test suite under TSan, assuming that we can get it clean?

Definitely, but last I checked there was still quite some work to be done :-)

lldb/include/lldb/Interpreter/OptionValue.h

 #include "lldb/Utility/FileSpecList.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StringList.h"
 #include "lldb/Utility/UUID.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-private-enumerations.h"
 #include "lldb/lldb-private-interfaces.h"
 #include "llvm/Support/JSON.h"
+#include <mutex>
 
 namespace lldb_private {
 
 // OptionValue
 class OptionValue {
 public:
   enum Type {
     eTypeInvalid = 0,
eDumpGroupHelp =
         (eDumpOptionName | eDumpOptionType | eDumpOptionDescription),
     eDumpGroupExport = (eDumpOptionCommand | eDumpOptionName | eDumpOptionValue)
   };
 
   OptionValue() = default;
 
   virtual ~OptionValue() = default;
 
+  OptionValue(const OptionValue &other);
+  
+  OptionValue& operator=(const OptionValue &other);
+
   // Subclasses should override these functions
   virtual Type GetType() const = 0;
 
   // If this value is always hidden, the avoid showing any info on this value,
   // just show the info for the child values.
   virtual bool ValueIsTransparent() const {
     return GetType() == eTypeProperties;
   }
private:
   std::optional<llvm::StringRef> GetStringValue() const;
   bool SetStringValue(llvm::StringRef new_value);
 
   std::optional<UUID> GetUUIDValue() const;
   bool SetUUIDValue(const UUID &uuid);
 
   const FormatEntity::Entry *GetFormatEntity() const;
   const RegularExpression *GetRegexValue() const;
+  
+  mutable std::mutex m_mutex;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_OPTIONVALUE_H

lldb/source/Interpreter/OptionValue.cpp

 #include "lldb/Interpreter/OptionValues.h"
 #include "lldb/Utility/StringList.h"
 
 #include <memory>
 
 using namespace lldb;
 using namespace lldb_private;
 
+OptionValue::OptionValue(const OptionValue &other) {
+  std::lock_guard<std::mutex> lock(other.m_mutex);
+
+  m_parent_wp = other.m_parent_wp;
+  m_callback = other.m_callback;
+  m_value_was_set = other.m_value_was_set;
+
+}
+
+OptionValue& OptionValue::operator=(const OptionValue &other) {
+  std::scoped_lock lock(m_mutex, other.m_mutex);
+
+  m_parent_wp = other.m_parent_wp;
+  m_callback = other.m_callback;
+  m_value_was_set = other.m_value_was_set;
+
+  return *this;
+}
+
 Status OptionValue::SetSubValue(const ExecutionContext *exe_ctx,
                                 VarSetOperationType op, llvm::StringRef name,
                                 llvm::StringRef value) {
   Status error;
   error.SetErrorString("SetSubValue is not supported");
   return error;
 }
 
 OptionValueBoolean *OptionValue::GetAsBoolean() {
   if (GetType() == OptionValue::eTypeBoolean)
     return static_cast<OptionValueBoolean *>(this);
   return nullptr;
 }
 
 const OptionValueBoolean *OptionValue::GetAsBoolean() const {
   if (GetType() == OptionValue::eTypeBoolean)
     return static_cast<const OptionValueBoolean *>(this);
   return nullptr;
 }
 
 const OptionValueChar *OptionValue::GetAsChar() const {
   if (GetType() == OptionValue::eTypeChar)
     return static_cast<const OptionValueChar *>(this);
   return nullptr;

 
 const OptionValueUUID *OptionValue::GetAsUUID() const {
   if (GetType() == OptionValue::eTypeUUID)
     return static_cast<const OptionValueUUID *>(this);
   return nullptr;
 }
 
 std::optional<bool> OptionValue::GetBooleanValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueBoolean *option_value = GetAsBoolean())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetBooleanValue(bool new_value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueBoolean *option_value = GetAsBoolean()) {
     option_value->SetCurrentValue(new_value);
     return true;
   }
   return false;
 }
 
 std::optional<char> OptionValue::GetCharValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueChar *option_value = GetAsChar())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetCharValue(char new_value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueChar *option_value = GetAsChar()) {
     option_value->SetCurrentValue(new_value);
     return true;
   }
   return false;
 }
 
 std::optional<int64_t> OptionValue::GetEnumerationValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueEnumeration *option_value = GetAsEnumeration())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetEnumerationValue(int64_t value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueEnumeration *option_value = GetAsEnumeration()) {
     option_value->SetCurrentValue(value);
     return true;
   }
   return false;
 }
 
 std::optional<FileSpec> OptionValue::GetFileSpecValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueFileSpec *option_value = GetAsFileSpec())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetFileSpecValue(FileSpec file_spec) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueFileSpec *option_value = GetAsFileSpec()) {
     option_value->SetCurrentValue(file_spec, false);
     return true;
   }
   return false;
 }
 
 bool OptionValue::AppendFileSpecValue(FileSpec file_spec) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueFileSpecList *option_value = GetAsFileSpecList()) {
     option_value->AppendCurrentValue(file_spec);
     return true;
   }
   return false;
 }
 
 std::optional<FileSpecList> OptionValue::GetFileSpecListValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueFileSpecList *option_value = GetAsFileSpecList())
     return option_value->GetCurrentValue();
   return {};
 }
 
 std::optional<lldb::Format> OptionValue::GetFormatValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueFormat *option_value = GetAsFormat())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetFormatValue(lldb::Format new_value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueFormat *option_value = GetAsFormat()) {
     option_value->SetCurrentValue(new_value);
     return true;
   }
   return false;
 }
 
 std::optional<lldb::LanguageType> OptionValue::GetLanguageValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueLanguage *option_value = GetAsLanguage())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetLanguageValue(lldb::LanguageType new_language) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueLanguage *option_value = GetAsLanguage()) {
     option_value->SetCurrentValue(new_language);
     return true;
   }
   return false;
 }
 
 const FormatEntity::Entry *OptionValue::GetFormatEntity() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueFormatEntity *option_value = GetAsFormatEntity())
     return &option_value->GetCurrentValue();
   return nullptr;
 }
 
 const RegularExpression *OptionValue::GetRegexValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueRegex *option_value = GetAsRegex())
     return option_value->GetCurrentValue();
   return nullptr;
 }
 
 std::optional<int64_t> OptionValue::GetSInt64Value() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueSInt64 *option_value = GetAsSInt64())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetSInt64Value(int64_t new_value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueSInt64 *option_value = GetAsSInt64()) {
     option_value->SetCurrentValue(new_value);
     return true;
   }
   return false;
 }
 
 std::optional<llvm::StringRef> OptionValue::GetStringValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueString *option_value = GetAsString())
     return option_value->GetCurrentValueAsRef();
   return {};
 }
 
 bool OptionValue::SetStringValue(llvm::StringRef new_value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueString *option_value = GetAsString()) {
     option_value->SetCurrentValue(new_value);
     return true;
   }
   return false;
 }
 
 std::optional<uint64_t> OptionValue::GetUInt64Value() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueUInt64 *option_value = GetAsUInt64())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetUInt64Value(uint64_t new_value) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueUInt64 *option_value = GetAsUInt64()) {
     option_value->SetCurrentValue(new_value);
     return true;
   }
   return false;
 }
 
 std::optional<UUID> OptionValue::GetUUIDValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueUUID *option_value = GetAsUUID())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetUUIDValue(const UUID &uuid) {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueUUID *option_value = GetAsUUID()) {
     option_value->SetCurrentValue(uuid);
     return true;
   }
   return false;
 }
 
 std::optional<ArchSpec> OptionValue::GetArchSpecValue() const {
+  std::lock_guard<std::mutex> lock(m_mutex);
   if (const OptionValueArch *option_value = GetAsArch())
     return option_value->GetCurrentValue();
   return {};
 }
 
 bool OptionValue::SetArchSpecValue(ArchSpec arch_spec) {
+    std::lock_guard<std::mutex> lock(m_mutex);
   if (OptionValueArch *option_value = GetAsArch()) {
     option_value->SetCurrentValue(arch_spec, false);
     return true;
   }
   return false;
 }
 
 const char *OptionValue::GetBuiltinTypeAsCString(Type t) {