Move from StringRef to std::string in the ScriptInterpreter API

[teemperor]

Summary

After D49309 it became clear that we always need a null-terminated string (for
the Python API), so we might as well change the API to accept an std::string&
instead of taking a StringRef and then always allocating a new std::string.

[dblaikie]

Seems good to me - though I haven't looked too closely/don't know this code terribly well (so you're welcome to wait if you know someone else more knowledgable might take a look too - or if you're fairly confident, commit & they can always follow up in post-commit)

[labath]

Normally this would be clearly a good thing, but the added complication here is that this function is part of a class hierarchy, and so this way you are forcing every implementation to take a std::string, even though only one of them cares about null-termination.

In performance-critical code, llvm would use llvm::Twine as an argument, which is able to avoid copies if the input string happens to be null-terminated (toNullTerminatedStringRef). However, this code is hardly that critical (and ScriptInterpreterPython is the only non-trivial class in the hierarchy), so I don't think it really matters what you do here.

[dblaikie]

In D49411#1164680, @labath wrote:

Normally this would be clearly a good thing, but the added complication here is that this function is part of a class hierarchy, and so this way you are forcing every implementation to take a std::string, even though only one of them cares about null-termination.

In performance-critical code, llvm would use llvm::Twine as an argument, which is able to avoid copies if the input string happens to be null-terminated (toNullTerminatedStringRef). However, this code is hardly that critical (and ScriptInterpreterPython is the only non-trivial class in the hierarchy), so I don't think it really matters what you do here.

Fair points all, Pavel - thanks for chiming in!

[clayborg]

Much simpler solution is inlined. Let me know what you think?

source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

859

Remove everything in this patch and just do:

Py_BuildValue("(Os#)", session_dict.get(), command.data(), (int)command.size()));

[teemperor]

@clayborg Seems reasonable, even though the Python docs say that we should use Py_ssize_t instead of int. I'll update the patch.

include/lldb/Interpreter/ScriptInterpreter.h

