Make UniqueCStringMap work with non-default-constructible types and other improvements/cleanups

Summary:
The motivation for this was me wanting to make the validity of dwarf
DIERefs explicit (via llvm::Optional<DIERef>). This meant that the class
would no longer have a default constructor. As the DIERef was being
stored in a UniqueCStringMap, this meant that this container (like all
standard containers) needed to work with non-default-constructible types
too.

This part is achieved by removing the default constructors for the map
entry types, and providing appropriate comparison overloads so that we
can search for map entries without constructing a dummy entry. While
doing that, I took the opportunity to modernize the code, and add some
tests. Functions that were completely unused are deleted.

This required also some changes in the Symtab code, as it was default
constructing map entries, which was not impossible even though its
value type was default-constructible. Technically, these changes could
be avoided with some SFINAE on the entry type, but I felt that the code
is cleaner this way anyway.

Reviewers: JDevlieghere, sgraenitz

Subscribers: mgorny, aprantl, lldb-commits

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

llvm-svn: 363357

lldb/include/lldb/Core/UniqueCStringMap.h

@@ -26,16 +26,10 @@ namespace lldb_private {
 template <typename T> class UniqueCStringMap {
 public:
   struct Entry {
-    Entry() {}
-
-    Entry(ConstString cstr) : cstring(cstr), value() {}
-
     Entry(ConstString cstr, const T &v) : cstring(cstr), value(v) {}
 
-    // This is only for uniqueness, not lexicographical ordering, so we can
-    // just compare pointers.
-    bool operator<(const Entry &rhs) const {
-      return cstring.GetCString() < rhs.cstring.GetCString();
+    friend bool operator<(const Entry &lhs, const Entry &rhs) {
+      return Compare()(lhs, rhs);
     }
 
     ConstString cstring;
@@ -53,17 +47,6 @@ public:
 
   void Clear() { m_map.clear(); }
 
-  // Call this function to always keep the map sorted when putting entries into
-  // the map.
-  void Insert(ConstString unique_cstr, const T &value) {
-    typename UniqueCStringMap<T>::Entry e(unique_cstr, value);
-    m_map.insert(std::upper_bound(m_map.begin(), m_map.end(), e), e);
-  }
-
-  void Insert(const Entry &e) {
-    m_map.insert(std::upper_bound(m_map.begin(), m_map.end(), e), e);
-  }
-
   // Get an entries by index in a variety of forms.
   //
   // The caller is responsible for ensuring that the collection does not change
@@ -101,13 +84,9 @@ public:
   // T values and only if there is a sensible failure value that can
   // be returned and that won't match any existing values.
   T Find(ConstString unique_cstr, T fail_value) const {
-    Entry search_entry(unique_cstr);
-    const_iterator end = m_map.end();
-    const_iterator pos = std::lower_bound(m_map.begin(), end, search_entry);
-    if (pos != end) {
-      if (pos->cstring == unique_cstr)
-        return pos->value;
-    }
+    auto pos = llvm::lower_bound(m_map, unique_cstr, Compare());
+    if (pos != m_map.end() && pos->cstring == unique_cstr)
+      return pos->value;
     return fail_value;
   }
 
@@ -117,10 +96,8 @@ public:
   // The caller is responsible for ensuring that the collection does not change
   // during while using the returned pointer.
   const Entry *FindFirstValueForName(ConstString unique_cstr) const {
-    Entry search_entry(unique_cstr);
-    const_iterator end = m_map.end();
-    const_iterator pos = std::lower_bound(m_map.begin(), end, search_entry);
-    if (pos != end && pos->cstring == unique_cstr)
+    auto pos = llvm::lower_bound(m_map, unique_cstr, Compare());
+    if (pos != m_map.end() && pos->cstring == unique_cstr)
       return &(*pos);
     return nullptr;
   }
@@ -147,15 +124,9 @@ public:
   size_t GetValues(ConstString unique_cstr, std::vector<T> &values) const {
     const size_t start_size = values.size();
 
-    Entry search_entry(unique_cstr);
-    const_iterator pos, end = m_map.end();
-    for (pos = std::lower_bound(m_map.begin(), end, search_entry); pos != end;
-         ++pos) {
-      if (pos->cstring == unique_cstr)
-        values.push_back(pos->value);
-      else
-        break;
-    }
+    for (const Entry &entry : llvm::make_range(std::equal_range(
+             m_map.begin(), m_map.end(), unique_cstr, Compare())))
+      values.push_back(entry.value);
 
     return values.size() - start_size;
   }
@@ -208,28 +179,27 @@ public:
     }
   }
 
-  size_t Erase(ConstString unique_cstr) {
-    size_t num_removed = 0;
-    Entry search_entry(unique_cstr);
-    iterator end = m_map.end();
-    iterator begin = m_map.begin();
-    iterator lower_pos = std::lower_bound(begin, end, search_entry);
-    if (lower_pos != end) {
-      if (lower_pos->cstring == unique_cstr) {
-        iterator upper_pos = std::upper_bound(lower_pos, end, search_entry);
-        if (lower_pos == upper_pos) {
-          m_map.erase(lower_pos);
-          num_removed = 1;
-        } else {
-          num_removed = std::distance(lower_pos, upper_pos);
-          m_map.erase(lower_pos, upper_pos);
-        }
-      }
-    }
-    return num_removed;
-  }
-
 protected:
+  struct Compare {
+    bool operator()(const Entry &lhs, const Entry &rhs) {
+      return operator()(lhs.cstring, rhs.cstring);
+    }
+
+    bool operator()(const Entry &lhs, ConstString rhs) {
+      return operator()(lhs.cstring, rhs);
+    }
+
+    bool operator()(ConstString lhs, const Entry &rhs) {
+      return operator()(lhs, rhs.cstring);
+    }
+
+    // This is only for uniqueness, not lexicographical ordering, so we can
+    // just compare pointers. *However*, comparing pointers from different
+    // allocations is UB, so we need compare their integral values instead.
+    bool operator()(ConstString lhs, ConstString rhs) {
+      return uintptr_t(lhs.GetCString()) < uintptr_t(rhs.GetCString());
+    }
+  };
   typedef std::vector<Entry> collection;
   typedef typename collection::iterator iterator;
   typedef typename collection::const_iterator const_iterator;

lldb/include/lldb/Symbol/Symtab.h

@@ -190,7 +190,7 @@ private:
                                         SymbolContextList &sc_list);
 
   void RegisterMangledNameEntry(
-      NameToIndexMap::Entry &entry, std::set<const char *> &class_contexts,
+      uint32_t value, std::set<const char *> &class_contexts,
       std::vector<std::pair<NameToIndexMap::Entry, const char *>> &backlog,
       RichManglingContext &rmc);
 

lldb/source/Symbol/Symtab.cpp

@@ -288,10 +288,8 @@ void Symtab::InitNameIndexes() {
     // Instantiation of the demangler is expensive, so better use a single one
     // for all entries during batch processing.
     RichManglingContext rmc;
-    NameToIndexMap::Entry entry;
-
-    for (entry.value = 0; entry.value < num_symbols; ++entry.value) {
-      Symbol *symbol = &m_symbols[entry.value];
+    for (uint32_t value = 0; value < num_symbols; ++value) {
+      Symbol *symbol = &m_symbols[value];
 
       // Don't let trampolines get into the lookup by name map If we ever need
       // the trampoline symbols to be searchable by name we can remove this and
@@ -303,52 +301,46 @@ void Symtab::InitNameIndexes() {
       // If the symbol's name string matched a Mangled::ManglingScheme, it is
       // stored in the mangled field.
       Mangled &mangled = symbol->GetMangled();
-      entry.cstring = mangled.GetMangledName();
-      if (entry.cstring) {
-        m_name_to_index.Append(entry);
+      if (ConstString name = mangled.GetMangledName()) {
+        m_name_to_index.Append(name, value);
 
         if (symbol->ContainsLinkerAnnotations()) {
           // If the symbol has linker annotations, also add the version without
           // the annotations.
-          entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
-                                        entry.cstring.GetStringRef()));
-          m_name_to_index.Append(entry);
+          ConstString stripped = ConstString(
+              m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
+          m_name_to_index.Append(stripped, value);
         }
 
         const SymbolType type = symbol->GetType();
         if (type == eSymbolTypeCode || type == eSymbolTypeResolver) {
           if (mangled.DemangleWithRichManglingInfo(rmc, lldb_skip_name))
-            RegisterMangledNameEntry(entry, class_contexts, backlog, rmc);
+            RegisterMangledNameEntry(value, class_contexts, backlog, rmc);
         }
       }
 
       // Symbol name strings that didn't match a Mangled::ManglingScheme, are
       // stored in the demangled field.
-      entry.cstring = mangled.GetDemangledName(symbol->GetLanguage());
-      if (entry.cstring) {
-        m_name_to_index.Append(entry);
+      if (ConstString name = mangled.GetDemangledName(symbol->GetLanguage())) {
+        m_name_to_index.Append(name, value);
 
         if (symbol->ContainsLinkerAnnotations()) {
           // If the symbol has linker annotations, also add the version without
           // the annotations.
-          entry.cstring = ConstString(m_objfile->StripLinkerSymbolAnnotations(
-                                        entry.cstring.GetStringRef()));
-          m_name_to_index.Append(entry);
+          name = ConstString(
+              m_objfile->StripLinkerSymbolAnnotations(name.GetStringRef()));
+          m_name_to_index.Append(name, value);
         }
-      }
 
-      // If the demangled name turns out to be an ObjC name, and is a category
-      // name, add the version without categories to the index too.
-      ObjCLanguage::MethodName objc_method(entry.cstring.GetStringRef(), true);
-      if (objc_method.IsValid(true)) {
-        entry.cstring = objc_method.GetSelector();
-        m_selector_to_index.Append(entry);
+        // If the demangled name turns out to be an ObjC name, and is a category
+        // name, add the version without categories to the index too.
+        ObjCLanguage::MethodName objc_method(name.GetStringRef(), true);
+        if (objc_method.IsValid(true)) {
+          m_selector_to_index.Append(objc_method.GetSelector(), value);
 
-        ConstString objc_method_no_category(
-            objc_method.GetFullNameWithoutCategory(true));
-        if (objc_method_no_category) {
-          entry.cstring = objc_method_no_category;
-          m_name_to_index.Append(entry);
+          if (ConstString objc_method_no_category =
+                  objc_method.GetFullNameWithoutCategory(true))
+            m_name_to_index.Append(objc_method_no_category, value);
         }
       }
     }
@@ -369,7 +361,7 @@ void Symtab::InitNameIndexes() {
 }
 
 void Symtab::RegisterMangledNameEntry(
-    NameToIndexMap::Entry &entry, std::set<const char *> &class_contexts,
+    uint32_t value, std::set<const char *> &class_contexts,
     std::vector<std::pair<NameToIndexMap::Entry, const char *>> &backlog,
     RichManglingContext &rmc) {
   // Only register functions that have a base name.
@@ -379,7 +371,7 @@ void Symtab::RegisterMangledNameEntry(
     return;
 
   // The base name will be our entry's name.
-  entry.cstring = ConstString(base_name);
+  NameToIndexMap::Entry entry(ConstString(base_name), value);
 
   rmc.ParseFunctionDeclContextName();
   llvm::StringRef decl_context = rmc.GetBufferRef();
@@ -447,24 +439,21 @@ void Symtab::AppendSymbolNamesToMap(const IndexCollection &indexes,
     std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
     // Create the name index vector to be able to quickly search by name
-    NameToIndexMap::Entry entry;
     const size_t num_indexes = indexes.size();
     for (size_t i = 0; i < num_indexes; ++i) {
-      entry.value = indexes[i];
+      uint32_t value = indexes[i];
       assert(i < m_symbols.size());
-      const Symbol *symbol = &m_symbols[entry.value];
+      const Symbol *symbol = &m_symbols[value];
 
       const Mangled &mangled = symbol->GetMangled();
       if (add_demangled) {
-        entry.cstring = mangled.GetDemangledName(symbol->GetLanguage());
-        if (entry.cstring)
-          name_to_index_map.Append(entry);
+        if (ConstString name = mangled.GetDemangledName(symbol->GetLanguage()))
+          name_to_index_map.Append(name, value);
       }
 
       if (add_mangled) {
-        entry.cstring = mangled.GetMangledName();
-        if (entry.cstring)
-          name_to_index_map.Append(entry);
+        if (ConstString name = mangled.GetMangledName())
+          name_to_index_map.Append(name, value);
       }
     }
   }

lldb/unittests/Core/CMakeLists.txt

@@ -2,6 +2,7 @@ add_lldb_unittest(LLDBCoreTests
   MangledTest.cpp
   RichManglingContextTest.cpp
   StreamCallbackTest.cpp
+  UniqueCStringMapTest.cpp
 
   LINK_LIBS
     lldbCore

lldb/unittests/Core/UniqueCStringMapTest.cpp

@@ -0,0 +1,53 @@
+//===-- UniqueCStringMapTest.cpp --------------------------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Core/UniqueCStringMap.h"
+#include "gmock/gmock.h"
+
+using namespace lldb_private;
+
+namespace {
+struct NoDefault {
+  int x;
+
+  NoDefault(int x) : x(x) {}
+  NoDefault() = delete;
+
+  friend bool operator==(NoDefault lhs, NoDefault rhs) {
+    return lhs.x == rhs.x;
+  }
+
+  friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
+                                       NoDefault x) {
+    return OS << "NoDefault{" << x.x << "}";
+  }
+};
+} // namespace
+
+TEST(UniqueCStringMap, NoDefaultConstructor) {
+  using MapT = UniqueCStringMap<NoDefault>;
+  using EntryT = MapT::Entry;
+
+  MapT Map;
+  ConstString Foo("foo"), Bar("bar");
+
+  Map.Append(Foo, NoDefault(42));
+  EXPECT_THAT(Map.Find(Foo, NoDefault(47)), NoDefault(42));
+  EXPECT_THAT(Map.Find(Bar, NoDefault(47)), NoDefault(47));
+  EXPECT_THAT(Map.FindFirstValueForName(Foo),
+              testing::Pointee(testing::Field(&EntryT::value, NoDefault(42))));
+  EXPECT_THAT(Map.FindFirstValueForName(Bar), nullptr);
+
+  std::vector<NoDefault> Values;
+  EXPECT_THAT(Map.GetValues(Foo, Values), 1);
+  EXPECT_THAT(Values, testing::ElementsAre(NoDefault(42)));
+
+  Values.clear();
+  EXPECT_THAT(Map.GetValues(Bar, Values), 0);
+  EXPECT_THAT(Values, testing::IsEmpty());
+}