[Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation

[abhina.sreeskantharajan]

Summary

This patch should fix the errors shown on the Windows bots by turning off text mode. I plan to investigate a better fix but this should unblock the buildbots for now.

[rnk]

clang/lib/Frontend/Rewrite/FrontendActions.cpp

190

In the long run, we really don't want this "is windows" check here in the caller. We really want a flag to push this logic down into the support library. Since we already know where we are going, I think these are the proper steps:

• revert to green
• add OF_TextWithCrLF, with the same behavior as OF_Text today
• update all callers that used OF_Text before these System/Z patches to OF_TextWithCrlf (NFC)
• make OF_Text skip CRLF conversion on Windows (check for breakage)
• start relanding OF_Text changes for System/Z again

Landing this patch as it is creates more code that we will have to edit and refactor later back to just OF_Text.

[abhina.sreeskantharajan]

Revert changes that cause errors instead by turning on binary mode again

[rnk]

lgtm

[abhina.sreeskantharajan]

clang/lib/Frontend/Rewrite/FrontendActions.cpp

190

Yes, this sounds like a good plan. It doesn't seem like z/OS and Windows can share the same OF_Text flag as I originally thought. Thanks!

[aganea]

Sorry, but after this patch, I still see the issue mentioned in https://reviews.llvm.org/D96363#2650460
@abhina.sreeskantharajan are you able to confirm the issue on your end?

[abhina.sreeskantharajan]

In D99363#2652899, @aganea wrote:

Sorry, but after this patch, I still see the issue mentioned in https://reviews.llvm.org/D96363#2650460
@abhina.sreeskantharajan are you able to confirm the issue on your end?

I don't have a Windows machine to test on, but I assumed since the problem was in rewrite then this patch should have fixed it. Would you be able to confirm if reverting the change in clang/lib/Driver/Driver.cpp fixes the problem for you?

[aganea]

In D99363#2652907, @abhina.sreeskantharajan wrote:

In D99363#2652899, @aganea wrote:

Sorry, but after this patch, I still see the issue mentioned in https://reviews.llvm.org/D96363#2650460
@abhina.sreeskantharajan are you able to confirm the issue on your end?

I don't have a Windows machine to test on, but I assumed since the problem was in rewrite then this patch should have fixed it. Would you be able to confirm if reverting the change in clang/lib/Driver/Driver.cpp fixes the problem for you?

I went back to the version on the left:

but that does not fix the problem.

Indeed, I've tested & the problem does not occur on Linux.

[aganea]

I'm just wondering if D96363 and all attached subsequent patches shouldn't be reverted for now. This is a quite trivial case uncovered by tests. On re-land, I would then add a test validating the issue on Windows:

$ cat -A rewrite-includes-clang-cl.cpp
// REQUIRES: system-windows^M$
// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c /Tp -^M$
^M$
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}^M$

[abhina.sreeskantharajan]

In D99363#2653201, @aganea wrote:

I'm just wondering if D96363 and all attached subsequent patches shouldn't be reverted for now. This is a quite trivial case uncovered by tests. On re-land, I would then add a test validating the issue on Windows:

$ cat -A rewrite-includes-clang-cl.cpp
// REQUIRES: system-windows^M$
// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c /Tp -^M$
^M$
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}^M$

There were a lot of similar patches so reverting all of them might be more work than isolating the change that caused it and reverting that. It seems that the patch you initially commented on did not contain the problematic change since reverting the change doesn't fix your issue. I created the following patch https://reviews.llvm.org/D99426 based on @rnk suggestion. I created a new flag for OF_TextWithCRLF on Windows and made sure my most recent text changes use the OF_Text flag while all other uses were changed to OF_TextWithCRLF. This should solve any CRLF issues that were introduced recently by my patches. If you have time, would you be able to test if that patch fixes your issue?

[aganea]

In D99363#2653476, @abhina.sreeskantharajan wrote:

In D99363#2653201, @aganea wrote:

