This is an archive of the discontinued LLVM Phabricator instance.

Fix issues with inferior stdout coming out of order
ClosedPublic

Authored by labath on Jul 23 2019, 8:01 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jul 23 2019, 8:01 AM
labath updated this revision to Diff 211328.Jul 23 2019, 10:27 AM

Remove sleep, left over from testing.

clayborg added inline comments.Jul 23 2019, 12:58 PM
include/lldb/Core/Debugger.h
350 ↗(On Diff #211328)

What about making this function:

void Debugger::FlushProcessOutput(Process &process, bool flush_stdout, bool flush_stderr);

Then we don't need to call this twice everywhere.

source/Core/Debugger.cpp
1416–1421 ↗(On Diff #211328)

If we change FlushProcessOutput as mentioned above this would be:

FlushProcessOutput(Process &process, got_stdout || got_state_changed, got_stderr || got_state_changed);
source/Interpreter/CommandInterpreter.cpp
2712–2713 ↗(On Diff #211328)
m_debugger.FlushProcessOutput(*process_sp, true, true);

I agree with Greg, having one function that can do any of the combinations of stdout & stderr seems more convenient.

Other than that, this is fine.

labath updated this revision to Diff 211439.Jul 24 2019, 1:14 AM

Thanks for the feedback. Updating the patch to avoid calling the flush function twice.

This revision was not accepted when it landed; it landed in state Needs Review.Jul 31 2019, 5:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 5:09 AM