[PATCH] D81156: [Support] Add stream tie function and use it for errs()

Wed Jun 10 01:36:08 PDT 2020

sammccall added a comment.

In D81156#2084209 <https://reviews.llvm.org/D81156#2084209>, @jhenderson wrote:

> In D81156#2084176 <https://reviews.llvm.org/D81156#2084176>, @sammccall wrote:
>
> > This is surprising and unsafe behaviour. Can we reconsider changing the default, at least until a more careful audit of the places errs() is used?
> >  In particular, if errs is used from multiple threads (with a lock) and outs is used from a single thread (so no lock needed) then this introduces racy flushes to outs() concurrent with writes.
>
>
> Thanks for highlighting this, it's been brought up on D81525 <https://reviews.llvm.org/D81525> as well, so rather than discuss it in two places, we'll look at it there, if that's okay?

Happy to discuss there. It's a separate issue though, the discussion there is about initialization being racy,

> As to why this was decided to be the default in the first place: a) it matches the default behaviour of `std::cout`/`std::cerr`, and b) in the typical case of printing an error message of some kind, it ensures we don't interleave error output in the middle of a line of other output, which seems like an almost universally good thing to me. But yet, we missed the thread-safety (or rather lack thereof) aspect of this change.

cout and cerr have been banned in every codebase I've done much IO in, so I had to look this up :-)
However they're *threadsafe*, and so synchronizing there is pretty different.
Per cppreference the specified behavior is also not the same: "synchronized C++ streams are guaranteed to be thread-safe (individual characters output from multiple threads may interleave, but no data races occur)" - this seems to apply to multiple streams too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81156