I'm just wondering if D96363 and all attached subsequent patches shouldn't be reverted for now. This is a quite trivial case uncovered by tests. On re-land, I would then add a test validating the issue on Windows:

$ cat -A rewrite-includes-clang-cl.cpp
// REQUIRES: system-windows^M$
// RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c /Tp -^M$
^M$
int foo();^M$
int bar();^M$
#define HELLO \^M$
  foo(); \^M$
  bar();^M$
^M$
int main() {^M$
  HELLO^M$
  return 0;^M$
}^M$

There were a lot of similar patches so reverting all of them might be more work than isolating the change that caused it and reverting that. It seems that the patch you initially commented on did not contain the problematic change since reverting the change doesn't fix your issue.

Actually it is git bisect that pointed me to that patch :-)

I created the following patch https://reviews.llvm.org/D99426 based on @rnk suggestion. I created a new flag for OF_TextWithCRLF on Windows and made sure my most recent text changes use the OF_Text flag while all other uses were changed to OF_TextWithCRLF. This should solve any CRLF issues that were introduced recently by my patches. If you have time, would you be able to test if that patch fixes your issue?

Yes I will!

[aganea]

In D99363#2653476, @abhina.sreeskantharajan wrote:

There were a lot of similar patches so reverting all of them might be more work than isolating the change that caused it and reverting that. It seems that the patch you initially commented on did not contain the problematic change since reverting the change doesn't fix your issue. I created the following patch https://reviews.llvm.org/D99426 based on @rnk suggestion. I created a new flag for OF_TextWithCRLF on Windows and made sure my most recent text changes use the OF_Text flag while all other uses were changed to OF_TextWithCRLF. This should solve any CRLF issues that were introduced recently by my patches. If you have time, would you be able to test if that patch fixes your issue?

I've applied https://reviews.llvm.org/D99426#2656738 over rGc4d5b956170dd85941c1c2787abaa2e01575234c but I'm still seeing the issue in https://reviews.llvm.org/D96363#2650460.

[abhina.sreeskantharajan]

In D99363#2656913, @aganea wrote:

In D99363#2653476, @abhina.sreeskantharajan wrote:

There were a lot of similar patches so reverting all of them might be more work than isolating the change that caused it and reverting that. It seems that the patch you initially commented on did not contain the problematic change since reverting the change doesn't fix your issue. I created the following patch https://reviews.llvm.org/D99426 based on @rnk suggestion. I created a new flag for OF_TextWithCRLF on Windows and made sure my most recent text changes use the OF_Text flag while all other uses were changed to OF_TextWithCRLF. This should solve any CRLF issues that were introduced recently by my patches. If you have time, would you be able to test if that patch fixes your issue?

I've applied https://reviews.llvm.org/D99426#2656738 over rGc4d5b956170dd85941c1c2787abaa2e01575234c but I'm still seeing the issue in https://reviews.llvm.org/D96363#2650460.

Ok, I missed the change in llvm/tools/dsymutil/DwarfLinkerForBinary.cpp.

In D99363#2656913, @aganea wrote:

In D99363#2653476, @abhina.sreeskantharajan wrote:

There were a lot of similar patches so reverting all of them might be more work than isolating the change that caused it and reverting that. It seems that the patch you initially commented on did not contain the problematic change since reverting the change doesn't fix your issue. I created the following patch https://reviews.llvm.org/D99426 based on @rnk suggestion. I created a new flag for OF_TextWithCRLF on Windows and made sure my most recent text changes use the OF_Text flag while all other uses were changed to OF_TextWithCRLF. This should solve any CRLF issues that were introduced recently by my patches. If you have time, would you be able to test if that patch fixes your issue?

I've applied https://reviews.llvm.org/D99426#2656738 over rGc4d5b956170dd85941c1c2787abaa2e01575234c but I'm still seeing the issue in https://reviews.llvm.org/D96363#2650460.

