[lldb][NFC] Remove ThreadSafeSTLVector and ThreadSafeSTLMap and their use in ValueObjectSynthetic

[teemperor]

Summary

ThreadSafeSTLVector and ThreadSafeSTLMap are not useful for achieving any degree of thread safety in LLDB
and should be removed before they are used in more places. They are only used (unsurprisingly incorrectly) in
ValueObjectSynthetic::GetChildAtIndex, so this patch replaces their use there with a simple mutex with which
we guard the related data structures. This doesn't make ValueObjectSynthetic::GetChildAtIndex
any more thread-safe, but on the other hand it at least allows us to get rid of the ThreadSafeSTL* data structures
without changing the observable behaviour of ValueObjectSynthetic (beside that it is now a few bytes smaller).

[labath]

I'm yet to see a non-trivial algorithm, which can be made thread-safe by employing "thread safe" containers. +1 for deleting these and sorting the ValueObjectSynthetic issues later.

lldb/include/lldb/Core/ThreadSafeSTLMap.h

-//===-- ThreadSafeSTLMap.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 liblldb_ThreadSafeSTLMap_h_
-#define liblldb_ThreadSafeSTLMap_h_
-
-#include <map>
-#include <mutex>
-
-#include "lldb/lldb-defines.h"
-
-namespace lldb_private {
-
-template <typename _Key, typename _Tp> class ThreadSafeSTLMap {
-public:
-  typedef std::map<_Key, _Tp> collection;
-  typedef typename collection::iterator iterator;
-  typedef typename collection::const_iterator const_iterator;
-  // Constructors and Destructors
-  ThreadSafeSTLMap() : m_collection(), m_mutex() {}
-
-  ~ThreadSafeSTLMap() {}
-
-  bool IsEmpty() const {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return m_collection.empty();
-  }
-
-  void Clear() {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return m_collection.clear();
-  }
-
-  size_t Erase(const _Key &key) {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return EraseNoLock(key);
-  }
-
-  size_t EraseNoLock(const _Key &key) { return m_collection.erase(key); }
-
-  bool GetValueForKey(const _Key &key, _Tp &value) const {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return GetValueForKeyNoLock(key, value);
-  }
-
-  // Call this if you have already manually locked the mutex using the
-  // GetMutex() accessor
-  bool GetValueForKeyNoLock(const _Key &key, _Tp &value) const {
-    const_iterator pos = m_collection.find(key);
-    if (pos != m_collection.end()) {
-      value = pos->second;
-      return true;
-    }
-    return false;
-  }
-
-  bool GetFirstKeyForValue(const _Tp &value, _Key &key) const {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return GetFirstKeyForValueNoLock(value, key);
-  }
-
-  bool GetFirstKeyForValueNoLock(const _Tp &value, _Key &key) const {
-    const_iterator pos, end = m_collection.end();
-    for (pos = m_collection.begin(); pos != end; ++pos) {
-      if (pos->second == value) {
-        key = pos->first;
-        return true;
-      }
-    }
-    return false;
-  }
-
-  bool LowerBound(const _Key &key, _Key &match_key, _Tp &match_value,
-                  bool decrement_if_not_equal) const {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return LowerBoundNoLock(key, match_key, match_value,
-                            decrement_if_not_equal);
-  }
-
-  bool LowerBoundNoLock(const _Key &key, _Key &match_key, _Tp &match_value,
-                        bool decrement_if_not_equal) const {
-    const_iterator pos = m_collection.lower_bound(key);
-    if (pos != m_collection.end()) {
-      match_key = pos->first;
-      if (decrement_if_not_equal && key != match_key &&
-          pos != m_collection.begin()) {
-        --pos;
-        match_key = pos->first;
-      }
-      match_value = pos->second;
-      return true;
-    }
-    return false;
-  }
-
-  iterator lower_bound_unsafe(const _Key &key) {
-    return m_collection.lower_bound(key);
-  }
-
-  void SetValueForKey(const _Key &key, const _Tp &value) {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    SetValueForKeyNoLock(key, value);
-  }
-
-  // Call this if you have already manually locked the mutex using the
-  // GetMutex() accessor
-  void SetValueForKeyNoLock(const _Key &key, const _Tp &value) {
-    m_collection[key] = value;
-  }
-
-  std::recursive_mutex &GetMutex() { return m_mutex; }
-
-private:
-  collection m_collection;
-  mutable std::recursive_mutex m_mutex;
-
-  // For ThreadSafeSTLMap only
-  DISALLOW_COPY_AND_ASSIGN(ThreadSafeSTLMap);
-};
-
-} // namespace lldb_private
-
-#endif // liblldb_ThreadSafeSTLMap_h_

lldb/include/lldb/Core/ThreadSafeSTLVector.h

