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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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>.

>> I think we can push this patch, and think about tied-by-default state later. (I originally requested an opt-in state in D81156 <https://reviews.llvm.org/D81156>, but did not think hard about the implications)
> 
> Given that clangd is actively broken, I think it's important to go in a direction that fixes that underlying issue sooner rather than later - which I think amounts to "revert the tie-by-default patch".

Not sure any other projects is impacted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81525