[lldb] Change ObjectValueDictionary to use a StringMap

llvm has a structure for maps where the key's type is a string. Using
that also means that the keys for OptionValueDictionary don't stick
around forever in ConstString's StringPool (even after they are gone).

The only thing we lose here is ordering: iterating over the map where the keys
are ConstStrings guarantees that we iterate in alphabetical order.
StringMap makes no guarantees about the ordering when you iterate over
the entire map.

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

lldb/include/lldb/Interpreter/OptionValueDictionary.h

@@ -9,11 +9,11 @@
 #ifndef LLDB_INTERPRETER_OPTIONVALUEDICTIONARY_H
 #define LLDB_INTERPRETER_OPTIONVALUEDICTIONARY_H
 
-#include <map>
-
 #include "lldb/Interpreter/OptionValue.h"
 #include "lldb/lldb-private-types.h"
 
+#include "llvm/ADT/StringMap.h"
+
 namespace lldb_private {
 
 class OptionValueDictionary
@@ -58,7 +58,7 @@ class OptionValueDictionary
 
   size_t GetNumValues() const { return m_values.size(); }
 
-  lldb::OptionValueSP GetValueForKey(ConstString key) const;
+  lldb::OptionValueSP GetValueForKey(llvm::StringRef key) const;
 
   lldb::OptionValueSP GetSubValue(const ExecutionContext *exe_ctx,
                                   llvm::StringRef name, bool will_modify,
@@ -67,21 +67,19 @@ class OptionValueDictionary
   Status SetSubValue(const ExecutionContext *exe_ctx, VarSetOperationType op,
                      llvm::StringRef name, llvm::StringRef value) override;
 
-  bool SetValueForKey(ConstString key,
-                      const lldb::OptionValueSP &value_sp,
+  bool SetValueForKey(llvm::StringRef key, const lldb::OptionValueSP &value_sp,
                       bool can_replace = true);
 
-  bool DeleteValueForKey(ConstString key);
+  bool DeleteValueForKey(llvm::StringRef key);
 
   size_t GetArgs(Args &args) const;
 
   Status SetArgs(const Args &args, VarSetOperationType op);
 
 protected:
-  typedef std::map<ConstString, lldb::OptionValueSP> collection;
   uint32_t m_type_mask;
   OptionEnumValues m_enum_values;
-  collection m_values;
+  llvm::StringMap<lldb::OptionValueSP> m_values;
   bool m_raw_value_dump;
 };
 

lldb/source/Core/Disassembler.cpp

@@ -755,7 +755,7 @@ OptionValueSP Instruction::ReadDictionary(FILE *in_file, Stream *out_stream) {
   char buffer[1024];
 
   auto option_value_sp = std::make_shared<OptionValueDictionary>();
-  static ConstString encoding_key("data_encoding");
+  static constexpr llvm::StringLiteral encoding_key("data_encoding");
   OptionValue::Type data_type = OptionValue::eTypeInvalid;
 
   while (!done) {
@@ -802,7 +802,6 @@ OptionValueSP Instruction::ReadDictionary(FILE *in_file, Stream *out_stream) {
         return option_value_sp;
       }
 
-      ConstString const_key(key.c_str());
       // Check value to see if it's the start of an array or dictionary.
 
       lldb::OptionValueSP value_sp;
@@ -838,15 +837,14 @@ OptionValueSP Instruction::ReadDictionary(FILE *in_file, Stream *out_stream) {
         value_sp = std::make_shared<OptionValueString>(value.c_str(), "");
       }
 
-      if (const_key == encoding_key) {
+      if (key == encoding_key) {
         // A 'data_encoding=..." is NOT a normal key-value pair; it is meta-data
-        // indicating the
-        // data type of an upcoming array (usually the next bit of data to be
-        // read in).
-        if (strcmp(value.c_str(), "uint32_t") == 0)
+        // indicating the data type of an upcoming array (usually the next bit
+        // of data to be read in).
+        if (llvm::StringRef(value) == "uint32_t")
           data_type = OptionValue::eTypeUInt64;
       } else
-        option_value_sp->GetAsDictionary()->SetValueForKey(const_key, value_sp,
+        option_value_sp->GetAsDictionary()->SetValueForKey(key, value_sp,
                                                            false);
     }
   }

lldb/source/Interpreter/OptionValueDictionary.cpp

@@ -33,20 +33,24 @@ void OptionValueDictionary::DumpValue(const ExecutionContext *exe_ctx,
     if (dump_mask & eDumpOptionType)
       strm.PutCString(" =");
 
-    collection::iterator pos, end = m_values.end();
-
     if (!one_line)
       strm.IndentMore();
 
-    for (pos = m_values.begin(); pos != end; ++pos) {
-      OptionValue *option_value = pos->second.get();
+    // m_values is not guaranteed to be sorted alphabetically, so for
+    // consistentcy we will sort them here before dumping
+    std::map<llvm::StringRef, OptionValue *> sorted_values;
+    for (const auto &value : m_values) {
+      sorted_values[value.first()] = value.second.get();
+    }
+    for (const auto &value : sorted_values) {
+      OptionValue *option_value = value.second;
 
       if (one_line)
         strm << ' ';
       else
         strm.EOL();
 
-      strm.Indent(pos->first.GetStringRef());
+      strm.Indent(value.first);
 
       const uint32_t extra_dump_options = m_raw_value_dump ? eDumpOptionRaw : 0;
       switch (dict_type) {
@@ -87,18 +91,17 @@ llvm::json::Value
 OptionValueDictionary::ToJSON(const ExecutionContext *exe_ctx) {
   llvm::json::Object dict;
   for (const auto &value : m_values) {
-    dict.try_emplace(value.first.GetCString(), value.second->ToJSON(exe_ctx));
+    dict.try_emplace(value.first(), value.second->ToJSON(exe_ctx));
   }
   return dict;
 }
 
 size_t OptionValueDictionary::GetArgs(Args &args) const {
   args.Clear();
-  collection::const_iterator pos, end = m_values.end();
-  for (pos = m_values.begin(); pos != end; ++pos) {
+  for (const auto &value : m_values) {
     StreamString strm;
-    strm.Printf("%s=", pos->first.GetCString());
-    pos->second->DumpValue(nullptr, strm, eDumpOptionValue | eDumpOptionRaw);
+    strm.Printf("%s=", value.first().data());
+    value.second->DumpValue(nullptr, strm, eDumpOptionValue | eDumpOptionRaw);
     args.AppendArgument(strm.GetString());
   }
   return args.GetArgumentCount();
@@ -178,15 +181,15 @@ Status OptionValueDictionary::SetArgs(const Args &args,
         if (error.Fail())
           return error;
         m_value_was_set = true;
-        SetValueForKey(ConstString(key), enum_value, true);
+        SetValueForKey(key, enum_value, true);
       } else {
         lldb::OptionValueSP value_sp(CreateValueFromCStringForTypeMask(
             value.str().c_str(), m_type_mask, error));
         if (value_sp) {
           if (error.Fail())
             return error;
           m_value_was_set = true;
-          SetValueForKey(ConstString(key), value_sp, true);
+          SetValueForKey(key, value_sp, true);
         } else {
           error.SetErrorString("dictionaries that can contain multiple types "
                                "must subclass OptionValueArray");
@@ -198,11 +201,11 @@ Status OptionValueDictionary::SetArgs(const Args &args,
   case eVarSetOperationRemove:
     if (argc > 0) {
       for (size_t i = 0; i < argc; ++i) {
-        ConstString key(args.GetArgumentAtIndex(i));
+        llvm::StringRef key(args.GetArgumentAtIndex(i));
         if (!DeleteValueForKey(key)) {
           error.SetErrorStringWithFormat(
               "no value found named '%s', aborting remove operation",
-              key.GetCString());
+              key.data());
           break;
         }
       }
@@ -267,7 +270,7 @@ OptionValueDictionary::GetSubValue(const ExecutionContext *exe_ctx,
     return nullptr;
   }
 
-  value_sp = GetValueForKey(ConstString(key));
+  value_sp = GetValueForKey(key);
   if (!value_sp) {
     error.SetErrorStringWithFormat(
       "dictionary does not contain a value for the key name '%s'",
@@ -297,22 +300,22 @@ Status OptionValueDictionary::SetSubValue(const ExecutionContext *exe_ctx,
 }
 
 lldb::OptionValueSP
-OptionValueDictionary::GetValueForKey(ConstString key) const {
+OptionValueDictionary::GetValueForKey(llvm::StringRef key) const {
   lldb::OptionValueSP value_sp;
-  collection::const_iterator pos = m_values.find(key);
+  auto pos = m_values.find(key);
   if (pos != m_values.end())
     value_sp = pos->second;
   return value_sp;
 }
 
-bool OptionValueDictionary::SetValueForKey(ConstString key,
+bool OptionValueDictionary::SetValueForKey(llvm::StringRef key,
                                            const lldb::OptionValueSP &value_sp,
                                            bool can_replace) {
   // Make sure the value_sp object is allowed to contain values of the type
   // passed in...
   if (value_sp && (m_type_mask & value_sp->GetTypeAsMask())) {
     if (!can_replace) {
-      collection::const_iterator pos = m_values.find(key);
+      auto pos = m_values.find(key);
       if (pos != m_values.end())
         return false;
     }
@@ -322,8 +325,8 @@ bool OptionValueDictionary::SetValueForKey(ConstString key,
   return false;
 }
 
-bool OptionValueDictionary::DeleteValueForKey(ConstString key) {
-  collection::iterator pos = m_values.find(key);
+bool OptionValueDictionary::DeleteValueForKey(llvm::StringRef key) {
+  auto pos = m_values.find(key);
   if (pos != m_values.end()) {
     m_values.erase(pos);
     return true;

lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp

@@ -18,7 +18,6 @@
 #include "lldb/Interpreter/OptionValueDictionary.h"
 #include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Stream.h"
 
 #include "Plugins/Process/Utility/ARMDefines.h"
@@ -14353,9 +14352,9 @@ bool EmulateInstructionARM::TestEmulation(Stream *out_stream, ArchSpec &arch,
     return false;
   }
 
-  static ConstString opcode_key("opcode");
-  static ConstString before_key("before_state");
-  static ConstString after_key("after_state");
+  static constexpr llvm::StringLiteral opcode_key("opcode");
+  static constexpr llvm::StringLiteral before_key("before_state");
+  static constexpr llvm::StringLiteral after_key("after_state");
 
   OptionValueSP value_sp = test_data->GetValueForKey(opcode_key);
 

lldb/source/Plugins/Instruction/ARM/EmulateInstructionARM.h

@@ -11,7 +11,6 @@
 
 #include "Plugins/Process/Utility/ARMDefines.h"
 #include "lldb/Core/EmulateInstruction.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Status.h"
 #include <optional>
 

lldb/source/Plugins/Instruction/ARM/EmulationStateARM.cpp

@@ -264,8 +264,7 @@ bool EmulationStateARM::LoadRegistersStateFromDictionary(
   for (int i = 0; i < num; ++i) {
     sstr.Clear();
     sstr.Printf("%c%d", kind, i);
-    OptionValueSP value_sp =
-        reg_dict->GetValueForKey(ConstString(sstr.GetString()));
+    OptionValueSP value_sp = reg_dict->GetValueForKey(sstr.GetString());
     if (value_sp.get() == nullptr)
       return false;
     uint64_t reg_value = value_sp->GetUInt64Value();
@@ -277,8 +276,8 @@ bool EmulationStateARM::LoadRegistersStateFromDictionary(
 
 bool EmulationStateARM::LoadStateFromDictionary(
     OptionValueDictionary *test_data) {
-  static ConstString memory_key("memory");
-  static ConstString registers_key("registers");
+  static constexpr llvm::StringLiteral memory_key("memory");
+  static constexpr llvm::StringLiteral registers_key("registers");
 
   if (!test_data)
     return false;
@@ -288,8 +287,8 @@ bool EmulationStateARM::LoadStateFromDictionary(
   // Load memory, if present.
 
   if (value_sp.get() != nullptr) {
-    static ConstString address_key("address");
-    static ConstString data_key("data");
+    static constexpr llvm::StringLiteral address_key("address");
+    static constexpr llvm::StringLiteral data_key("data");
     uint64_t start_address = 0;
 
     OptionValueDictionary *mem_dict = value_sp->GetAsDictionary();
@@ -327,7 +326,7 @@ bool EmulationStateARM::LoadStateFromDictionary(
   if (!LoadRegistersStateFromDictionary(reg_dict, 'r', dwarf_r0, 16))
     return false;
 
-  static ConstString cpsr_name("cpsr");
+  static constexpr llvm::StringLiteral cpsr_name("cpsr");
   value_sp = reg_dict->GetValueForKey(cpsr_name);
   if (value_sp.get() == nullptr)
     return false;

lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp

@@ -12,7 +12,6 @@
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Symbol/UnwindPlan.h"
 #include "lldb/Utility/ArchSpec.h"
-#include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/RegisterValue.h"
 #include "lldb/Utility/Stream.h"
 

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

@@ -300,20 +300,18 @@ size_t ObjectFilePECOFF::GetModuleSpecifications(
     if (!module_env_option) {
       // Step 2: Try with the file name in lowercase.
       auto name_lower = name.GetStringRef().lower();
-      module_env_option =
-          map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+      module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
     }
     if (!module_env_option) {
       // Step 3: Try with the file name with ".debug" suffix stripped.
       auto name_stripped = name.GetStringRef();
       if (name_stripped.consume_back_insensitive(".debug")) {
-        module_env_option = map->GetValueForKey(ConstString(name_stripped));
+        module_env_option = map->GetValueForKey(name_stripped);
         if (!module_env_option) {
           // Step 4: Try with the file name in lowercase with ".debug" suffix
           // stripped.
           auto name_lower = name_stripped.lower();
-          module_env_option =
-              map->GetValueForKey(ConstString(llvm::StringRef(name_lower)));
+          module_env_option = map->GetValueForKey(llvm::StringRef(name_lower));
         }
       }
     }

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

@@ -60,7 +60,7 @@ def cleanup(setting):
         self.assertEqual(self.dbg.GetSelectedTarget().GetNumBreakpoints(), 1)
 
     def test_append_target_env_vars(self):
-        """Test that 'append target.run-args' works."""
+        """Test that 'append target.env-vars' works."""
         # Append the env-vars.
         self.runCmd('settings append target.env-vars MY_ENV_VAR=YES')
         # And add hooks to restore the settings during tearDown().
@@ -268,7 +268,7 @@ def do_test_run_args_and_env_vars(self, use_launchsimple):
                 found_env_var = True
                 break
         self.assertTrue(found_env_var,
-                        "MY_ENV_VAR was not set in LunchInfo object")
+                        "MY_ENV_VAR was not set in LaunchInfo object")
 
         self.assertEqual(launch_info.GetNumArguments(), 3)
         self.assertEqual(launch_info.GetArgumentAtIndex(0), "A")

lldb/unittests/Interpreter/TestOptionValue.cpp

@@ -146,12 +146,12 @@ TEST(TestProperties, DeepCopy) {
   ASSERT_TRUE(dict_copy_ptr->OptionWasSet());
   ASSERT_EQ(dict_copy_ptr->GetNumValues(), 2U);
 
-  auto value_ptr = dict_copy_ptr->GetValueForKey(ConstString("A"));
+  auto value_ptr = dict_copy_ptr->GetValueForKey("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"));
+  value_ptr = dict_copy_ptr->GetValueForKey("B");
   ASSERT_TRUE(value_ptr);
   ASSERT_EQ(value_ptr->GetParent().get(), dict_copy_ptr);
   ASSERT_EQ(value_ptr->GetUInt64Value(), 2U);