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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

>> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

I'm just wondering if D96363  and all attached 
subsequent patches shouldn't be reverted for now. This is 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: windows^M$
  // RUN: %clang_cl /E -Xclang -frewrite-includes %s | %clang_cl /c 
/clang:-verify /Tp -^M$
  // expected-no-diagnostics^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$


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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:
F16001158: image.png 
but that does not fix the problem.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbc5d4bcc2deb: [Windows] Turn off text mode in TableGen and 
Rewriter to stop CRLF translation (authored by abhina.sreeskantharajan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

Files:
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  llvm/lib/TableGen/Main.cpp


Index: llvm/lib/TableGen/Main.cpp
===
--- llvm/lib/TableGen/Main.cpp
+++ llvm/lib/TableGen/Main.cpp
@@ -93,7 +93,7 @@
 
   Records.startTimer("Parse, build records");
   ErrorOr> 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");
@@ -138,7 +138,7 @@
 // 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;
   }
Index: clang/lib/Frontend/Rewrite/FrontendActions.cpp
===
--- clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -185,7 +185,7 @@
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(/*Binary=*/false, 
getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, 
getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
@@ -194,7 +194,7 @@
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, 
getCurrentFileOrBufferName());
   if (!OS) return;
 
   DoRewriteTest(CI.getPreprocessor(), OS.get());


Index: llvm/lib/TableGen/Main.cpp
===
--- llvm/lib/TableGen/Main.cpp
+++ llvm/lib/TableGen/Main.cpp
@@ -93,7 +93,7 @@
 
   Records.startTimer("Parse, build records");
   ErrorOr> 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");
@@ -138,7 +138,7 @@
 // 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;
   }
Index: clang/lib/Frontend/Rewrite/FrontendActions.cpp
===
--- clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -185,7 +185,7 @@
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(/*Binary=*/false, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
@@ -194,7 +194,7 @@
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, getCurrentFileOrBufferName());
   if (!OS) return;
 
   DoRewriteTest(CI.getPreprocessor(), OS.get());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan added inline comments.



Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:190
+  std::unique_ptr OS = CI.createDefaultOutputFile(
+  /*Binary=*/llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows(),
+  getCurrentFileOrBufferName());

rnk wrote:
> 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.
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan updated this revision to Diff 82.
abhina.sreeskantharajan added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

Files:
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  llvm/lib/TableGen/Main.cpp


Index: llvm/lib/TableGen/Main.cpp
===
--- llvm/lib/TableGen/Main.cpp
+++ llvm/lib/TableGen/Main.cpp
@@ -93,7 +93,7 @@
 
   Records.startTimer("Parse, build records");
   ErrorOr> 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");
@@ -138,7 +138,7 @@
 // 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;
   }
Index: clang/lib/Frontend/Rewrite/FrontendActions.cpp
===
--- clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -185,7 +185,7 @@
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(/*Binary=*/false, 
getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, 
getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
@@ -194,7 +194,7 @@
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, 
getCurrentFileOrBufferName());
   if (!OS) return;
 
   DoRewriteTest(CI.getPreprocessor(), OS.get());


Index: llvm/lib/TableGen/Main.cpp
===
--- llvm/lib/TableGen/Main.cpp
+++ llvm/lib/TableGen/Main.cpp
@@ -93,7 +93,7 @@
 
   Records.startTimer("Parse, build records");
   ErrorOr> 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");
@@ -138,7 +138,7 @@
 // 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;
   }
Index: clang/lib/Frontend/Rewrite/FrontendActions.cpp
===
--- clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -185,7 +185,7 @@
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(/*Binary=*/false, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
@@ -194,7 +194,7 @@
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
   std::unique_ptr OS =
-  CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
+  CI.createDefaultOutputFile(/*Binary=*/true, getCurrentFileOrBufferName());
   if (!OS) return;
 
   DoRewriteTest(CI.getPreprocessor(), OS.get());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk requested changes to this revision.
rnk added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:190
+  std::unique_ptr OS = CI.createDefaultOutputFile(
+  /*Binary=*/llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows(),
+  getCurrentFileOrBufferName());

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99363

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Abhina Sree via Phabricator via cfe-commits]