Sorry, I realized the change for DwarfLinkerForBinary.cpp was missing. This was only other file from https://reviews.llvm.org/D96363 that was using OF_TextWithCRLF instead of OF_Text. Please let me know if this latest patch https://reviews.llvm.org/file/data/2jljo4tfl5aiisvwpzg2/PHID-FILE-egbpcbhz3t7b7a2tcjka/D99426.diff fixes your issue. If not, I will revert the remaining changes in my old commit to unblock you.

[aganea]

In D99363#2653476, @abhina.sreeskantharajan wrote:

This was only other file from https://reviews.llvm.org/D96363 that was using OF_TextWithCRLF instead of OF_Text. Please let me know if this latest patch https://reviews.llvm.org/file/data/2jljo4tfl5aiisvwpzg2/PHID-FILE-egbpcbhz3t7b7a2tcjka/D99426.diff fixes your issue.

There's no difference after applying the above change, I still see the issue.

I realized the change for DwarfLinkerForBinary.cpp was missing

Sorry if I misunderstand, but I don't think DwarfLinkerForBinary.cpp is used when targeting Windows binaries.

If not, I will revert the remaining changes in my old commit to unblock you.

I am not blocked. If the fix comes later on in the following days/weeks, I'm fine with that.

clang/lib/Frontend/Rewrite/FrontendActions.cpp

 
 //===----------------------------------------------------------------------===//
 // Preprocessor Actions
 //===----------------------------------------------------------------------===//
 
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
   std::unique_ptr<raw_ostream> OS =
-      CI.createDefaultOutputFile(/*Binary=*/false, getCurrentFileOrBufferName());
+      CI.createDefaultOutputFile(/*Binary=*/true, getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
 }
 
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance &CI = getCompilerInstance();
   std::unique_ptr<raw_ostream> OS =
-      CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
+      CI.createDefaultOutputFile(/*Binary=*/true, getCurrentFileOrBufferName());
   if (!OS) return;
 
   DoRewriteTest(CI.getPreprocessor(), OS.get());
 }
 
 class RewriteIncludesAction::RewriteImportsListener : public ASTReaderListener {
   CompilerInstance &CI;
   std::weak_ptr<raw_ostream> Out;

llvm/lib/TableGen/Main.cpp

int llvm::TableGenMain(const char *argv0, TableGenMainFn *MainFn) {
 
   if (TimePhases)
     Records.startPhaseTiming();
 
   // Parse the input file.
 
   Records.startTimer("Parse, build records");
   ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr =
-      MemoryBuffer::getFileOrSTDIN(InputFilename, /*IsText=*/true);
+      MemoryBuffer::getFileOrSTDIN(InputFilename, /*IsText=*/false);
   if (std::error_code EC = FileOrErr.getError())
     return reportError(argv0, "Could not open input file '" + InputFilename +
                                   "': " + EC.message() + "\n");
 
   Records.saveInputFilename(InputFilename);
 
   // Tell SrcMgr about this buffer, which is what TGParser will pick up.
   SrcMgr.AddNewSourceBuffer(std::move(*FileOrErr), SMLoc());
int llvm::TableGenMain(const char *argv0, TableGenMainFn *MainFn) {
 
   Records.startTimer("Write output");
   bool WriteFile = true;
   if (WriteIfChanged) {
     // Only updates the real output file if there are any differences.
     // This prevents recompilation of all the files depending on it if there
     // aren't any.
     if (auto ExistingOrErr =
-            MemoryBuffer::getFile(OutputFilename, /*IsText=*/true))
+            MemoryBuffer::getFile(OutputFilename, /*IsText=*/false))
       if (std::move(ExistingOrErr.get())->getBuffer() == Out.str())
         WriteFile = false;
   }
   if (WriteFile) {
     std::error_code EC;
     ToolOutputFile OutFile(OutputFilename, EC, sys::fs::OF_Text);
     if (EC)
       return reportError(argv0, "error opening " + OutputFilename + ": " +