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

[Pavel Labath via lldb-commits]

labath wrote:

> > How would that work? We'd check the value of the passed-in `FILE*` and choose a mutex based on whether it happens to be `stdout` ? I don't think I'd like that for several reasons:
> > 
> > * it doesn't handle the case where `stdout/err` is redirected externally (it still locks even though it shouldn't)
> > * it's doesn't let you create a terminal-like experience when embedding lldb-as-a-library (it doesn't lock even though it should)
> 
> What I had in mind was much simpler than that: it would only apply when the debugger is the one creating the streams in its constructor. I was specifically referring to:
> 
> ```
> Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
>     : UserID(g_unique_id++),
>       Properties(std::make_shared<OptionValueProperties>()),
>       m_input_file_sp(std::make_shared<NativeFile>(stdin, false)),
>       m_output_stream_sp(std::make_shared<StreamFile>(stdout, false)),
>       m_error_stream_sp(std::make_shared<StreamFile>(stderr, false)),
> ```
> 
> While not doing this when someone calls `SetOutputFile` or `SetErrorFile`.

I see. Well.. that's slightly cleaner, but it's currently a no-op because even our own driver calls [SetOutputFileHandle](https://github.com/llvm/llvm-project/blob/1199bbb396fb9554401ad5ae1816b6648bab76a9/lldb/tools/driver/Driver.cpp#L442). I guess we could change that, but I also don't particularly like the inaccessibility of this feature.

> 
> > > Conceptually I'd also lean towards (2) but I worry about losing the benefits the current approach brings. Deciding whether "the output is particularly important" is a judgement call that we can't enforce at the API level. I like how the current implementation makes it easy to do the right thing and hard to do the wrong thing (or at least make you think about locking). If the default is to not lock the stdout (the alternative of always locking is equivalent to (1)) then I'm skeptical that we won't just end up with the same outcome that we have with two mutexes.
> > 
> > 
> > But with two mutexes you still don't get stdout<=>stderr synchronization, which means that stderr output can corrupt things you're writing to stdout. What the stderr mutex buys you is stderr<=>stderr synchronization, which is... maybe nice, but maybe also not necessary?
> 
> Yes, I think we're saying the same thing here.

I guess so, but I think we're drawing different conclusions. My take on this is that if the only thing we get from locking stderr is the ability to mutually exclude two stderr accesses, then maybe we don't need to bother with locking stderr at all.

> > FWIW, there is some precedent for unconditional stdout-stderr interaction: writes to `std::cerr` will automatically flush `std::cout` regardless of whether the two streams refer to the same terminal. And the `iostream` library allows you to connect arbitrary two streams in this way. So, I think we _could_ say we "tie" debuggers output and error streams in this way, even if they are completely independent, because they are "our" stdout and stderr.
> 
> Fair enough. What I'm hearing is that you think it's important to synchronize the two somehow and I'd rather do it consistently rather than doing it only sometimes which makes it hard to know what the expectations both for us as the people writing that code and for our users.

Yes, I think that tying them *always* is better than tying them *sometimes*. However, I don't really have a clear preference between the other options flying around. The options being:
- tying them always
- not tying them at all
- not tying them and not locking stderr

If it sounds like I'm in favor of one of them, it's because I'm comparing its (dis)advantages relative to some other approach. However, I think all of these approaches have their (dis)advantages.

> It should be fairly straightforward to support tying the two streams together. I'll see how that goes and update the PR.

A random idea, I don't know if it's a good one: have one object (LockableStreamPair?) that holds the two streams and their mutex. One less argument to pass around?

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