-//===-- ThreadSafeSTLVector.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 liblldb_ThreadSafeSTLVector_h_
-#define liblldb_ThreadSafeSTLVector_h_
-
-#include <mutex>
-#include <vector>
-
-#include "lldb/lldb-defines.h"
-
-namespace lldb_private {
-
-template <typename _Object> class ThreadSafeSTLVector {
-public:
-  typedef std::vector<_Object> collection;
-  typedef typename collection::iterator iterator;
-  typedef typename collection::const_iterator const_iterator;
-  // Constructors and Destructors
-  ThreadSafeSTLVector() : m_collection(), m_mutex() {}
-
-  ~ThreadSafeSTLVector() = default;
-
-  bool IsEmpty() const {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return m_collection.empty();
-  }
-
-  void Clear() {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return m_collection.clear();
-  }
-
-  size_t GetCount() {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return m_collection.size();
-  }
-
-  void AppendObject(_Object &object) {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    m_collection.push_back(object);
-  }
-
-  _Object GetObject(size_t index) {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    return m_collection.at(index);
-  }
-
-  void SetObject(size_t index, const _Object &object) {
-    std::lock_guard<std::recursive_mutex> guard(m_mutex);
-    m_collection.at(index) = object;
-  }
-
-  std::recursive_mutex &GetMutex() { return m_mutex; }
-
-private:
-  collection m_collection;
-  mutable std::recursive_mutex m_mutex;
-
-  // For ThreadSafeSTLVector only
-  DISALLOW_COPY_AND_ASSIGN(ThreadSafeSTLVector);
-};
-
-} // namespace lldb_private
-
-#endif // liblldb_ThreadSafeSTLVector_h_

lldb/include/lldb/Core/ValueObjectSyntheticFilter.h

 //===-- ValueObjectSyntheticFilter.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 liblldb_ValueObjectSyntheticFilter_h_
 #define liblldb_ValueObjectSyntheticFilter_h_
 
-#include "lldb/Core/ThreadSafeSTLMap.h"
-#include "lldb/Core/ThreadSafeSTLVector.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Symbol/CompilerType.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private-enumerations.h"
 
protected:
 
   virtual void CreateSynthFilter();
 
   // we need to hold on to the SyntheticChildren because someone might delete
   // the type binding while we are alive
   lldb::SyntheticChildrenSP m_synth_sp;
   std::unique_ptr<SyntheticChildrenFrontEnd> m_synth_filter_up;
 
-  typedef ThreadSafeSTLMap<uint32_t, ValueObject *> ByIndexMap;
-  typedef ThreadSafeSTLMap<const char *, uint32_t> NameToIndexMap;
-  typedef ThreadSafeSTLVector<lldb::ValueObjectSP> SyntheticChildrenCache;
+  typedef std::map<uint32_t, ValueObject *> ByIndexMap;
+  typedef std::map<const char *, uint32_t> NameToIndexMap;
+  typedef std::vector<lldb::ValueObjectSP> SyntheticChildrenCache;
 
   typedef ByIndexMap::iterator ByIndexIterator;
   typedef NameToIndexMap::iterator NameToIndexIterator;
 
+  std::mutex m_child_mutex;
+  /// Guarded by m_child_mutex;
   ByIndexMap m_children_byindex;
+  /// Guarded by m_child_mutex;
   NameToIndexMap m_name_toindex;
+  /// Guarded by m_child_mutex;
+  SyntheticChildrenCache m_synthetic_children_cache;
+
   uint32_t m_synthetic_children_count; // FIXME use the ValueObject's
                                        // ChildrenManager instead of a special
                                        // purpose solution
-  SyntheticChildrenCache m_synthetic_children_cache;
 
   ConstString m_parent_type_name;
 
   LazyBool m_might_have_children;
 
   LazyBool m_provides_value;
 
 private:

lldb/source/Core/ValueObjectSyntheticFilter.cpp

public:
   bool MightHaveChildren() override { return true; }
 
   bool Update() override { return false; }
 };
 
 ValueObjectSynthetic::ValueObjectSynthetic(ValueObject &parent,
                                            lldb::SyntheticChildrenSP filter)
     : ValueObject(parent), m_synth_sp(filter), m_children_byindex(),
-      m_name_toindex(), m_synthetic_children_count(UINT32_MAX),
-      m_synthetic_children_cache(), m_parent_type_name(parent.GetTypeName()),
+      m_name_toindex(), m_synthetic_children_cache(),
+      m_synthetic_children_count(UINT32_MAX),
+      m_parent_type_name(parent.GetTypeName()),
       m_might_have_children(eLazyBoolCalculate),
       m_provides_value(eLazyBoolCalculate) {
   SetName(parent.GetName());
   CopyValueData(m_parent);
   CreateSynthFilter();
 }
 
 ValueObjectSynthetic::~ValueObjectSynthetic() = default;
