[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:
> > 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?
> Ran both` check-all` and check-llvm-tools-llvm-readobj, did not see any regression ... (based on master b341d305a4cef8e75f520d104cd09b0fcfdc8bba)
I only get test failures when I have just this file changed. When I apply the whole patch, the test failures go away.
A coworker pointed me to the rules here: http://eel.is/c++draft/expr.static.cast#2

The tl;dr is that this is valid //if// it's always true that `W.getOStream()` here is always a `formatted_raw_ostream`. If it's not, then you get UB (as evidenced by crashes when running unit tests). The rest of this patch updates all the places so that it is `formatted_raw_ostream` everywhere, hence why you don't see any crashes.

So in that sense, this patch is fine, but IMO it's a very fragile contract that we shouldn't rely on -- there's 4 layers of indirection between where `ScopedPrinter` is created (and now initialized w/ `fouts()` instead of `outs()`). Would it be possible, for instance, to instead change `ScopedPrinter` to take a `formatted_raw_ostream` instead of a `raw_ostream`? Or maybe insert calls to `flush()` at appropriate places (whenever switching between GNU vs LLVM style printing)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63115