[tools] Mark output of tools as text if it is really text

[Kai]

Summary

Several LLVM tools write text files/streams without using OF_Text.
This can cause problems on platforms which distinguish between
text and binary output. This PR adds the OF_Text flag for the
following tools:

• llvm-dis
• llvm-dwarfdump
• llvm-mca
• llvm-mc (assembler files only)
• opt (assembler files only)
• RemarkStreamer (used e.g. by opt)

Most notably, llc already makes the distinction between text and binary output.

[thegameg]

llvm/lib/IR/RemarkStreamer.cpp

133

Depending on RemarksFormat, the output can also be LLVM Bitstream. Should this set OF_Text only if the format is YAML?

[rnk]

The main effect that I'm aware of here is that this will do CRLF conversion on Windows. Are we sure we really want that? I just did the opposite to TableGen output in rL371683. In general, I think the majority of tools that write text use OF_Text, so your change makes this minority of tools consistent with the other ones.

[Kai]

Use OF_Text for the RemarkStreamer only if format is YAML.

[Kai]

In D67696#1674254, @rnk wrote:

The main effect that I'm aware of here is that this will do CRLF conversion on Windows. Are we sure we really want that? I just did the opposite to TableGen output in rL371683. In general, I think the majority of tools that write text use OF_Text, so your change makes this minority of tools consistent with the other ones.

I am on a platform where the difference between text and binary streams is much greater than on Windows. Therefore I really appreciate the consistency.
Yes, I noted your commit. It had surprising effects...

llvm/lib/IR/RemarkStreamer.cpp

133

Ah - thanks. Of course it should only be set if the format is YAML.

[Kai]

Fixed an error introduced in the previous revision.

[Kai]

• Add OF_Text for opt assembly output
• Fix RemarkStreamer
• Use OF_None instead of F_None in new code

[rnk]

lgtm

[Bigcheese]

Do we really want to output \r\n on Windows? There is all of one program (notepad.exe) that doesn't support \n on Windows.

[MaskRay]

In D67696#1675194, @Kai wrote:

In D67696#1674254, @rnk wrote:

The main effect that I'm aware of here is that this will do CRLF conversion on Windows. Are we sure we really want that? I just did the opposite to TableGen output in rL371683. In general, I think the majority of tools that write text use OF_Text, so your change makes this minority of tools consistent with the other ones.

I am on a platform where the difference between text and binary streams is much greater than on Windows. Therefore I really appreciate the consistency.
Yes, I noted your commit. It had surprising effects...

Can you summarize the differences on that platform? z/OS?

While b mode is allowed for fopen in ISO C, on POSIX-conforming systems:

The character 'b' shall have no effect, but is allowed for ISO C standard conformance.

llvm/lib/IR/RemarkStreamer.cpp

if (RemarksWithHotness)
     Context.setDiagnosticsHotnessRequested(true);
 
   if (RemarksHotnessThreshold)
     Context.setDiagnosticsHotnessThreshold(RemarksHotnessThreshold);
 
   if (RemarksFilename.empty())
     return nullptr;
 
+  Expected<remarks::Format> Format = remarks::parseFormat(RemarksFormat);
+  if (Error E = Format.takeError())
+    return make_error<RemarkSetupFormatError>(std::move(E));
+
   std::error_code EC;
+  auto Flags = *Format == remarks::Format::YAML ? sys::fs::OF_Text
+                                                : sys::fs::OF_None;
   auto RemarksFile =
-      std::make_unique<ToolOutputFile>(RemarksFilename, EC, sys::fs::OF_None);
+      std::make_unique<ToolOutputFile>(RemarksFilename, EC, Flags);
   // We don't use llvm::FileError here because some diagnostics want the file
   // name separately.
   if (EC)
     return make_error<RemarkSetupFileError>(errorCodeToError(EC));
 
-  Expected<remarks::Format> Format = remarks::parseFormat(RemarksFormat);
-  if (Error E = Format.takeError())
-    return make_error<RemarkSetupFormatError>(std::move(E));
-
   Expected<std::unique_ptr<remarks::RemarkSerializer>> RemarkSerializer =
       remarks::createRemarkSerializer(
           *Format, remarks::SerializerMode::Separate, RemarksFile->os());
   if (Error E = RemarkSerializer.takeError())
     return make_error<RemarkSetupFormatError>(std::move(E));
 
   Context.setRemarkStreamer(std::make_unique<RemarkStreamer>(
       std::move(*RemarkSerializer), RemarksFilename));

llvm/tools/llvm-dis/llvm-dis.cpp

if (InputFilename == "-") {
       StringRef IFN = InputFilename;
       OutputFilename = (IFN.endswith(".bc") ? IFN.drop_back(3) : IFN).str();
       OutputFilename += ".ll";
     }
   }
 
   std::error_code EC;
   std::unique_ptr<ToolOutputFile> Out(
-      new ToolOutputFile(OutputFilename, EC, sys::fs::OF_None));
+      new ToolOutputFile(OutputFilename, EC, sys::fs::OF_Text));
   if (EC) {
     errs() << EC.message() << '\n';
     return 1;
   }
 
   std::unique_ptr<AssemblyAnnotationWriter> Annotator;
   if (ShowAnnotations)
     Annotator.reset(new CommentWriter());

llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp

int main(int argc, char **argv) {
   //        compatible.
   if (Diff && Verbose) {
     WithColor::error() << "incompatible arguments: specifying both -diff and "
                           "-verbose is currently not supported";
     return 0;
   }
 
   std::error_code EC;
-  ToolOutputFile OutputFile(OutputFilename, EC, sys::fs::OF_None);
+  ToolOutputFile OutputFile(OutputFilename, EC, sys::fs::OF_Text);
   error("Unable to open output file" + OutputFilename, EC);
   // Don't remove output file if we exit with an error.
   OutputFile.keep();
 
   bool OffsetRequested = false;
 
   // Defaults to dumping all sections, unless brief mode is specified in which
   // case only the .debug_info section in dumped.

llvm/tools/llvm-mc/llvm-mc.cpp

if (!TheTarget) {
     return nullptr;
   }
 
   // Update the triple name and return the found target.
   TripleName = TheTriple.getTriple();
   return TheTarget;
 }
 
-static std::unique_ptr<ToolOutputFile> GetOutputStream(StringRef Path) {
+static std::unique_ptr<ToolOutputFile> GetOutputStream(StringRef Path,
+    sys::fs::OpenFlags Flags) {
   std::error_code EC;
-  auto Out = std::make_unique<ToolOutputFile>(Path, EC, sys::fs::OF_None);
+  auto Out = std::make_unique<ToolOutputFile>(Path, EC, Flags);
   if (EC) {
     WithColor::error() << EC.message() << '\n';
     return nullptr;
   }
 
   return Out;
 }
 
int main(int argc, char **argv) {
   std::string FeaturesStr;
   if (MAttrs.size()) {
     SubtargetFeatures Features;
     for (unsigned i = 0; i != MAttrs.size(); ++i)
       Features.AddFeature(MAttrs[i]);
     FeaturesStr = Features.getString();
   }
 
-  std::unique_ptr<ToolOutputFile> Out = GetOutputStream(OutputFilename);
+  sys::fs::OpenFlags Flags = (FileType == OFT_AssemblyFile) ? sys::fs::OF_Text
+                                                            : sys::fs::OF_None;
+  std::unique_ptr<ToolOutputFile> Out = GetOutputStream(OutputFilename, Flags);
   if (!Out)
     return 1;
 
   std::unique_ptr<ToolOutputFile> DwoOut;
   if (!SplitDwarfFile.empty()) {
     if (FileType != OFT_ObjectFile) {
       WithColor::error() << "dwo output only supported with object files\n";
       return 1;
     }
-    DwoOut = GetOutputStream(SplitDwarfFile);
+    DwoOut = GetOutputStream(SplitDwarfFile, sys::fs::OF_None);
     if (!DwoOut)
       return 1;
   }
 
   std::unique_ptr<buffer_ostream> BOS;
   raw_pwrite_stream *OS = &Out->os();
   std::unique_ptr<MCStreamer> Str;
 

llvm/tools/llvm-mca/llvm-mca.cpp

const Target *getTarget(const char *ProgName) {
   return TheTarget;
 }
 
 ErrorOr<std::unique_ptr<ToolOutputFile>> getOutputStream() {
   if (OutputFilename == "")
     OutputFilename = "-";
   std::error_code EC;
   auto Out =
-      std::make_unique<ToolOutputFile>(OutputFilename, EC, sys::fs::OF_None);
+      std::make_unique<ToolOutputFile>(OutputFilename, EC, sys::fs::OF_Text);
   if (!EC)
     return std::move(Out);
   return EC;
 }
 } // end of anonymous namespace
 
 static void processOptionImpl(cl::opt<bool> &O, const cl::opt<bool> &Default) {
   if (!O.getNumOccurrences() || O.getPosition() < Default.getPosition())

llvm/tools/opt/opt.cpp

if (!OutputFilename.empty())
       errs() << "WARNING: The -o (output filename) option is ignored when\n"
                 "the --disable-output option is used.\n";
   } else {
     // Default to standard output.
     if (OutputFilename.empty())
       OutputFilename = "-";
 
     std::error_code EC;
-    Out.reset(new ToolOutputFile(OutputFilename, EC, sys::fs::OF_None));
+    sys::fs::OpenFlags Flags = OutputAssembly ? sys::fs::OF_Text
+                                              : sys::fs::OF_None;
+    Out.reset(new ToolOutputFile(OutputFilename, EC, Flags));
     if (EC) {
       errs() << EC.message() << '\n';
       return 1;
     }
 
     if (!ThinLinkBitcodeFile.empty()) {
       ThinLinkOut.reset(
           new ToolOutputFile(ThinLinkBitcodeFile, EC, sys::fs::OF_None));