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

[Pavel Labath via lldb-commits]

labath 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.

Correct. This is not how IOHandlerProcessSTDIO works right now (but [IOHandlerEditline](https://github.com/llvm/llvm-project/blob/e235fcb582eec5f58c905b66f96d0732d17b875e/lldb/source/Host/common/Editline.cpp#L1660) does). I'm using that as an example of something that would not be possible if one relied solely on the locking mechanism. The curses gui iohandler might be an even better example of that (although I'd much rather see it burn than improve it), as there the output might need to go to some particular curses window in order to avoid completely corrupting the curses window layout.

> 
> > 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.

Thanks for trying it out. You're right, it probably isn't representative, but this test failure probably is a symptom of the problem -- this implementation would not trigger the prompt redrawing code I linked to above.

> @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 sorry about the confusion. I think that's a reflection of my ambiguity. I wouldn't really say I want to take this in a specific direction. I'm more of in the "asking questions" stage, where I'm turning the problem over in order to understand it.

Yesterday, I wasn't sure if we need the async stream mechanism. Today, I think we do (because of the prompt redrawing thingy). I think the two questions (async print and stream protection), but are separate in a way (the iohandlers still need, and the editline iohandler actually uses a mutex), but they are also kind of related in that they have to work together to achieve desired outcome ("nice" console output?) -- which may impact how they're implemented. 

At this point, I think the main question I am trying to answer is about a some guideline/rule about the usage of the various printing mechanism. Imagine I'm writing (or reviewing) a piece of code trying to print something. How do I know which method to use. If I can get a hold of a Debugger, I have the choice between GetOutputStreamSP, GetOutputFile and GetAsyncOutputStream. This patch doesn't really change that (which is why I think you say it's a net benefit), but it might make it seem like using GetOutputStreamSP (after locking it) is safe/okay -- which I think is the part that's bothering me.

So to try to propose an answer to this question: would it be correct to say that (even after this patch, as it is right now), one should approximately always use GetAsyncOutputStream for printing stuff out (at least in cases where one doesn't have the CommandReturnObject around), and that the other methods exist as implementation details of that (supporting locking in iohandlers, which is necessary to produce output correctly), or due legacy code we don't know how to get rid of?

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