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

[ychen]

Summary

Use llvm::fouts() as the default stream for outputing. No new stream
should be constructed to output at the same time.

https://bugs.llvm.org/show_bug.cgi?id=42140

[MaskRay]

The code change looks good. Thanks!

llvm/test/tools/llvm-readobj/fix-interleaved-output.test

4

You may add a comment (starting with ## ) explaining what the test intends to check.

Do you have a better name than fix-interleaved-output.test?

[jhenderson]

llvm/tools/llvm-readobj/llvm-readobj.cpp

469

I thought the idea of the change was to avoid the need for the explicit flush?

[ychen]

• update

[ychen]

• update

[ychen]

In D63115#1537527, @MaskRay wrote:

The code change looks good. Thanks!

Thank you for the review. Test file name updated to reflect the comment in it.

llvm/tools/llvm-readobj/llvm-readobj.cpp

469

You are correct. It helps me identify the root cause: opts::CodeViewMergedTypes also uses its own stream.

[rupprecht]

llvm/test/tools/llvm-readobj/check-output-order.test

7–9 ↗

(On Diff #204096)

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.

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

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?

[ychen]

• update comment

[ychen]

Thanks for the review, Jordan.

llvm/test/tools/llvm-readobj/check-output-order.test

7–9 ↗

(On Diff #204096)

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.

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

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)

[rupprecht]

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

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.

[ychen]

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

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?

[rupprecht]

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

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)?

[ychen]

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

The contract could be checked by adding assert(&W.getOStream() == &llvm::fouts()).

Relying on flush() and having more than one stream outputting sounds more fragile to me. It needs tracking more locations in the source code.

change ScopedPrinter to take a formatted_raw_ostream

This could serve the purpose here. But ScopedPrinter should not care about if the stream is formatted or not.

[ychen]

• add assertion

[jhenderson]

By the way, since you are touching COFF and Mach-O code, your test should probably be run on a file with each of those formats too.

llvm/test/tools/llvm-readobj/check-output-order.test

7–9 ↗

(On Diff #204096)

--all can be combined with other options manually, if we are concerned with getting everything. I'm not sure how worth it it is to check every combination, but I certainly won't argue against somebody doing the work if they feel there's value.

When it comes to checking, you can just check the last line or two of the previous block, followed by the first couple of lines of the next block. If they are interleaved, one or other of the checks will fail.

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

The OS here should definitely be a reference, not a copy since the buffering for streams is local to the instance, and not attached to the main stream itself that everything wraps.

Is there anything stopping GNUStyle having a raw_ostream & and letting virtual function handling do the work of figuring out the appropriate thing to call? That would solve all these issues.

[ychen]

llvm/test/tools/llvm-readobj/check-output-order.test

7–9 ↗

(On Diff #204096)

--all can be combined with other options manually, if we are concerned with getting everything. I'm not sure how worth it it is to check every combination, but I certainly won't argue against somebody doing the work if they feel there's value.

I'm not entirely sure either. Should we keep the TODO here or you prefer not to have it?

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

GNUStyle uses formatted_raw_ostream::PadToColumn a lot, which looks like ScopedPrinter::startLine (both for indentation). I think we could get away with not using llvm::fouts(), and make GNUStyle use ScopedPrinter like LLVMStype do, but it does not seem to be a trivial change.

[ychen]

In D63115#1539481, @jhenderson wrote:

By the way, since you are touching COFF and Mach-O code, your test should probably be run on a file with each of those formats too.

The change in COFF and Mach-o dumper is covered by

tools/llvm-readobj/coff-needed-libs.test (COFFDumper.cpp)
tools/llvm-readobj/macho-needed-libs.test (MachoDumper.cpp)

[jhenderson]

In D63115#1540134, @ychen wrote:

In D63115#1539481, @jhenderson wrote:

By the way, since you are touching COFF and Mach-O code, your test should probably be run on a file with each of those formats too.

The change in COFF and Mach-o dumper is covered by

tools/llvm-readobj/coff-needed-libs.test (COFFDumper.cpp)
tools/llvm-readobj/macho-needed-libs.test (MachoDumper.cpp)

Oops, I was viewing via the "/new/" page. Sorry.

llvm/test/tools/llvm-readobj/check-output-order.test

7–9 ↗

(On Diff #204096)

I don't think we need the TODO, but I'm okay with the idea of using --all in this test.

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

@rupprecht, what do you think?

[ychen]

• update test

[rupprecht]

llvm/tools/llvm-readobj/ELFDumper.cpp

391–393

I'm understanding the issue a little better now... I think there's probably a better way, but for now having this assert seems good enough to get this checked in (and unblock the other patch that depends on this).

[ychen]

Thanks a lot for the review, Jordan. Do you mind commit this for me?

[rupprecht]

Sure, committed as r363198

[ychen]

Thanks!

llvm/test/tools/llvm-readobj/fix-interleaved-output.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --needed-libs --dynamic-table %t | FileCheck %s
+
+# CHECK:      Dynamic section at offset 0x1b8 contains 1 entries:
+# CHECK-NEXT:   Tag                Type                 Name/Value
+# CHECK-NEXT:   0x0000000000000000 (NULL)               0x0
+# CHECK-NEXT: NeededLibraries [
+# CHECK-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Entries:
+      - Tag: DT_NULL
+        Value: 0
+ProgramHeaders:
+  - Type: PT_DYNAMIC
+    Sections:
+      - Section: .dynamic

llvm/tools/llvm-readobj/COFFDumper.cpp

for (const ImportDirectoryEntryRef &DirRef : Obj->import_directories()) {
     StringRef Name;
     if (!DirRef.getName(Name))
       Libs.push_back(Name);
   }
 
   llvm::stable_sort(Libs);
 
   for (const auto &L : Libs) {
-    outs() << "  " << L << "\n";
+    W.startLine() << L << "\n";
   }
 }
 
 void COFFDumper::printImportedSymbols(
     iterator_range<imported_symbol_iterator> Range) {
   for (const ImportedSymbolRef &I : Range) {
     StringRef Sym;
     error(I.getSymbolName(Sym));

llvm/tools/llvm-readobj/ELFDumper.cpp

public:
   virtual void printMipsPLT(const MipsGOTParser<ELFT> &Parser) = 0;
   const ELFDumper<ELFT> *dumper() const { return Dumper; }
 
 private:
   const ELFDumper<ELFT> *Dumper;
 };
 
 template <typename ELFT> class GNUStyle : public DumpStyle<ELFT> {
-  formatted_raw_ostream OS;
+  formatted_raw_ostream &OS;
 
 public:
   TYPEDEF_ELF_TYPES(ELFT)
 
   GNUStyle(ScopedPrinter &W, ELFDumper<ELFT> *Dumper)
-      : DumpStyle<ELFT>(Dumper), OS(W.getOStream()) {}
+      : DumpStyle<ELFT>(Dumper),
+        OS(static_cast<formatted_raw_ostream&>(W.getOStream())) {}
 
   void printFileHeaders(const ELFO *Obj) override;
   void printGroupSections(const ELFFile<ELFT> *Obj) override;
   void printRelocations(const ELFO *Obj) override;
   void printSectionHeaders(const ELFO *Obj) override;
   void printSymbols(const ELFO *Obj, bool PrintSymbols,
                     bool PrintDynamicSymbols) override;
   void printHashSymbols(const ELFO *Obj) override;
   void printDynamic(const ELFFile<ELFT> *Obj) override;

llvm/tools/llvm-readobj/MachODumper.cpp

if (Command.C.cmd == MachO::LC_LOAD_DYLIB ||
         Libs.push_back(P);
       }
     }
   }
 
   llvm::stable_sort(Libs);
 
   for (const auto &L : Libs) {
-    outs() << "  " << L << "\n";
+    W.startLine() << L << "\n";
   }
 }
 
 void MachODumper::printMachODataInCode() {
   for (const auto &Load : Obj->load_commands()) {
     if (Load.C.cmd  == MachO::LC_DATA_IN_CODE) {
       MachO::linkedit_data_command LLC = Obj->getLinkeditDataLoadCommand(Load);
       DictScope Group(W, "DataInCode");

llvm/tools/llvm-readobj/llvm-readobj.cpp

if (opts::Output == opts::LLVM) {
     Writer.startLine() << "\n";
     Writer.printString("File", Obj->getFileName());
     Writer.printString("Format", Obj->getFileFormatName());
     Writer.printString("Arch", Triple::getArchTypeName(
                                    (llvm::Triple::ArchType)Obj->getArch()));
     Writer.printString("AddressSize",
                        formatv("{0}bit", 8 * Obj->getBytesInAddress()));
     Dumper->printLoadName();
+    Writer.flush();
   }
 
   if (opts::FileHeaders)
     Dumper->printFileHeaders();
   if (opts::SectionHeaders)
     Dumper->printSectionHeaders();
   if (opts::Relocations)
     Dumper->printRelocations();
static void dumpWindowsResourceFile(WindowsResource *WinRes) {
   WindowsRes::Dumper Dumper(WinRes, Printer);
   if (auto Err = Dumper.printData())
     reportError(WinRes->getFileName(), std::move(Err));
 }
 
 
 /// Opens \a File and dumps it.
 static void dumpInput(StringRef File) {
-  ScopedPrinter Writer(outs());
+  ScopedPrinter Writer(fouts());
 
   // Attempt to open the binary.
   Expected<OwningBinary<Binary>> BinaryOrErr = createBinary(File);
   if (!BinaryOrErr)
     reportError(File, BinaryOrErr.takeError());
   Binary &Binary = *BinaryOrErr.get().getBinary();
 
   if (Archive *Arc = dyn_cast<Archive>(&Binary))