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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht 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())) {}
 
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63115