[lldb][NFCI] Change return type of PersistentExpressionState::GetNextPersistentVariableName

[bulbazord]

Summary

These don't really need to be in the ConstString StringPool. Note that
they still end up there because we need to wrap them in ConstStrings to
use them with certain APIs, but we can stop creating them as
ConstStrings to start.

[aprantl]

Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, otherwise it seems to needlessly complicate things.

[bulbazord]

In D152324#4403368, @aprantl wrote:

Is the idea to remove the ConstString from these APIs too? If yes, this LGTM, otherwise it seems to needlessly complicate things.

Yes, I was going to remove ConstString from these other APIs too. I can let this patch sit here until I can do more work to make sure that happens though. I'm somewhat confident that removing ConstString from this area of LLDB is going to be better long-term, but I've been wrong about this before! :)

[JDevlieghere]

LGTM

lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp

129–134

If we're going to return a std::string, we might as well use a raw_string_ostream and simplify this function.

[fdeazeve]

lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp

129–134

+1, we can avoid the conversion of SmallString->std::string (and enable copy elision) by declaring name as std::string

(we don't necessarily need to do it in this patch though, either way is fine IMO)

[jingham]

I wonder about this one. In every instance where the API is used, its result is turned into a ConstString first. That's because this variable name lives in the same slot as normal variable names, which come from the debug information and so tend to be in the ConstString pool for better reasons. Do you project being able to get rid of that latter requirement? If not, it seems a bit odd to go to the trouble to avoid this value starting life as a ConstString when the first thing everybody does with it is to turn it into a ConstString.

[bulbazord]

In D152324#4415324, @jingham wrote:

I wonder about this one. In every instance where the API is used, its result is turned into a ConstString first. That's because this variable name lives in the same slot as normal variable names, which come from the debug information and so tend to be in the ConstString pool for better reasons. Do you project being able to get rid of that latter requirement? If not, it seems a bit odd to go to the trouble to avoid this value starting life as a ConstString when the first thing everybody does with it is to turn it into a ConstString.

That's the major reason I'm sitting on this right now. I want to gather some more evidence and test things more before deciding this is the direction to go in. I'm pretty sure ConstString in its current form is the wrong abstraction for this, but perhaps some kind of variation of ConstString is the right call? We'll see...

lldb/include/lldb/Expression/ExpressionVariable.h

public:
 
   virtual lldb::ExpressionVariableSP
   CreatePersistentVariable(ExecutionContextScope *exe_scope,
                            ConstString name, const CompilerType &type,
                            lldb::ByteOrder byte_order,
                            uint32_t addr_byte_size) = 0;
 
   /// Return a new persistent variable name with the specified prefix.
-  virtual ConstString GetNextPersistentVariableName(bool is_error = false) = 0;
+  virtual std::string GetNextPersistentVariableName(bool is_error = false) = 0;
 
   virtual void
   RemovePersistentVariable(lldb::ExpressionVariableSP variable) = 0;
 
   virtual std::optional<CompilerType>
   GetCompilerTypeFromPersistentDecl(ConstString type_name) = 0;
 
   virtual lldb::addr_t LookupSymbol(ConstString name);

lldb/source/Core/ValueObject.cpp

ValueObjectSP ValueObject::Persist() {
 
   PersistentExpressionState *persistent_state =
       target_sp->GetPersistentExpressionStateForLanguage(
           GetPreferredDisplayLanguage());
 
   if (!persistent_state)
     return nullptr;
 
-  ConstString name = persistent_state->GetNextPersistentVariableName();
+  const std::string name = persistent_state->GetNextPersistentVariableName();
 
-  ValueObjectSP const_result_sp =
-      ValueObjectConstResult::Create(target_sp.get(), GetValue(), name);
+  ValueObjectSP const_result_sp = ValueObjectConstResult::Create(
+      target_sp.get(), GetValue(), ConstString(name));
 
   ExpressionVariableSP persistent_var_sp =
       persistent_state->CreatePersistentVariable(const_result_sp);
   persistent_var_sp->m_live_sp = persistent_var_sp->m_frozen_sp;
   persistent_var_sp->m_flags |= ExpressionVariable::EVIsProgramReference;
 
   return persistent_var_sp->GetValueObject();
 }

lldb/source/Expression/Materializer.cpp

void Dematerialize(lldb::StackFrameSP &frame_sp, IRMemoryMap &map,
 
     if (!persistent_state) {
       err.SetErrorString("Couldn't dematerialize a result variable: "
                          "corresponding type system doesn't handle persistent "
                          "variables");
       return;
     }
 
-    ConstString name = m_delegate
-                           ? m_delegate->GetName()
-                           : persistent_state->GetNextPersistentVariableName();
+    ConstString name =
+        m_delegate
+            : ConstString(persistent_state->GetNextPersistentVariableName());
 
     lldb::ExpressionVariableSP ret = persistent_state->CreatePersistentVariable(
         exe_scope, name, m_type, map.GetByteOrder(), map.GetAddressByteSize());
 
     if (!ret) {
       err.SetErrorStringWithFormat("couldn't dematerialize a result variable: "
                                    "failed to make persistent variable %s",
                                    name.AsCString());

lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.h

public:
 
   lldb::ExpressionVariableSP CreatePersistentVariable(
       ExecutionContextScope *exe_scope, ConstString name,
       const CompilerType &compiler_type, lldb::ByteOrder byte_order,
       uint32_t addr_byte_size) override;
 
   void RemovePersistentVariable(lldb::ExpressionVariableSP variable) override;
 
-  ConstString GetNextPersistentVariableName(bool is_error = false) override;
+  std::string GetNextPersistentVariableName(bool is_error = false) override;
 
   /// Returns the next file name that should be used for user expressions.
   std::string GetNextExprFileName() {
     std::string name;
     name.append("<user expression ");
     name.append(std::to_string(m_next_user_file_id++));
     name.append(">");
     return name;

lldb/source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp

 ClangPersistentVariables::GetClangModulesDeclVendor() {
   if (!m_modules_decl_vendor_sp) {
     m_modules_decl_vendor_sp.reset(
         ClangModulesDeclVendor::Create(*m_target_sp));
   }
   return m_modules_decl_vendor_sp;
 }
 
-ConstString
+std::string
 ClangPersistentVariables::GetNextPersistentVariableName(bool is_error) {
   llvm::SmallString<64> name;
   {
     llvm::raw_svector_ostream os(name);
     os << GetPersistentVariablePrefix(is_error)
        << m_next_persistent_variable_id++;
   }
-  return ConstString(name);
+  return std::string(name);
 }

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

 
 void ClangUserExpression::ClangUserExpressionHelper::CommitPersistentDecls() {
   if (m_result_synthesizer_up) {
     m_result_synthesizer_up->CommitPersistentDecls();
   }
 }
 
 ConstString ClangUserExpression::ResultDelegate::GetName() {
-  return m_persistent_state->GetNextPersistentVariableName(false);
+  return ConstString(m_persistent_state->GetNextPersistentVariableName(false));
 }
 
 void ClangUserExpression::ResultDelegate::DidDematerialize(
     lldb::ExpressionVariableSP &variable) {
   m_variable = variable;
 }
 
 void ClangUserExpression::ResultDelegate::RegisterPersistentState(
     PersistentExpressionState *persistent_state) {
   m_persistent_state = persistent_state;
 }
 
 lldb::ExpressionVariableSP &ClangUserExpression::ResultDelegate::GetVariable() {
   return m_variable;
 }

lldb/source/Target/ABI.cpp

if (persistent) {
     Target &target = *thread.CalculateTarget();
     PersistentExpressionState *persistent_expression_state =
         target.GetPersistentExpressionStateForLanguage(
             ast_type.GetMinimumLanguage());
 
     if (!persistent_expression_state)
       return {};
 
-    ConstString persistent_variable_name =
-        persistent_expression_state->GetNextPersistentVariableName();
+    ConstString persistent_variable_name = ConstString(
+        persistent_expression_state->GetNextPersistentVariableName());
 
     lldb::ValueObjectSP const_valobj_sp;
 
     // Check in case our value is already a constant value
     if (return_valobj_sp->GetIsConstant()) {
       const_valobj_sp = return_valobj_sp;
       const_valobj_sp->SetName(persistent_variable_name);
     } else