From a9d58436af83eeeca376a0686bb13fc43d3f8ece Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Wed, 31 Jul 2019 12:06:50 +0000 Subject: [PATCH] Fix issues with inferior stdout coming out of order Summary: We've had a bug where two pieces of code, executing on two threads were attempting to write inferior output simultaneously. The first one was in Debugger::HandleProcessEvent, which handled the cases where stdout was coming while the process was running. The second was in CommandInterpreter::IOHandlerInputComplete, which was ensuring that any output is printed before the command which caused process to run terminates. Both of these things make sense, but the fact they were implemented as two independent functions without any synchronization meant that race conditions could occur (e.g. both threads call process->GetSTDOUT, get two chunks of data, but then end up calling stream->Write in opposite order). This was most apparent in situations where a process quickly writes a bunch of output and then exits (as all our register tests do). This patch adds a mutex to ensure that stdout forwarding happens atomically. It also refactors a code somewhat in order to reduce code duplication. Reviewers: clayborg, jingham Subscribers: jfb, mgorny, lldb-commits Differential Revision: https://reviews.llvm.org/D65152 llvm-svn: 367418 --- lldb/include/lldb/Core/Debugger.h | 7 +- lldb/include/lldb/Interpreter/CommandInterpreter.h | 2 +- lldb/source/Core/Debugger.cpp | 83 ++++++---------------- lldb/source/Interpreter/CommandInterpreter.cpp | 32 ++------- 4 files changed, 32 insertions(+), 92 deletions(-) diff --git a/lldb/include/lldb/Core/Debugger.h b/lldb/include/lldb/Core/Debugger.h index 8e60871..8e857e0 100644 --- a/lldb/include/lldb/Core/Debugger.h +++ b/lldb/include/lldb/Core/Debugger.h @@ -345,9 +345,10 @@ protected: void HandleThreadEvent(const lldb::EventSP &event_sp); - size_t GetProcessSTDOUT(Process *process, Stream *stream); - - size_t GetProcessSTDERR(Process *process, Stream *stream); + // Ensures two threads don't attempt to flush process output in parallel. + std::mutex m_output_flush_mutex; + void FlushProcessOutput(Process &process, bool flush_stdout, + bool flush_stderr); SourceManager::SourceFileCache &GetSourceFileCache() { return m_source_file_cache; diff --git a/lldb/include/lldb/Interpreter/CommandInterpreter.h b/lldb/include/lldb/Interpreter/CommandInterpreter.h index d738bbc..3d704d1 100644 --- a/lldb/include/lldb/Interpreter/CommandInterpreter.h +++ b/lldb/include/lldb/Interpreter/CommandInterpreter.h @@ -518,7 +518,7 @@ protected: bool IOHandlerInterrupt(IOHandler &io_handler) override; - size_t GetProcessOutput(); + void GetProcessOutput(); void SetSynchronous(bool value); diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index ca19ad6..5efe786 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -1252,60 +1252,23 @@ void Debugger::HandleBreakpointEvent(const EventSP &event_sp) { // } } -size_t Debugger::GetProcessSTDOUT(Process *process, Stream *stream) { - size_t total_bytes = 0; - if (stream == nullptr) - stream = GetOutputFile().get(); - - if (stream) { - // The process has stuff waiting for stdout; get it and write it out to the - // appropriate place. - if (process == nullptr) { - TargetSP target_sp = GetTargetList().GetSelectedTarget(); - if (target_sp) - process = target_sp->GetProcessSP().get(); - } - if (process) { - Status error; - size_t len; - char stdio_buffer[1024]; - while ((len = process->GetSTDOUT(stdio_buffer, sizeof(stdio_buffer), - error)) > 0) { - stream->Write(stdio_buffer, len); - total_bytes += len; - } - } - stream->Flush(); - } - return total_bytes; -} - -size_t Debugger::GetProcessSTDERR(Process *process, Stream *stream) { - size_t total_bytes = 0; - if (stream == nullptr) - stream = GetOutputFile().get(); - - if (stream) { - // The process has stuff waiting for stderr; get it and write it out to the - // appropriate place. - if (process == nullptr) { - TargetSP target_sp = GetTargetList().GetSelectedTarget(); - if (target_sp) - process = target_sp->GetProcessSP().get(); - } - if (process) { - Status error; - size_t len; - char stdio_buffer[1024]; - while ((len = process->GetSTDERR(stdio_buffer, sizeof(stdio_buffer), - error)) > 0) { - stream->Write(stdio_buffer, len); - total_bytes += len; - } - } - stream->Flush(); - } - return total_bytes; +void Debugger::FlushProcessOutput(Process &process, bool flush_stdout, + bool flush_stderr) { + const auto &flush = [&](Stream &stream, + size_t (Process::*get)(char *, size_t, Status &)) { + Status error; + size_t len; + char buffer[1024]; + while ((len = (process.*get)(buffer, sizeof(buffer), error)) > 0) + stream.Write(buffer, len); + stream.Flush(); + }; + + std::lock_guard guard(m_output_flush_mutex); + if (flush_stdout) + flush(*GetAsyncOutputStream(), &Process::GetSTDOUT); + if (flush_stderr) + flush(*GetAsyncErrorStream(), &Process::GetSTDERR); } // This function handles events that were broadcast by the process. @@ -1345,15 +1308,9 @@ void Debugger::HandleProcessEvent(const EventSP &event_sp) { pop_process_io_handler); } - // Now display and STDOUT - if (got_stdout || got_state_changed) { - GetProcessSTDOUT(process_sp.get(), output_stream_sp.get()); - } - - // Now display and STDERR - if (got_stderr || got_state_changed) { - GetProcessSTDERR(process_sp.get(), error_stream_sp.get()); - } + // Now display STDOUT and STDERR + FlushProcessOutput(*process_sp, got_stdout || got_state_changed, + got_stderr || got_state_changed); // Give structured data events an opportunity to display. if (got_structured_data) { diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index bd09f89..8af8cc42 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -2664,32 +2664,14 @@ void CommandInterpreter::UpdateExecutionContext( } } -size_t CommandInterpreter::GetProcessOutput() { - // The process has stuff waiting for stderr; get it and write it out to the - // appropriate place. - char stdio_buffer[1024]; - size_t len; - size_t total_bytes = 0; - Status error; +void CommandInterpreter::GetProcessOutput() { TargetSP target_sp(m_debugger.GetTargetList().GetSelectedTarget()); - if (target_sp) { - ProcessSP process_sp(target_sp->GetProcessSP()); - if (process_sp) { - while ((len = process_sp->GetSTDOUT(stdio_buffer, sizeof(stdio_buffer), - error)) > 0) { - size_t bytes_written = len; - m_debugger.GetOutputFile()->Write(stdio_buffer, bytes_written); - total_bytes += len; - } - while ((len = process_sp->GetSTDERR(stdio_buffer, sizeof(stdio_buffer), - error)) > 0) { - size_t bytes_written = len; - m_debugger.GetErrorFile()->Write(stdio_buffer, bytes_written); - total_bytes += len; - } - } - } - return total_bytes; + if (!target_sp) + return; + + if (ProcessSP process_sp = target_sp->GetProcessSP()) + m_debugger.FlushProcessOutput(*process_sp, /*flush_stdout*/ true, + /*flush_stderr*/ true); } void CommandInterpreter::StartHandlingCommand() { -- 2.7.4