Fix TestProcessIO.py when run against a remote target

[vharron]

Summary

Fixed test case to copy redirected stdout/stderr files from remote
target to host

llgs wasn't bothering to put the pty master file handle in the right
place if stdout/stderr were redirected to a file. It is still needed
for stdin.

Fixed some erroneous log messages

[ovyalov]

test/python_api/process/io/TestProcessIO.py

128–129

read_output_file_and_delete and read_error_file_and_delete look pretty identical -
we may have have common function read_file_and_delete(self, local_file) which is used by read_output_file_and_delete as read_file_and_delete(self.local_output_file) and read_error_file_and_delete as read_file_and_delete(self.local_error_file)

[vharron]

refactored common code out of read_output/error_file_and_delete

[ovyalov]

lgtm

[clayborg]

See my inlined code comments.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp

503

Should this be:

if (launch_info.GetFileActionForFD (STDOUT_FILENO) == nullptr &&
    launch_info.GetFileActionForFD (STDERR_FILENO) == nullptr)

525–543

Remove this in favor of just inlining the following code?

if (launch_info.GetFileActionForFD (STDOUT_FILENO) == nullptr &&
    launch_info.GetFileActionForFD (STDERR_FILENO) == nullptr)

thoughts? You could add new functions to ProcessLaunch if you want to make it:

if (!launch_info.HasFileActionFor(STDOUT_FILENO) && !launch_info.HasFileAction(STDERR_FILENO))

Or if you added a var arg version:

if (!launch_info.HasFileActionForAnyOf(STDOUT_FILENO, STDERR_FILENO))

572

Inline accessors to m_process_launch_info as state above?

1897

Get rid of this empty line diff?

[vharron]

Removed functions for checking stdio over gdb-remote

[vharron]

Single line diff left intentionally to separate functions by a blank line (convention/standard)

[clayborg]

Looks good

[vharron]

rebased against ToT

[vharron]

Committed revision 229141.

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.h

private:
     GetNextSavedRegistersID ();
 
     void
     MaybeCloseInferiorTerminalConnection ();
 
     void
     ClearProcessSpecificData ();
 
+    // some stdio may be sent over gdb remote
     bool
-    ShouldRedirectInferiorOutputOverGdbRemote (const lldb_private::ProcessLaunchInfo &launch_info) const;
+    InferiorStdioOverGdbRemote (const lldb_private::ProcessLaunchInfo &launch_info) const;
+
+    // some output may be sent over gdb remote
+    bool
+    InferiorStdoutOrStderrOverGdbRemote (const lldb_private::ProcessLaunchInfo &launch_info) const;
 
     //------------------------------------------------------------------
     // For GDBRemoteCommunicationServer only
     //------------------------------------------------------------------
     DISALLOW_COPY_AND_ASSIGN (GDBRemoteCommunicationServer);
 };
 
 #endif  // liblldb_GDBRemoteCommunicationServer_h_

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServer.cpp

GDBRemoteCommunicationServer::LaunchProcess ()
     // derived classes, one for lldb-platform, the other for lldb-gdbserver.
     if (IsGdbServer ())
         return LaunchProcessForDebugging ();
     else
         return LaunchPlatformProcess ();
 }
 
 bool
