[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#2088534 <https://reviews.llvm.org/D81525#2088534>, @MaskRay wrote:

> In D81525#2088463 <https://reviews.llvm.org/D81525#2088463>, @dblaikie wrote:
>
> > In D81525#2088388 <https://reviews.llvm.org/D81525#2088388>, @MaskRay wrote:
> >
> > > In D81525#2088321 <https://reviews.llvm.org/D81525#2088321>, @dblaikie wrote:
> > >
> > > > In D81525#2086997 <https://reviews.llvm.org/D81525#2086997>, @labath wrote:
> > > >
> > > > > In D81525#2085642 <https://reviews.llvm.org/D81525#2085642>, @dblaikie wrote:
> > > > >
> > > > > > In D81525#2084521 <https://reviews.llvm.org/D81525#2084521>, @labath wrote:
> > > > > >
> > > > > > > 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?
> > > > >
> > > > >
> > > > > Affirmative.
> > > > >
> > > > > >> 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.
> > > > >
> > > > > Unfortunately, I don't think there's a good way to provide those guarantees -- this amounts to ensuring that `flush()` does not race with any other stream output operation (including itself). That is probably impossible without locking, which will likely have significant implications on the stream performance.
> > > > >
> > > > > When you say "worthwhile tradeoff", what do you mean exactly? `errs()` and `outs()` being tied by default, or the entire tieing concept in general?
> > > >
> > > >
> > > > Sorry, yeah - I mean tie-by-default. Essentially: Seems like this is isn't a good idea if it means that outs and errs become racy by default. I think that makes this an opt-in situation, where the app has to make the choice/provide the guarantee that it won't be writing to outs and errs from different threads.
> > >
> > >
> > > Calling `errs() << char` was racy before @jhenderson's patch. `raw_fd_ostream::write_impl` modifies a member variable `pos`.
> >
> >
> > Sorry, specifically I was talking about racing between writing to outs() and writing to errs() - as far as I understand it, those did not race prior to the tying (ie: they provided the "usual thread safety guarantees" - which is that mutations of distinct objects do not race) but race once the tying was done (because a write to errs might flush outs mid-write). This is the issue @sammccall raised earlier & sounds like it actively impacts clangd.
>
>
> I don't think it is actively impacting clangd. Mitigated by D81538 <https://reviews.llvm.org/D81538>.


Ah, glad to hear they have a workaround - though still seems problematic that this fairly reasonable use case is broken/requires a workaround.

In D81525#2088546 <https://reviews.llvm.org/D81525#2088546>, @MaskRay wrote:

> In D81525#2088503 <https://reviews.llvm.org/D81525#2088503>, @mehdi_amini wrote:
>
> > In D81525#2088371 <https://reviews.llvm.org/D81525#2088371>, @MaskRay wrote:
> >
> > > > Unless there is an immediate resolution (and the discussion seems to indicate that it is worth taking our time here), can we revert the patch that introduces a TSAN failure?
> > >
> > > @mehdi_amini Pushing this patch will fix the issue.
> >
> >
> > This patch isn't approved, and discussion is going on: the most straightforward way of addressing the bug is to revert *first* to remove the urgency here. It is easy to reapply with your change though when it is ready.
>
>
> There have been several commits touching this area (there are dependent .debug_line commits as well). It is not a "reverting D81156 <https://reviews.llvm.org/D81156> and everything will be fine" state. You mentioned some TSAN failures so I asked what are affected. We need to figure out a good way to address the issue.


But there's open debate about what that is and ongoing breakage in tree - @jhenderson please revert the original patch & continue discussion after that.

> So far I believe pushing this and things will be fixed. (accessing outs()errs() concurrently is still racy - but accessing outs() concurrently & accessing errs() concurrently were already racy - I don't know what projects can actually trigger the code path)

saying "outs() isn't thread safe" isn't super surprising (a bit surprising compared to cout/cerr, admittedly) - but "outs() doesn't provide the usual thread safety guarantees" is a bit more problematic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81525