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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D81156#2088666 <https://reviews.llvm.org/D81156#2088666>, @sanjoy.google wrote:

> In D81156#2088659 <https://reviews.llvm.org/D81156#2088659>, @MaskRay wrote:
>
> > In D81156#2088609 <https://reviews.llvm.org/D81156#2088609>, @sanjoy.google wrote:
> >
> > > In D81156#2084324 <https://reviews.llvm.org/D81156#2084324>, @jhenderson wrote:
> > >
> > > > In D81156#2084273 <https://reviews.llvm.org/D81156#2084273>, @sammccall wrote:
> > > >
> > > > > 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,
> > > >
> > > >
> > > > Yeah, I realised that after posting that previous post, sorry! Still, probably best to keep the discussion in one place for now.
> > >
> > >
> > > Did we resolve this?  In an internal XLA build tsan complains about this being racy, and I'm not sure if the problem is with how we are using `llvm::dbgs()` or with this patch.  If the latter, please rollback while we figure out a non-racy way to achieve the same thing.
> >
> >
> > Does 030897523d43e3296f69d25a71a140d9e5793c6a <https://reviews.llvm.org/rG030897523d43e3296f69d25a71a140d9e5793c6a> fix the race?
>
>
> That should fix it, thanks!
>
> > (clangd used tie now. .debug_line parsing depends on it. Reverting is more disruptive than simply disabling tie-by-default behavior)


Note, printing via `dbgs() << "a"` or `errs() << "a"` concurrently was racy even (in both NDEBUG and !NDEBUG builds) before this patch.

This patch introduced additional problems that (1) calling `errs()` concurrently would be racy (due to `S.tie(&outs())`) (2) printing to errs() in one thread and printing to outs() in another thread would be racy.

Anyway, you may want to investigate whether there are existing problems due to concurrent dbgs() or errs() uses, even if TSAN did not catch it before this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81156