-GDBRemoteCommunicationServer::ShouldRedirectInferiorOutputOverGdbRemote (const lldb_private::ProcessLaunchInfo &launch_info) const
+GDBRemoteCommunicationServer::InferiorStdioOverGdbRemote (const lldb_private::ProcessLaunchInfo &launch_info) const
+{
+    if (InferiorStdoutOrStderrOverGdbRemote(launch_info))
+    {
+        // At least some stdio is going over gdb remote
+        return true;
+    }
+
+    // else, check stdin
+    auto stdin_file_action = launch_info.GetFileActionForFD (STDIN_FILENO);
+
+    // If neither stdout and stderr file actions are specified, we're not doing anything special, so
+    // assume we want to redirect stdout/stderr over gdb-remote $O messages.
+    if (stdin_file_action == nullptr)
+    {
+        // stdin may arrive over gdb-remote protocol.
+        return true;
+    }
+
+    // no stdio over gdb remote
+    // it's either disabled, or redirected to a file
+    return false;
+}
+
+bool
+GDBRemoteCommunicationServer::InferiorStdoutOrStderrOverGdbRemote (const lldb_private::ProcessLaunchInfo &launch_info) const
 {
     // Retrieve the file actions specified for stdout and stderr.
     auto stdout_file_action = launch_info.GetFileActionForFD (STDOUT_FILENO);
     auto stderr_file_action = launch_info.GetFileActionForFD (STDERR_FILENO);
 
     // If neither stdout and stderr file actions are specified, we're not doing anything special, so
     // assume we want to redirect stdout/stderr over gdb-remote $O messages.
-    if ((stdout_file_action == nullptr) && (stderr_file_action == nullptr))
+    if (stdout_file_action == nullptr || stderr_file_action == nullptr)
     {
         // Send stdout/stderr over the gdb-remote protocol.
         return true;
     }
 
-    // Any other setting for either stdout or stderr implies we are either suppressing
-    // it (with /dev/null) or we've got it set to a PTY.  Either way, we don't want the
-    // output over gdb-remote.
+    // no stdout or stderr over gdb remote
+    // it's either disabled, or redirected to a file
     return false;
 }
 
 lldb_private::Error
 GDBRemoteCommunicationServer::LaunchProcessForDebugging ()
 {
     Log *log (GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS));
 
     if (!m_process_launch_info.GetArguments ().GetArgumentCount ())
         return lldb_private::Error ("%s: no process command line specified to launch", __FUNCTION__);
GDBRemoteCommunicationServer::LaunchProcessForDebugging ()
     {
         fprintf (stderr, "%s: failed to launch executable %s", __FUNCTION__, m_process_launch_info.GetArguments ().GetArgumentAtIndex (0));
         return error;
     }
 
     // Handle mirroring of inferior stdout/stderr over the gdb-remote protocol as needed.
     // llgs local-process debugging may specify PTYs, which will eliminate the need to reflect inferior
     // stdout/stderr over the gdb-remote protocol.