abhina.sreeskantharajan created this revision.
abhina.sreeskantharajan added reviewers: rnk, amccarth, yroux, aganea.
Herald added a subscriber: hiraditya.
abhina.sreeskantharajan requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99363

Files:
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  llvm/lib/TableGen/Main.cpp


Index: llvm/lib/TableGen/Main.cpp
===
--- llvm/lib/TableGen/Main.cpp
+++ llvm/lib/TableGen/Main.cpp
@@ -17,6 +17,7 @@
 #include "llvm/TableGen/Main.h"
 #include "TGParser.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -90,10 +91,13 @@
 Records.startPhaseTiming();
 
   // Parse the input file.
+  // On Windows, set binary mode to turn off CRLF translation.
+  bool IsWindows = llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows();
 
   Records.startTimer("Parse, build records");
   ErrorOr> FileOrErr =
-  MemoryBuffer::getFileOrSTDIN(InputFilename, /*IsText=*/true);
+  MemoryBuffer::getFileOrSTDIN(InputFilename,
+   IsWindows ? /*IsText=*/false : true);
   if (std::error_code EC = FileOrErr.getError())
 return reportError(argv0, "Could not open input file '" + InputFilename +
   "': " + EC.message() + "\n");
@@ -137,8 +141,8 @@
 // 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))
+if (auto ExistingOrErr = MemoryBuffer::getFile(
+OutputFilename, IsWindows ? /*IsText=*/false : true))
   if (std::move(ExistingOrErr.get())->getBuffer() == Out.str())
 WriteFile = false;
   }
Index: clang/lib/Frontend/Rewrite/FrontendActions.cpp
===
--- clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -24,6 +24,7 @@
 #include "clang/Serialization/ModuleFile.h"
 #include "clang/Serialization/ModuleManager.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
@@ -184,8 +185,10 @@
 
 void RewriteMacrosAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
-  std::unique_ptr OS =
-  CI.createDefaultOutputFile(/*Binary=*/false, 
getCurrentFileOrBufferName());
+  // On Windows, set binary mode to avoid CRLF translation.
+  std::unique_ptr OS = CI.createDefaultOutputFile(
+  /*Binary=*/llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows(),
+  getCurrentFileOrBufferName());
   if (!OS) return;
 
   RewriteMacrosInInput(CI.getPreprocessor(), OS.get());
@@ -193,8 +196,10 @@
 
 void RewriteTestAction::ExecuteAction() {
   CompilerInstance  = getCompilerInstance();
-  std::unique_ptr OS =
-  CI.createDefaultOutputFile(false, getCurrentFileOrBufferName());
+  // On Windows, set binary mode to avoid CRLF translation.
+  std::unique_ptr OS = CI.createDefaultOutputFile(
+  /*Binary=*/llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows(),
+  getCurrentFileOrBufferName());
   if (!OS) return;
 
   DoRewriteTest(CI.getPreprocessor(), OS.get());


Index: llvm/lib/TableGen/Main.cpp
===
--- llvm/lib/TableGen/Main.cpp
+++ llvm/lib/TableGen/Main.cpp
@@ -17,6 +17,7 @@
 #include "llvm/TableGen/Main.h"
 #include "TGParser.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -90,10 +91,13 @@
 Records.startPhaseTiming();
 
   // Parse the input file.
+  // On Windows, set binary mode to turn off CRLF translation.
+  bool IsWindows = llvm::Triple(LLVM_HOST_TRIPLE).isOSWindows();
 
   Records.startTimer("Parse, build records");
   ErrorOr> FileOrErr =
-  MemoryBuffer::getFileOrSTDIN(InputFilename, /*IsText=*/true);
+  MemoryBuffer::getFileOrSTDIN(InputFilename,
+   IsWindows ? /*IsText=*/false : true);
   if (std::error_code EC = FileOrErr.getError())
 return reportError(argv0, "Could not open input file '" + InputFilename +
   "': " + EC.message() + "\n");
@@ -137,8 +141,8 @@
 // Only updates the real output file if there are any differences.
 // This prevents recompilation of all