[PATCH] D81525: [Support] Ensure errs() is constructed after outs() and don't rerun tie when errs() is called

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D81525#2084521 <https://reviews.llvm.org/D81525#2084521>, @labath wrote:

> In D81525#2084291 <https://reviews.llvm.org/D81525#2084291>, @sammccall wrote:
>
> > There's a second threadsafety issue I'd like to highlight, around runtime rather than initialization.
> >
> > raw_ostreams are not threadsafe, including outs(). It's not necessarily safe to flush outs when errs() is accessed.
> >  For example, in clangd errs() is the log stream, written by multiple threads with a lock. outs() is the protocol stream, written by the main thread without a lock.
> >  After this change, all logging threads are trying to flush outs, racing with the main thread which is writing to it.
> >  I can fix this in clangd but it has me questioning whether this is unsafe for other existing users and likely to go unnoticed for new ones.
>


+1

>> Is it really vital that this behaviour be the default? I mentioned on D81156 <https://reviews.llvm.org/D81156> that this seemed surprising and unsafe.
>>  std::{cout,cerr} do something vaguely related (they're threadsafe and synchronize between operations), but that seems a) much safer than this and b) mostly a concession to history maybe?
> 
> I think it's important to note that
>  a) `std::cout/cerr` are guarenteed to be  threadsafe only in `std::ios_base::sync_with_stdio(true)` mode https://en.cppreference.com/w/cpp/io/cerr. What `sync_with_stdio(true)` essentially does is put the standard character streams into the "unbuffered" mode.
>  b) raw_fd_ostream is threadsafe if it is unbuffered. I think we may even have some code relying on the fact that it can be used from a signal handler (with some care).
> 
> However, this doesn't really mean that the two streams are doing the same thing as there are (at least) two important differences:
>  i) `std` streams are thread-safe by default -- one has to call `sync_with_stdio(false)` explicitly, and I don't think many people do. `outs()` is not thread safe by default.

When you say "not thread safe" you mean specifically for using outs from more than one thread at the same time, yes?

> The tieing patch makes `errs()` also not thread safe by default (and makes its thread-safety dependent on what the `outs()` stream does.

Yeah, I think /this/ is perhaps unacceptable. Making writing to an error stream now a race with writing to the output stream seems a bit too subtle to me.

> ii) although the `std` streams are unbuffered in the `sync_with_stdio(true)`, it does not mean that they are writing to the OS directly. there's another layer of buffering in the C library, which means the performance impact of this is not as huge. an unbuffered raw_fd_ostream writes to the OS directly, and the penalty for using it is much higher.
> 
> Anyway, I don't think all this invalidates the original argument, but it is something to keep in mind.
> 
> In D81525#2084434 <https://reviews.llvm.org/D81525#2084434>, @sammccall wrote:
> 
>> In D81525#2084348 <https://reviews.llvm.org/D81525#2084348>, @jhenderson wrote:
>>
>> > That being said, is there an issue even if we change the default that somebody could manually tie `errs()` to `outs()`? The user has to make sure the initialisation order is either amenable, or that they have untied things before destruction occurs.
>>
>>
>> Yup, in fact IIUC C++17 guarantees that `errs().tie(&outs())` executes in the "wrong" order.
>>
>> Is it important to support initializing errs but not outs? The static initializer for errs could initialize outs too, in the right order. This "only" breaks silly people who tie the other way.
>>
>> In any case this is subtle enough (with user-provided streams) that tie() should probably document it.
> 
> 
> We could ensure that `outs()` is constructed before `errs()` even if they are not tied by default, just in case someone decides to tie them.
> 
> I think it would be nice to have them be tied by default, though it's true that there is a fair amount of potential for breaking existing use cases

If they can still at least provide "The usual thread safety guarantees" (writing to (apparently) independent objects (errs() and outs()) does not race) then I'm not significantly opposed. If this breaks that guarantee - I don't think that's a sufficiently worthwhile tradeoff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81525/new/

https://reviews.llvm.org/D81525