private:
     bool m_enable_io;
     bool m_set_lldb_globals;
     bool m_maskout_errors;
   };
 
   virtual bool Interrupt() { return false; }
 
   virtual bool ExecuteOneLine(
-      llvm::StringRef command, CommandReturnObject *result,
+      const std::string &command, CommandReturnObject *result,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) = 0;
 
   virtual void ExecuteInterpreterLoop() = 0;
 
   virtual bool ExecuteOneLineWithReturn(
-      llvm::StringRef in_string, ScriptReturnType return_type, void *ret_value,
+      const std::string &in_string, ScriptReturnType return_type,
+      void *ret_value,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) {
     return true;
   }
 
   virtual Status ExecuteMultipleLines(
       const char *in_string,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) {
     Status error;

source/Plugins/ScriptInterpreter/None/ScriptInterpreterNone.h

 
 class ScriptInterpreterNone : public ScriptInterpreter {
 public:
   ScriptInterpreterNone(CommandInterpreter &interpreter);
 
   ~ScriptInterpreterNone() override;
 
   bool ExecuteOneLine(
-      llvm::StringRef command, CommandReturnObject *result,
+      const std::string &command, CommandReturnObject *result,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
 
   void ExecuteInterpreterLoop() override;
 
   //------------------------------------------------------------------
   // Static Functions
   //------------------------------------------------------------------
   static void Initialize();

source/Plugins/ScriptInterpreter/None/ScriptInterpreterNone.cpp

 using namespace lldb;
 using namespace lldb_private;
 
 ScriptInterpreterNone::ScriptInterpreterNone(CommandInterpreter &interpreter)
     : ScriptInterpreter(interpreter, eScriptLanguageNone) {}
 
 ScriptInterpreterNone::~ScriptInterpreterNone() {}
 
-bool ScriptInterpreterNone::ExecuteOneLine(llvm::StringRef command,
+bool ScriptInterpreterNone::ExecuteOneLine(const std::string &command,
                                            CommandReturnObject *,
                                            const ExecuteScriptOptions &) {
   m_interpreter.GetDebugger().GetErrorFile()->PutCString(
       "error: there is no embedded script interpreter in this mode.\n");
   return false;
 }
 
 void ScriptInterpreterNone::ExecuteInterpreterLoop() {

source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h

#endif
 
   ScriptInterpreterPython(CommandInterpreter &interpreter);
 
   ~ScriptInterpreterPython() override;
 
   bool Interrupt() override;
 
   bool ExecuteOneLine(
-      llvm::StringRef command, CommandReturnObject *result,
+      const std::string &command, CommandReturnObject *result,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
 
   void ExecuteInterpreterLoop() override;
 
   bool ExecuteOneLineWithReturn(
-      llvm::StringRef in_string,
+      const std::string &in_string,
       ScriptInterpreter::ScriptReturnType return_type, void *ret_value,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
 
   lldb_private::Status ExecuteMultipleLines(
       const char *in_string,
       const ExecuteScriptOptions &options = ExecuteScriptOptions()) override;
 
   Status

source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

static void ReadThreadBytesReceived(void *baton, const void *src,
   if (src && src_len) {
     Stream *strm = (Stream *)baton;
     strm->Write(src, src_len);
     strm->Flush();
   }
 }
 
 bool ScriptInterpreterPython::ExecuteOneLine(
-    llvm::StringRef command, CommandReturnObject *result,
+    const std::string &command, CommandReturnObject *result,
     const ExecuteScriptOptions &options) {
-  std::string command_str = command.str();
 
   if (!m_valid_session)
     return false;
 
   if (!command.empty()) {
     // We want to call run_one_line, passing in the dictionary and the command
     // string.  We cannot do this through PyRun_SimpleString here because the
     // command string may contain escaped characters, and putting it inside
bool success = false;
 
       // Find the correct script interpreter dictionary in the main module.
       PythonDictionary &session_dict = GetSessionDictionary();
       if (session_dict.IsValid()) {
         if (GetEmbeddedInterpreterModuleObjects()) {
           if (PyCallable_Check(m_run_one_line_function.get())) {
             PythonObject pargs(
                 PyRefType::Owned,
-                Py_BuildValue("(Os)", session_dict.get(), command_str.c_str()));
+                Py_BuildValue("(Os)", session_dict.get(), command.c_str()));
             if (pargs.IsValid()) {
               PythonObject return_value(
                   PyRefType::Owned,
                   PyObject_CallObject(m_run_one_line_function.get(),
                                       pargs.get()));
               if (return_value.IsValid())
                 success = true;
               else if (options.GetMaskoutErrors() && PyErr_Occurred()) {
#endif
     }
 
     if (success)
       return true;
 
     // The one-liner failed.  Append the error message.
     if (result) {
       result->AppendErrorWithFormat(
-          "python failed attempting to evaluate '%s'\n", command_str.c_str());
+          "python failed attempting to evaluate '%s'\n", command.c_str());
     }
     return false;
   }
 
   if (result)
     result->AppendError("empty command passed to python\n");
   return false;
 }
if (IsExecutingPython()) {
     }
   }
   if (log)
     log->Printf("ScriptInterpreterPython::Interrupt() python code not running, "
                 "can't interrupt");
   return false;
 }
 bool ScriptInterpreterPython::ExecuteOneLineWithReturn(
-    llvm::StringRef in_string, ScriptInterpreter::ScriptReturnType return_type,
-    void *ret_value, const ExecuteScriptOptions &options) {
+    const std::string &in_string,
+    ScriptInterpreter::ScriptReturnType return_type, void *ret_value,
+    const ExecuteScriptOptions &options) {
 
   Locker locker(this, ScriptInterpreterPython::Locker::AcquireLock |
                           ScriptInterpreterPython::Locker::InitSession |
                           (options.GetSetLLDBGlobals()
                                ? ScriptInterpreterPython::Locker::InitGlobals
                                : 0) |
                           Locker::NoSTDIN,
                 ScriptInterpreterPython::Locker::FreeAcquiredLock |
bool ScriptInterpreterPython::ExecuteOneLineWithReturn(
 
   if (!locals.IsValid())
     locals = globals;
 
   py_error.Reset(PyRefType::Borrowed, PyErr_Occurred());
   if (py_error.IsValid())
     PyErr_Clear();
 
-  std::string as_string = in_string.str();
   { // scope for PythonInputReaderManager
     // PythonInputReaderManager py_input(options.GetEnableIO() ? this : NULL);
     py_return.Reset(PyRefType::Owned,
-                    PyRun_String(as_string.c_str(), Py_eval_input,
+                    PyRun_String(in_string.c_str(), Py_eval_input,
                                  globals.get(), locals.get()));
     if (!py_return.IsValid()) {
       py_error.Reset(PyRefType::Borrowed, PyErr_Occurred());
       if (py_error.IsValid())
         PyErr_Clear();
 
       py_return.Reset(PyRefType::Owned,
-                      PyRun_String(as_string.c_str(), Py_single_input,
+                      PyRun_String(in_string.c_str(), Py_single_input,
                                    globals.get(), locals.get()));
     }
   }
 
   if (py_return.IsValid()) {
     switch (return_type) {
     case eScriptReturnTypeCharPtr: // "char *"
     {
if (!item || !*item)
     return false;
   std::string command(item);
   command += ".__doc__";
 
   char *result_ptr = nullptr; // Python is going to point this to valid data if
                               // ExecuteOneLineWithReturn returns successfully
 
   if (ExecuteOneLineWithReturn(
-          command.c_str(), ScriptInterpreter::eScriptReturnTypeCharStrOrNone,
+          command, ScriptInterpreter::eScriptReturnTypeCharStrOrNone,
           &result_ptr,
           ScriptInterpreter::ExecuteScriptOptions().SetEnableIO(false))) {
     if (result_ptr)
       dest.assign(result_ptr);
     return true;
   } else {
     StreamString str_stream;
     str_stream.Printf(