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

Sanjoy Das (Work Account) via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 11 15:27:00 PDT 2020


sanjoy.google added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81156





More information about the llvm-commits mailing list