[PATCH] D63115: [llvm-readobj] Fix output interleaving issue caused by using multiple streams at the same time.

[Yuanfang Chen via Phabricator via llvm-commits]

ychen marked an inline comment as done.
ychen added inline comments.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:391-392
   GNUStyle(ScopedPrinter &W, ELFDumper<ELFT> *Dumper)
-      : DumpStyle<ELFT>(Dumper), OS(W.getOStream()) {}
+      : DumpStyle<ELFT>(Dumper),
+        OS(static_cast<formatted_raw_ostream&>(W.getOStream())) {}
 
----------------
rupprecht wrote:
> ychen wrote:
> > rupprecht wrote:
> > > I don't understand this part of the change. `W.getOStream()` is a `raw_ostream` and can't be casted to a subclass. What this is doing is strange -- I'm not sure how it compiles.
> > > 
> > > I don't think the change to this file is actually necessary, is it?
> > > can't be casted to a subclass.
> > Why not if we know for a fact that the referenced object is indeed that subclass?
> > 
> > > What this is doing is strange -- I'm not sure how it compiles.
> > I could be wrong but I think it is because there is no compiler checking for static_cast.
> > 
> > >I don't think the change to this file is actually necessary, is it?
> > If not, `GNUStyle<>::OS` is another output stream that has different buffering state than the stream referenced by `W.getOStream()` here. (See formatted_raw_ostream::setStream)
> > If not, GNUStyle<>::OS is another output stream
> It looks like `formatted_raw_ostream` just wraps the stream passed in; it doesn't create another stream
> 
> I don't think what's happening is what appears to be happening. It looks like a cast. But I think it's creating a temporary reference that immediately leaves the stack and causes heavy UB here.
> 
> Have you run unit tests with this part of the change? There are tons of crashes for `check-llvm-tools-llvm-readobj` with just this part of the change.
Ran both` check-all` and `check-llvm-tools-llvm-readobj`, did not see any regression ... (based on master b341d305a4cef8e75f520d104cd09b0fcfdc8bba)

> It looks like formatted_raw_ostream just wraps the stream passed in;
Agree. And formatted_raw_ostream is a stream by itself even without wrapping the stream passed in.

> it doesn't create another stream
I'm not sure. ` formatted_raw_ostream` is a subclass of `raw_ostream`. Why is it not another stream?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63115