[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 2 inline comments as done.
ychen added a comment.

Thanks for the review, Jordan.



================
Comment at: llvm/test/tools/llvm-readobj/check-output-order.test:7-9
+## TODO: Add a --all-debug option for llvm-readobj to turn on literally every
+##       option, then copy the output of `llvm-readobj --all-debug` and paste
+##       it into the CHECK line here.
----------------
rupprecht wrote:
> There is an `--all` option that turns on: `--file-headers, --program-headers, --section-headers, --symbols, --relocations, --dynamic-table, --notes, --version-info, --unwind, --section-groups and --elf-hash-histogram`
> 
> Is that what you want here?
> 
> That said, checking the full content of `--all` in a test may be too verbose. Maybe just check a few lines from each section; enough to make sure they aren't interleaved.
Totally agree that checking for all output in the test file is too much. Comments updated.

`--all ` does not contain every possible piece of information. If we really want to check the order is the same as printing order (for all possible output) in the code for robustness, I think `--all-debug` is still needed.


================
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:
> 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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63115