Mark output as text if it is really text

[abhina.sreeskantharajan]

Summary

This is a continuation of https://reviews.llvm.org/D67696. The following places need to set the OF_Text flag correctly.

[abhina.sreeskantharajan]

Remove ARCMigrate change in this patch.

[abhina.sreeskantharajan]

Rerun CI

[JDevlieghere]

LGTM with two inline nits.

clang/lib/Frontend/Rewrite/FrontendActions.cpp

188

273

[abhina.sreeskantharajan]

Thanks for reviewing! Addressing inline comments by adding /*Binary*/ comment.

[JDevlieghere]

Thanks. LGTM!

[aganea]

Hello! This patch breaks compilation when using -frewrite-includes on Windows. It adds an extra line with CRLF, which causes compilation failures with macros line-breaks:

$ cat -A hello.cpp
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}
$ clang-cl /E -Xclang -frewrite-includes hello.cpp > test.cpp

$ cat -A test.cpp
# 1 "<built-in>"^M^M$
# 1 "hello.cpp"^M^M$
int foo();^M^M$
int bar();^M^M$
#define HELLO \^M^M$
  foo(); \^M^M$
  bar();^M^M$
^M^M$
int main() {^M^M$
  HELLO^M^M$
  return 0;^M^M$
}^M^M$

$ clang-cl /c test.cpp
hello.cpp(8,3): error: C++ requires a type specifier for all declarations
  foo(); \
  ^
hello.cpp(10,3): error: C++ requires a type specifier for all declarations
  bar();
  ^
2 errors generated.

$ clang-cl /c hello.cpp

$ 

@abhina.sreeskantharajan would you have a chance to take a look please?

[abhina.sreeskantharajan]

In D96363#2650460, @aganea wrote:

Hello! This patch breaks compilation when using -frewrite-includes on Windows. It adds an extra line with CRLF, which causes compilation failures with macros line-breaks:

$ cat -A hello.cpp
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}
$ clang-cl /E -Xclang -frewrite-includes hello.cpp > test.cpp

$ cat -A test.cpp
# 1 "<built-in>"^M^M$
# 1 "hello.cpp"^M^M$
int foo();^M^M$
int bar();^M^M$
#define HELLO \^M^M$
  foo(); \^M^M$
  bar();^M^M$
^M^M$
int main() {^M^M$
  HELLO^M^M$
  return 0;^M^M$
}^M^M$

$ clang-cl /c test.cpp
hello.cpp(8,3): error: C++ requires a type specifier for all declarations
  foo(); \
  ^
hello.cpp(10,3): error: C++ requires a type specifier for all declarations
  bar();
  ^
2 errors generated.

$ clang-cl /c hello.cpp

$ 

@abhina.sreeskantharajan would you have a chance to take a look please?

Sure, let me take a look.

[rnk]

This is why, in the past, we have taken the direction of opening all files as binary files. You have probably noticed all of the changes in this direction that you have to work around. I think the best solution here would be to add a new OF_TextNoCrlf mode that does what you need it to for System/Z, but opens the file in binary mode on Windows.

[abhina.sreeskantharajan]

In D96363#2650904, @rnk wrote:

This is why, in the past, we have taken the direction of opening all files as binary files. You have probably noticed all of the changes in this direction that you have to work around. I think the best solution here would be to add a new OF_TextNoCrlf mode that does what you need it to for System/Z, but opens the file in binary mode on Windows.

Right, adding a new flag like OF_TextNoCrlf is something I can look into as a solution. However, if Windows is always using binary mode and has no use for OF_Text flag, maybe I can globally set that mode similar to how I set it for ToolOutputFiles. I don't think any other platform is affected by the binary/text mode.

[rnk]

In D96363#2650999, @abhina.sreeskantharajan wrote:

Right, adding a new flag like OF_TextNoCrlf is something I can look into as a solution. However, if Windows is always using binary mode and has no use for OF_Text flag, maybe I can globally set that mode similar to how I set it for ToolOutputFiles. I don't think any other platform is affected by the binary/text mode.

I can't say for sure if we always want LF output on Windows, so I'm not sure we should commit to that simplification. I think we usually want to avoid CRLF when we are writing code-like output files, things like JSON, YAML, or C++ .inc files, but we might want to retain CRLF when writing to things like log files or IDEs. And even if we audit all current use cases, someone in the future might want CRLF conversion. On the other hand, it does leave behind a bit of a trap for developers, who will probably pick OF_Text before OF_TextNoCrlf. Maybe OF_Text should not do the conversion, and a longer OF_TextWithCrlf should enable CRLF conversion.

[abhina.sreeskantharajan]

In D96363#2651116, @rnk wrote:

In D96363#2650999, @abhina.sreeskantharajan wrote:

Right, adding a new flag like OF_TextNoCrlf is something I can look into as a solution. However, if Windows is always using binary mode and has no use for OF_Text flag, maybe I can globally set that mode similar to how I set it for ToolOutputFiles. I don't think any other platform is affected by the binary/text mode.

