[lldb/ScriptInterpreter] Extract IO redirection logic and move it out of ScriptInterpreterPython (NFC)

[JDevlieghere]

Summary

This patch takes the IO redirection logic from ScriptInterpreterPython and moves it into the interpreter library so that it can be used by other script interpreters. I've turned it into a RAII object so that we don't have to worry about cleaning up in the calling code.

[JDevlieghere]

Consume the error when the result pointer is null.

[labath]

Sounds like a good idea, just the error handling needs to be done carefully.

lldb/source/Interpreter/ScriptInterpreter.cpp

171

I'm pretty sure this will trigger an assertion about overwriting an unchecked error. One way to handle this would be to wrap the error in an ErrorAsOutParameter object, but I think it's better to replace the constructor with a static factory function returning an Expected<unique_ptr>.

[JDevlieghere]

lldb/source/Interpreter/ScriptInterpreter.cpp

171

That's how I did it at first, but I found this more elegant and compact. Maybe the ErrorAsOutParameter would tip the scales. I'll change it back to a factory.

[JDevlieghere]

This now has a small function change w.r.t. the error message.

[labath]

I think this looks good, though it looks like you have uploaded an partial patch...

lldb/include/lldb/Interpreter/ScriptInterpreter.h

37–38

If that works, I suppose it's fine. But I wouldn't be surprised if this trick backfires on some STL implementations.

I think that making an exception for make_unique on classes with private constructors is fine (we already have a bunch of classes like that)...

lldb/source/Interpreter/ScriptInterpreter.cpp

111–211

Given that none of this fails (though maybe it should), I think it would be cleaner to keep it in the constructor. You can still keep the static factory thing as a wrapper if you want symmetry.

111–211

Similarly, I would find this better as return std::make_unique<ScriptInterpreterIORedirect>(std::move(nullin.get()), std::move(nullout.get())) (with an appropriate constructor. Mainly because then I don't need to worry about what will the destructor of ScriptInterpreterIORedirect do if we return with the object in an partially initialized state.

[labath]

lldb/include/lldb/Interpreter/ScriptInterpreter.h

37–38

BTW, i've just thought of another reason to not do the friend thingy -- making std::make_unique allows _anyone_ to construct this class via std::make_unique, which defeats the purpose of making the constructor private.

OTOH, if we can successfully separate the fallible and infallible parts of the construction, then making the infallible part (constructor) public may not be that dangerous. For example, in this case, making the (FileUP, FileUP) constructor public might be ok.

lldb/include/lldb/Interpreter/ScriptInterpreter.h

 //===-- ScriptInterpreter.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 LLDB_INTERPRETER_SCRIPTINTERPRETER_H
 #define LLDB_INTERPRETER_SCRIPTINTERPRETER_H
 
-#include "lldb/lldb-private.h"
-
 #include "lldb/Breakpoint/BreakpointOptions.h"
+#include "lldb/Core/Communication.h"
 #include "lldb/Core/PluginInterface.h"
 #include "lldb/Core/SearchFilter.h"
+#include "lldb/Core/StreamFile.h"
+#include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Utility/Broadcaster.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StructuredData.h"
-
-#include "lldb/Host/PseudoTerminal.h"
+#include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 class ScriptInterpreterLocker {
 public:
   ScriptInterpreterLocker() = default;
 
   virtual ~ScriptInterpreterLocker() = default;
 
 private:
   ScriptInterpreterLocker(const ScriptInterpreterLocker &) = delete;
   const ScriptInterpreterLocker &
   operator=(const ScriptInterpreterLocker &) = delete;
 };
 