bool ValueObjectSynthetic::UpdateValue() {
 
   // let our backend do its update
   if (!m_synth_filter_up->Update()) {
     LLDB_LOGF(log,
               "[ValueObjectSynthetic::UpdateValue] name=%s, synthetic "
               "filter said caches are stale - clearing",
               GetName().AsCString());
     // filter said that cached values are stale
-    m_children_byindex.Clear();
-    m_name_toindex.Clear();
+    {
+      std::lock_guard<std::mutex> guard(m_child_mutex);
+      m_children_byindex.clear();
+      m_name_toindex.clear();
+    }
     // usually, an object's value can change but this does not alter its
     // children count for a synthetic VO that might indeed happen, so we need
     // to tell the upper echelons that they need to come back to us asking for
     // children
     m_children_count_valid = false;
-    m_synthetic_children_cache.Clear();
+    {
+      std::lock_guard<std::mutex> guard(m_child_mutex);
+      m_synthetic_children_cache.clear();
+    }
     m_synthetic_children_count = UINT32_MAX;
     m_might_have_children = eLazyBoolCalculate;
   } else {
     LLDB_LOGF(log,
               "[ValueObjectSynthetic::UpdateValue] name=%s, synthetic "
               "filter said caches are still valid",
               GetName().AsCString());
   }
lldb::ValueObjectSP ValueObjectSynthetic::GetChildAtIndex(size_t idx,
   LLDB_LOGF(log,
             "[ValueObjectSynthetic::GetChildAtIndex] name=%s, retrieving "
             "child at index %zu",
             GetName().AsCString(), idx);
 
   UpdateValueIfNeeded();
 
   ValueObject *valobj;
-  if (!m_children_byindex.GetValueForKey(idx, valobj)) {
+  bool child_is_cached;
+  {
+    std::lock_guard<std::mutex> guard(m_child_mutex);
+    auto cached_child_it = m_children_byindex.find(idx);
+    child_is_cached = cached_child_it != m_children_byindex.end();
+    if (child_is_cached)
+      valobj = cached_child_it->second;
+  }
+
+  if (!child_is_cached) {
     if (can_create && m_synth_filter_up != nullptr) {
       LLDB_LOGF(log,
                 "[ValueObjectSynthetic::GetChildAtIndex] name=%s, child at "
                 "index %zu not cached and will be created",
                 GetName().AsCString(), idx);
 
       lldb::ValueObjectSP synth_guy = m_synth_filter_up->GetChildAtIndex(idx);
 
       LLDB_LOGF(
           log,
           "[ValueObjectSynthetic::GetChildAtIndex] name=%s, child at index "
           "%zu created as %p (is "
           "synthetic: %s)",
           GetName().AsCString(), idx, static_cast<void *>(synth_guy.get()),
           synth_guy.get()
               ? (synth_guy->IsSyntheticChildrenGenerated() ? "yes" : "no")
               : "no");
 
       if (!synth_guy)
         return synth_guy;
 
+      {
+        std::lock_guard<std::mutex> guard(m_child_mutex);
         if (synth_guy->IsSyntheticChildrenGenerated())
-        m_synthetic_children_cache.AppendObject(synth_guy);
-      m_children_byindex.SetValueForKey(idx, synth_guy.get());
+          m_synthetic_children_cache.push_back(synth_guy);
+        m_children_byindex[idx] = synth_guy.get();
+      }
       synth_guy->SetPreferredDisplayLanguageIfNeeded(
           GetPreferredDisplayLanguage());
       return synth_guy;
     } else {
       LLDB_LOGF(log,
                 "[ValueObjectSynthetic::GetChildAtIndex] name=%s, child at "
                 "index %zu not cached and cannot "
                 "be created (can_create = %s, synth_filter = %p)",
ValueObjectSynthetic::GetChildMemberWithName(ConstString name,
 
   return GetChildAtIndex(index, can_create);
 }
 
 size_t ValueObjectSynthetic::GetIndexOfChildWithName(ConstString name) {
   UpdateValueIfNeeded();
 
   uint32_t found_index = UINT32_MAX;
-  bool did_find = m_name_toindex.GetValueForKey(name.GetCString(), found_index);
+  bool did_find;
+  {
+    std::lock_guard<std::mutex> guard(m_child_mutex);
+    auto name_to_index = m_name_toindex.find(name.GetCString());
+    did_find = name_to_index != m_name_toindex.end();
+    if (did_find)
+      found_index = name_to_index->second;
+  }
 
   if (!did_find && m_synth_filter_up != nullptr) {
     uint32_t index = m_synth_filter_up->GetIndexOfChildWithName(name);
     if (index == UINT32_MAX)
       return index;
-    m_name_toindex.SetValueForKey(name.GetCString(), index);
+    std::lock_guard<std::mutex> guard(m_child_mutex);
+    m_name_toindex[name.GetCString()] = index;
     return index;
   } else if (!did_find && m_synth_filter_up == nullptr)
     return UINT32_MAX;
   else /*if (iter != m_name_toindex.end())*/
     return found_index;
 }
 
 bool ValueObjectSynthetic::IsInScope() { return m_parent->IsInScope(); }