[Lldb-commits] [lldb] [lldb] Synchronize the debugger's stdout and stderr streams (PR #126630)

[Jonas Devlieghere via lldb-commits]

JDevlieghere wrote:

> That said, not needing to replicate the locking logic in every IOHandler would definitely be nice. However, I'm not certain about the feasibility of that. In a way, I think that the PrintAsync approach is better because it lets the IOHandler know that some async printing is happening, and it can choose how to deal with it. For example, the ProcessIOHandler (forwarding stdout of the debugger proces) can end up in a state where the debugged process has written only a half a line. If async printing is plumbed through it, it will know that is happening, and it can choose to do something about (print an extra newline to delimit process output, or maybe erase the last output line, and then redraw it after the async output -- I don't know).

Are you saying this is how it _could_ work? Looking at `IOHandlerProcessSTDIO`, it doesn't implement `PrintAsync` so it's just using the default implementation which prints to the debugger output stream.
 
> I'm not a mind reader nor a time traveller, but I have a feeling this is the reason why the async functionality is implemented the way it is. Maybe that's something we don't need? I think I could live without it. But if that is the case, I would like to see it dismantled, at least partially. For example, do you think it would be possible to change Debugger::PrintAsync to print the output directly (after locking the stream and such) -- without going through the IOHandlers and stuff?

I ran the test suite with this change:

```
diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp
index 65f0502143bc..7e306fd7dcf4 100644
--- a/lldb/source/Core/Debugger.cpp
+++ b/lldb/source/Core/Debugger.cpp
@@ -1200,13 +1200,10 @@ bool Debugger::CheckTopIOHandlerTypes(IOHandler::Type top_type,
 }
 
 void Debugger::PrintAsync(const char *s, size_t len, bool is_stdout) {
-  bool printed = m_io_handler_stack.PrintAsync(s, len, is_stdout);
-  if (!printed) {
-    LockableStreamFileSP stream_sp =
-        is_stdout ? m_output_stream_sp : m_error_stream_sp;
-    LockedStreamFile locked_stream = stream_sp->Lock();
-    locked_stream.Write(s, len);
-  }
+  LockableStreamFileSP stream_sp =
+      is_stdout ? m_output_stream_sp : m_error_stream_sp;
+  LockedStreamFile locked_stream = stream_sp->Lock();
+  locked_stream.Write(s, len);
 }
 
 llvm::StringRef Debugger::GetTopIOHandlerControlSequence(char ch) {
```

I'm not sure how representative it is to rely on the test suite for something like this, but I got one failure (`TestBreakpointCallbackCommandSource.py`) because we don't redraw the prompt.

@labath After reading your last message I'm not sure where you want to take this. I understand the desire to not have two ways of doing things, but I don't necessarily see how they have to be tied together. I'm convinced that protecting the underlying streams is a net benefit. We were doing that in an ad-hoc way in the IOHandler previously and this moves it up a level. If that means we can improve other things (like the asynchronous IOHandler) then I'm happy to continue chipping away at this, but your earlier message sounds like you're proposing that as an alternative and I'm not sure I fully understand what you have in mind. 


https://github.com/llvm/llvm-project/pull/126630