I can't say for sure if we always want LF output on Windows, so I'm not sure we should commit to that simplification. I think we usually want to avoid CRLF when we are writing code-like output files, things like JSON, YAML, or C++ .inc files, but we might want to retain CRLF when writing to things like log files or IDEs. And even if we audit all current use cases, someone in the future might want CRLF conversion. On the other hand, it does leave behind a bit of a trap for developers, who will probably pick OF_Text before OF_TextNoCrlf. Maybe OF_Text should not do the conversion, and a longer OF_TextWithCrlf should enable CRLF conversion.

Ok, I've created a patch to workaround the errors https://reviews.llvm.org/D99363. But I do plan to investigate a better solution going forward.

clang/lib/ARCMigrate/FileRemapper.cpp

bool FileRemapper::flushToDisk(StringRef outputDir, DiagnosticsEngine &Diag) {
   return flushToFile(infoFile, Diag);
 }
 
 bool FileRemapper::flushToFile(StringRef outputPath, DiagnosticsEngine &Diag) {
   using namespace llvm::sys;
 
   std::error_code EC;
   std::string infoFile = std::string(outputPath);
-  llvm::raw_fd_ostream infoOut(infoFile, EC, llvm::sys::fs::OF_None);
+  llvm::raw_fd_ostream infoOut(infoFile, EC, llvm::sys::fs::OF_Text);
   if (EC)
     return report(EC.message(), Diag);
 
   for (MappingsTy::iterator
          I = FromToMappings.begin(), E = FromToMappings.end(); I != E; ++I) {
 
     const FileEntry *origFE = I->first;
     SmallString<200> origPath = StringRef(origFE->getName());

clang/lib/Driver/Driver.cpp

void Driver::generateCompilationDiagnostics(
   }
 
   // Assume associated files are based off of the first temporary file.
   CrashReportInfo CrashInfo(TempFiles[0], VFS);
 
   llvm::SmallString<128> Script(CrashInfo.Filename);
   llvm::sys::path::replace_extension(Script, "sh");
   std::error_code EC;
-  llvm::raw_fd_ostream ScriptOS(Script, EC, llvm::sys::fs::CD_CreateNew);
+  llvm::raw_fd_ostream ScriptOS(Script, EC, llvm::sys::fs::CD_CreateNew,
+                                llvm::sys::fs::FA_Write,
+                                llvm::sys::fs::OF_Text);
   if (EC) {
     Diag(clang::diag::note_drv_command_failed_diag_msg)
         << "Error generating run script: " << Script << " " << EC.message();
   } else {
     ScriptOS << "# Crash reproducer for " << getClangFullVersion() << "\n"
              << "# Driver args: ";
     printArgList(ScriptOS, C.getInputArgs());
     ScriptOS << "# Original command: ";

clang/lib/Frontend/Rewrite/FrontendActions.cpp

 
 //===----------------------------------------------------------------------===//
 // Preprocessor Actions
 //===----------------------------------------------------------------------===//
 
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
   std::unique_ptr<raw_ostream> OS =
-      CI.createDefaultOutputFile(true, getCurrentFileOrBufferName());
+      CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
 }
 
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
   std::unique_ptr<raw_ostream> OS =
void visitModuleFile(StringRef Filename,
 
     (*OS) << "#pragma clang module endbuild /*" << MF->ModuleName << "*/\n";
   }
 };
 
 bool RewriteIncludesAction::BeginSourceFileAction(CompilerInstance &CI) {
   if (!OutputStream) {
     OutputStream =
-        CI.createDefaultOutputFile(true, getCurrentFileOrBufferName());
+        CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
     if (!OutputStream)
       return false;
   }
 
   auto &OS = *OutputStream;
 
   // If we're preprocessing a module map, start by dumping the contents of the
   // module itself before switching to the input buffer.

llvm/tools/dsymutil/DwarfLinkerForBinary.cpp

static Error emitRemarks(const LinkOptions &Options, StringRef BinaryPath,
   sys::path::append(Path, sys::path::filename(BinaryPath));
   if (Options.NumDebugMaps > 1) {
     // More than one debug map means we have a fat binary.
     Path += '-';
     Path += ArchName;
   }
 
   std::error_code EC;
-  raw_fd_ostream OS(Options.NoOutput ? "-" : Path.str(), EC, sys::fs::OF_None);
+  raw_fd_ostream OS(Options.NoOutput ? "-" : Path.str(), EC,
+                    Options.RemarksFormat == remarks::Format::Bitstream
+                        ? sys::fs::OF_None
+                        : sys::fs::OF_Text);
   if (EC)
     return errorCodeToError(EC);
 
   if (Error E = RL.serialize(OS, Options.RemarksFormat))
     return E;
 
   return Error::success();
 }