-    if (ShouldRedirectInferiorOutputOverGdbRemote (m_process_launch_info))
+    if (InferiorStdioOverGdbRemote (m_process_launch_info))
     {
         if (log)
             log->Printf ("GDBRemoteCommunicationServer::%s pid %" PRIu64 " setting up stdout/stderr redirection via $O gdb-remote commands", __FUNCTION__, m_debugged_process_sp->GetID ());
 
         // Setup stdout/stderr mapping from inferior to $O
         auto terminal_fd = m_debugged_process_sp->GetTerminalFileDescriptor ();
         if (terminal_fd >= 0)
         {
GDBRemoteCommunicationServer::SetSTDIOFileDescriptor (int fd)
 
     m_stdio_communication.SetConnection (conn_up.release());
     if (!m_stdio_communication.IsConnected ())
     {
         error.SetErrorString ("failed to set connection for inferior I/O communication");
         return error;
     }
 
+    if (InferiorStdoutOrStderrOverGdbRemote(m_process_launch_info))
+    {
         m_stdio_communication.SetReadThreadBytesReceivedCallback (STDIOReadThreadBytesReceived, this);
         m_stdio_communication.StartReadThread();
+    }
 
     return error;
 }
 
 void
 GDBRemoteCommunicationServer::STDIOReadThreadBytesReceived (void *baton, const void *src, size_t src_len)
 {
     GDBRemoteCommunicationServer *server = reinterpret_cast<GDBRemoteCommunicationServer*> (baton);

 
 bool
 GDBRemoteCommunicationServer::DebugserverProcessReaped (lldb::pid_t pid)
 {
     Mutex::Locker locker (m_spawned_pids_mutex);
     FreePortForProcess(pid);
     return m_spawned_pids.erase(pid) > 0;
 }
+
 bool
 GDBRemoteCommunicationServer::ReapDebugserverProcess (void *callback_baton,
                                                       lldb::pid_t pid,
                                                       bool exited,
                                                       int signal,    // Zero for no signal
                                                       int status)    // Exit value of process if signal is zero
 {
     GDBRemoteCommunicationServer *server = (GDBRemoteCommunicationServer *)callback_baton;

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (object_file)
 
                 if (stdout_path == NULL)
                     stdout_path = slave_name;
 
                 if (stderr_path == NULL)
                     stderr_path = slave_name;
 
                 if (log)
-                    log->Printf ("ProcessGDBRemote::%s adjusted STDIO paths for local platform (IsHost() is true) using slave: stdin=%s, stdout=%s, stdout=%s",
+                    log->Printf ("ProcessGDBRemote::%s adjusted STDIO paths for local platform (IsHost() is true) using slave: stdin=%s, stdout=%s, stderr=%s",
                                  __FUNCTION__,
                                  stdin_path ? stdin_path : "<null>",
                                  stdout_path ? stdout_path : "<null>",
                                  stderr_path ? stderr_path : "<null>");
             }
 
             if (log)
-                log->Printf ("ProcessGDBRemote::%s final STDIO paths after all adjustments: stdin=%s, stdout=%s, stdout=%s",
+                log->Printf ("ProcessGDBRemote::%s final STDIO paths after all adjustments: stdin=%s, stdout=%s, stderr=%s",
                              __FUNCTION__,
                              stdin_path ? stdin_path : "<null>",
                              stdout_path ? stdout_path : "<null>",
                              stderr_path ? stderr_path : "<null>");
 
             if (stdin_path)
                 m_gdb_comm.SetSTDIN (stdin_path);
             if (stdout_path)

test/python_api/process/io/TestProcessIO.py

def setUp(self):
         self.local_input_file  = os.path.join(os.getcwd(), "input.txt")
         self.local_output_file = os.path.join(os.getcwd(), "output.txt")
         self.local_error_file  = os.path.join(os.getcwd(), "error.txt")
 
         self.input_file  = os.path.join(self.get_process_working_directory(), "input.txt")
         self.output_file = os.path.join(self.get_process_working_directory(), "output.txt")
         self.error_file  = os.path.join(self.get_process_working_directory(), "error.txt")
         self.lines = ["Line 1", "Line 2", "Line 3"]
-    
-    def read_output_file_and_delete (self):
-        self.assertTrue(os.path.exists(self.output_file), "Make sure output.txt file exists")
-        f = open(self.output_file, 'r')
+    # target_file - path on local file system or remote file system if running remote
+    # local_file - path on local system
+    def read_file_and_delete(self, target_file, local_file):
+        if lldb.remote_platform:
+            self.runCmd('platform get-file "{remote}" "{local}"'.format(
+                remote=target_file, local=local_file))
+
+        self.assertTrue(os.path.exists(local_file), 'Make sure "{local}" file exists'.format(local=local_file))
+        f = open(local_file, 'r')
         contents = f.read()
         f.close()
-        os.unlink(self.output_file)
+
+        #TODO: add 'platform delete-file' file command
+        #if lldb.remote_platform:
+        #    self.runCmd('platform delete-file "{remote}"'.format(remote=target_file))
+        os.unlink(local_file)
         return contents
 
+    def read_output_file_and_delete(self):
+        return self.read_file_and_delete(self.output_file, self.local_output_file)
+
     def read_error_file_and_delete(self):
-        self.assertTrue(os.path.exists(self.error_file), "Make sure error.txt file exists")
-        f = open(self.error_file, 'r')
-        contents = f.read()
-        f.close()
-        os.unlink(self.error_file)
-        return contents
+        return self.read_file_and_delete(self.error_file, self.local_error_file)
 
     def create_target(self):
         '''Create the target and launch info that will be used by all tests'''
         self.target = self.dbg.CreateTarget(self.exe)        
         self.launch_info = lldb.SBLaunchInfo([self.exe])
         self.launch_info.SetWorkingDirectory(self.get_process_working_directory())
     
     def redirect_stdin(self):
def redirect_stdin(self):
         if lldb.remote_platform:
             self.runCmd('platform put-file "{local}" "{remote}"'.format(
                 local=self.local_input_file, remote=self.input_file))
 
         # This is the function to remove the custom formats in order to have a
         # clean slate for the next test case.
         def cleanup():
             os.unlink(self.local_input_file)
-            # if lldb.remote_platform:
-                # TODO: add delete file command
-                # self.runCmd('platform delete-file "{local}" "{remote}"'.format(
-                #    local=self.local_input_file, remote=self.input_file))'''
+            #TODO: add 'platform delete-file' file command
+            #if lldb.remote_platform:
+            #    self.runCmd('platform delete-file "{remote}"'.format(remote=self.input_file))
 
         # Execute the cleanup function during test case tear down.
         self.addTearDownHook(cleanup)
         self.launch_info.AddOpenFileAction(0, self.input_file, True, False);
         
     def redirect_stdout(self):
         '''Redirect STDOUT (file descriptor 1) to use our output.txt file'''
         self.launch_info.AddOpenFileAction(1, self.output_file, False, True);