+class ScriptInterpreterIORedirect {
+public:
+  /// Create an IO redirect with /dev/null as input, output and error file.
+  ScriptInterpreterIORedirect(llvm::Error &error);
+
+  /// Create an IO redirect that redirects the output to the command return
+  /// object if set or to the debugger otherwise.
+  ScriptInterpreterIORedirect(Debugger &debugger, CommandReturnObject *result);
+
+  ~ScriptInterpreterIORedirect();
+
+  lldb::FileSP GetInputFile() const { return m_input_file_sp; }
+  lldb::FileSP GetOutputFile() const { return m_output_file_sp->GetFileSP(); }
+  lldb::FileSP GetErrorFile() const { return m_error_file_sp->GetFileSP(); }
+
+  /// Flush our output and error file handles.
+  void Flush();
+
+private:
+  lldb::FileSP m_input_file_sp;
+  lldb::StreamFileSP m_output_file_sp;
+  lldb::StreamFileSP m_error_file_sp;
+  Communication m_communication;
+  bool m_disconnect;
+};
+
 class ScriptInterpreter : public PluginInterface {
 public:
   enum ScriptReturnType {
     eScriptReturnTypeCharPtr,
     eScriptReturnTypeBool,
     eScriptReturnTypeShortInt,
     eScriptReturnTypeShortIntUnsigned,
     eScriptReturnTypeInt,

lldb/source/Interpreter/ScriptInterpreter.cpp

 //===-- ScriptInterpreter.cpp ---------------------------------------------===//
 //
 // 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/Interpreter/ScriptInterpreter.h"
 
+#include <memory>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string>
 
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/ConnectionFileDescriptor.h"
+#include "lldb/Host/Pipe.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StringList.h"
 
 using namespace lldb;
 using namespace lldb_private;
Status ScriptInterpreter::SetBreakpointCommandCallbackFunction(
   }
   return error;
 }
 
 std::unique_ptr<ScriptInterpreterLocker>
 ScriptInterpreter::AcquireInterpreterLock() {
   return std::unique_ptr<ScriptInterpreterLocker>(
       new ScriptInterpreterLocker());
 }
+
+static void ReadThreadBytesReceived(void *baton, const void *src,
+                                    size_t src_len) {
+  if (src && src_len) {
+    Stream *strm = (Stream *)baton;
+    strm->Write(src, src_len);
+    strm->Flush();
+  }
+}
+
+ScriptInterpreterIORedirect::ScriptInterpreterIORedirect(
+    Debugger &debugger, CommandReturnObject *result)
+    : m_communication("lldb.ScriptInterpreterIORedirect.comm"),
+      m_disconnect(false) {
+  if (result) {
+    m_input_file_sp = debugger.GetInputFileSP();
+
+    Pipe pipe;
+    Status pipe_result = pipe.CreateNew(false);
+#if defined(_WIN32)
+    lldb::file_t read_file = pipe.GetReadNativeHandle();
+    pipe.ReleaseReadFileDescriptor();
+    std::unique_ptr<ConnectionGenericFile> conn_up =
+        std::make_unique<ConnectionGenericFile>(read_file, true);
+#else
+    std::unique_ptr<ConnectionFileDescriptor> conn_up =
+        std::make_unique<ConnectionFileDescriptor>(
+            pipe.ReleaseReadFileDescriptor(), true);
+#endif
+
+    if (conn_up->IsConnected()) {
+      m_communication.SetConnection(std::move(conn_up));
+      m_communication.SetReadThreadBytesReceivedCallback(
+          ReadThreadBytesReceived, &result->GetOutputStream());
+      m_communication.StartReadThread();
+      m_disconnect = true;
+
+      FILE *outfile_handle = fdopen(pipe.ReleaseWriteFileDescriptor(), "w");
+      m_output_file_sp = std::make_shared<StreamFile>(outfile_handle, true);
+      m_error_file_sp = m_output_file_sp;
+      if (outfile_handle)
+        ::setbuf(outfile_handle, nullptr);
+
+      result->SetImmediateOutputFile(debugger.GetOutputStream().GetFileSP());
+      result->SetImmediateErrorFile(debugger.GetErrorStream().GetFileSP());
+    }
+  }
+
+  if (!m_input_file_sp || !m_output_file_sp || !m_error_file_sp)
+    debugger.AdoptTopIOHandlerFilesIfInvalid(m_input_file_sp, m_output_file_sp,
+                                             m_error_file_sp);
+}
+
+ScriptInterpreterIORedirect::ScriptInterpreterIORedirect(llvm::Error &error)
+    : m_communication("lldb.ScriptInterpreterIORedirect.comm"),
+      m_disconnect(false) {
+  auto nullin = FileSystem::Instance().Open(FileSpec(FileSystem::DEV_NULL),
+                                            File::eOpenOptionRead);
+  if (!nullin) {
+    error = nullin.takeError();
+    return;
+  }
+
+  auto nullout = FileSystem::Instance().Open(FileSpec(FileSystem::DEV_NULL),
+                                             File::eOpenOptionWrite);
+  if (!nullout) {
+    error = nullout.takeError();
+    return;
+  }
+
+  m_input_file_sp = std::move(nullin.get());
+  m_error_file_sp = m_output_file_sp =
+      std::make_shared<StreamFile>(std::move(nullout.get()));
+}
+
+void ScriptInterpreterIORedirect::Flush() {
+  if (m_output_file_sp)
+    m_output_file_sp->Flush();
+  if (m_error_file_sp)
+    m_error_file_sp->Flush();
+}
+
+ScriptInterpreterIORedirect::~ScriptInterpreterIORedirect() {
+  if (!m_disconnect)
+    return;
+
+  assert(m_output_file_sp);
+  assert(m_error_file_sp);
+  assert(m_output_file_sp == m_error_file_sp);
+
+  // Close the write end of the pipe since we are done with our one line
+  // script. This should cause the read thread that output_comm is using to
+  // exit.
+  m_output_file_sp->GetFile().Close();
+  // The close above should cause this thread to exit when it gets to the end
+  // of file, so let it get all its data.
+  m_communication.JoinReadThread();
+  // Now we can close the read end of the pipe.
+  m_communication.Disconnect();
+}

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp

 #include "lldb/Utility/Timer.h"
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/ConnectionGenericFileWindows.h"
 #endif
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatAdapters.h"
 
 #include <memory>
 #include <mutex>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string>
bool ScriptInterpreterPythonImpl::GetEmbeddedInterpreterModuleObjects() {
 
   m_run_one_line_function =
       module_dict.GetItemForKey(PythonString("run_one_line"));
   m_run_one_line_str_global =
       module_dict.GetItemForKey(PythonString("g_run_one_line_str"));
   return m_run_one_line_function.IsValid();
 }
 
-static void ReadThreadBytesReceived(void *baton, const void *src,
-                                    size_t src_len) {
-  if (src && src_len) {
-    Stream *strm = (Stream *)baton;
-    strm->Write(src, src_len);
-    strm->Flush();
-  }
-}
-
 bool ScriptInterpreterPythonImpl::ExecuteOneLine(
     llvm::StringRef 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
     // another string to pass to PyRun_SimpleString messes up the escaping.  So
     // we use the following more complicated method to pass the command string
     // directly down to Python.
-    Debugger &debugger = m_debugger;
-
-    FileSP input_file_sp;
-    StreamFileSP output_file_sp;
-    StreamFileSP error_file_sp;
-    Communication output_comm(
-        "lldb.ScriptInterpreterPythonImpl.ExecuteOneLine.comm");
-    bool join_read_thread = false;
+    std::unique_ptr<ScriptInterpreterIORedirect> io_redirect;
     if (options.GetEnableIO()) {
-      if (result) {
-        input_file_sp = debugger.GetInputFileSP();
-        // Set output to a temporary file so we can forward the results on to
-        // the result object
-
-        Pipe pipe;
-        Status pipe_result = pipe.CreateNew(false);
-        if (pipe_result.Success()) {
-#if defined(_WIN32)
-          lldb::file_t read_file = pipe.GetReadNativeHandle();
-          pipe.ReleaseReadFileDescriptor();
-          std::unique_ptr<ConnectionGenericFile> conn_up(
-              new ConnectionGenericFile(read_file, true));
-#else
-          std::unique_ptr<ConnectionFileDescriptor> conn_up(
-              new ConnectionFileDescriptor(pipe.ReleaseReadFileDescriptor(),
-                                           true));
-#endif
-          if (conn_up->IsConnected()) {
-            output_comm.SetConnection(std::move(conn_up));
-            output_comm.SetReadThreadBytesReceivedCallback(
-                ReadThreadBytesReceived, &result->GetOutputStream());
-            output_comm.StartReadThread();
-            join_read_thread = true;
-            FILE *outfile_handle =
-                fdopen(pipe.ReleaseWriteFileDescriptor(), "w");
-            output_file_sp = std::make_shared<StreamFile>(outfile_handle, true);
-            error_file_sp = output_file_sp;
-            if (outfile_handle)
-              ::setbuf(outfile_handle, nullptr);
-
-            result->SetImmediateOutputFile(
-                debugger.GetOutputStream().GetFileSP());
-            result->SetImmediateErrorFile(
-                debugger.GetErrorStream().GetFileSP());
-          }
-        }
-      }
-      if (!input_file_sp || !output_file_sp || !error_file_sp)
-        debugger.AdoptTopIOHandlerFilesIfInvalid(input_file_sp, output_file_sp,
-                                                 error_file_sp);
+      io_redirect =
+          std::make_unique<ScriptInterpreterIORedirect>(m_debugger, result);
     } else {
-      auto nullin = FileSystem::Instance().Open(
-                                  FileSpec(FileSystem::DEV_NULL),
-                                  File::eOpenOptionRead);
-      auto nullout = FileSystem::Instance().Open(
-                                  FileSpec(FileSystem::DEV_NULL),
-                                  File::eOpenOptionWrite);
-      if (!nullin) {
-        result->AppendErrorWithFormatv("failed to open /dev/null: {0}\n",
-                                       llvm::fmt_consume(nullin.takeError()));
-        return false;
-      }
-      if (!nullout) {
+      llvm::Error error = llvm::Error::success();
+      io_redirect = std::make_unique<ScriptInterpreterIORedirect>(error);
+      if (error) {
+        if (result)
           result->AppendErrorWithFormatv("failed to open /dev/null: {0}\n",
-                                       llvm::fmt_consume(nullout.takeError()));
+                                         llvm::fmt_consume(std::move(error)));
+        else
+          llvm::consumeError(std::move(error));
         return false;
       }
-      input_file_sp = std::move(nullin.get());
-      error_file_sp = output_file_sp = std::make_shared<StreamFile>(std::move(nullout.get()));
     }
 
     bool success = false;
     {
       // WARNING!  It's imperative that this RAII scope be as tight as
       // possible. In particular, the scope must end *before* we try to join
       // the read thread.  The reason for this is that a pre-requisite for
       // joining the read thread is that we close the write handle (to break
       // the pipe and cause it to wake up and exit).  But acquiring the GIL as
       // below will redirect Python's stdio to use this same handle.  If we
       // close the handle while Python is still using it, bad things will
       // happen.
       Locker locker(
           this,
           Locker::AcquireLock | Locker::InitSession |
               (options.GetSetLLDBGlobals() ? Locker::InitGlobals : 0) |
               ((result && result->GetInteractive()) ? 0 : Locker::NoSTDIN),
-          Locker::FreeAcquiredLock | Locker::TearDownSession, input_file_sp,
-          output_file_sp->GetFileSP(), error_file_sp->GetFileSP());
+          Locker::FreeAcquiredLock | Locker::TearDownSession,
+          io_redirect->GetInputFile(), io_redirect->GetOutputFile(),
+          io_redirect->GetErrorFile());
 
       // 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,
bool success = false;
                 PyErr_Print();
                 PyErr_Clear();
               }
             }
           }
         }
       }
 
-      // Flush our output and error file handles
-      output_file_sp->Flush();
-      error_file_sp->Flush();
-    }
-
-    if (join_read_thread) {
-      // Close the write end of the pipe since we are done with our one line
-      // script. This should cause the read thread that output_comm is using to
-      // exit
-      output_file_sp->GetFile().Close();
-      // The close above should cause this thread to exit when it gets to the
-      // end of file, so let it get all its data
-      output_comm.JoinReadThread();
-      // Now we can close the read end of the pipe
-      output_comm.Disconnect();
+      io_redirect->Flush();
     }
 
     if (success)
       return true;
 
     // The one-liner failed.  Append the error message.
     if (result) {
       result->AppendErrorWithFormat(