[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-02 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

In D99426#2664883 , 
@abhina.sreeskantharajan wrote:

> In D99426#2664841 , @MaskRay wrote:
>
>>   /// The file should be opened in text mode on platforms like z/OS that make
>>   /// this distinction.
>>   OF_Text = 1,
>>   F_Text = 1, // For compatibility
>>   
>>   /// The file should use a carriage linefeed '\r\n'.
>>   /// Only makes a difference on windows.
>>   OF_CRLF = 2,
>>   
>>   /// The file should be opened in text mode and use a carriage linefeed 
>> '\r\n'
>>   /// on platforms that make this distinction.
>>   OF_TextWithCRLF = OF_Text | OF_CRLF,
>>
>> `OF_TextWithCRLF` needs to say what platforms make a difference.
>>
>> Can you mention in the description for Windows and z/OS, how these flags 
>> make a difference, and how developers should use these flags for portability?
>> It's still a bit unclear to me.
>>
>> e.g. when I need to use `OF_CRLR` instead of `OF_Text`?
>
> So developers should use either the OF_Text or OF_TextWithCRLF for text files 
> and OF_None for binary files. If the developer doesn't want carriage returns 
> on Windows, they should use OF_Text, if they do want carriage returns on 
> Windows, they should use OF_TextWithCRLF.
>
> So this is the behaviour per platform with my patch:
>
> z/OS:
> OF_None: open in binary mode
> OF_Text : open in text mode
> OF_TextWithCRLF: open in text mode
>
> Windows:
> OF_None: open file with no carriage return
> OF_Text: open file with no carriage return
> OF_TextWithCRLF: open file with carriage return
>
> This is the old behaviour for comparison:
> z/OS:
> OF_None: open in binary mode
> OF_Text: open in text mode
>
> Windows:
> OF_None: open file with no carriage return
> OF_Text: open file with carriage return

Thanks for the information. LG.
Please update the summary to include the information.

If the patch also affects SystemZ, please adjust the `[Windows] ` tag in the 
subject.




Comment at: llvm/include/llvm/Support/FileSystem.h:752
+  /// Only makes a difference on windows.
+  OF_CRLF = 2,
+

Am I right that this is an implementation detail and should not be used 
directly by users (use OF_TextWithCRLF instead)?
If so, please add a comment.



Comment at: llvm/include/llvm/Support/FileSystem.h:771
+  /// Force files Atime to be updated on access. Only makes a difference on
+  /// windows.
+  OF_UpdateAtime = 32,

windows -> Windows


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-02 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added a comment.

In D99426#2665361 , @aganea wrote:

> I am still concerned by the fact that this patch doesn't fix the issue 
> mentionned in https://reviews.llvm.org/D96363#2650460
> Was the intention to fix that issue? Will the fix be done in a subsequent 
> patch?

I was fairly confident that if https://reviews.llvm.org/D96363 was the patch 
that was causing the issue for you, then this patch would have fixed the 
problem. I've already reverted the change in 
clang/lib/Frontend/Rewrite/FrontendActions.cpp and you mentioned that reverting 
the change in clang/lib/Driver/Driver.cpp didn't resolve the issue either. This 
patch should turn off CRLF translation in the remaining 2 files modified in 
that patch (Driver.cpp and DwarfLinkerForBinary.cpp). Since this doesn't seem 
to fix your problem, I'll need to do further investigation into which patch 
actually does cause the issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-02 Thread Alexandre Ganea via Phabricator via lldb-commits 
aganea added a comment.

I am still concerned by the fact that this patch doesn't fix the issue 
mentionned in https://reviews.llvm.org/D96363#2650460
Was the intention to fix that issue? Will the fix be done in a subsequent patch?




Comment at: llvm/lib/Support/Windows/Path.inc:1087
+  if (Flags & OF_CRLF) {
+assert(Flags & OF_Text, "Flags set OF_CRLF without OF_Text");
 CrtOpenFlags |= _O_TEXT;

`s/,/ &&/`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan updated this revision to Diff 334792.
abhina.sreeskantharajan added a comment.

Update comment in FileSystem.h for OF_TextWithCRLF


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

Files:
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/modularize/ModuleAssistant.cpp
  clang-tools-extra/pp-trace/PPTrace.cpp
  clang/lib/ARCMigrate/PlistReporter.cpp
  clang/lib/Driver/Compilation.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/DependencyGraph.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/driver/cc1as_main.cpp
  flang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/DriverUtils.cpp
  lld/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Utility/GDBRemote.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/tools/lldb-server/LLDBServerUtilities.cpp
  llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/CodeGen/RegAllocPBQP.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/ProfileData/GCOV.cpp
  llvm/lib/ProfileData/SampleProfWriter.cpp
  llvm/lib/Support/FileCollector.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/Unix/Program.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-cxxmap/llvm-cxxmap.cpp
  llvm/tools/llvm-dis/llvm-dis.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
  llvm/tools/llvm-link/llvm-link.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-opt-report/OptReport.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-xray/xray-account.cpp
  llvm/tools/llvm-xray/xray-converter.cpp
  llvm/tools/llvm-xray/xray-extract.cpp
  llvm/tools/llvm-xray/xray-graph-diff.cpp
  llvm/tools/llvm-xray/xray-graph.cpp
  llvm/tools/opt/opt.cpp
  llvm/tools/verify-uselistorder/verify-uselistorder.cpp
  llvm/unittests/Support/Path.cpp
  polly/lib/Exchange/JSONExporter.cpp

Index: polly/lib/Exchange/JSONExporter.cpp
===
--- polly/lib/Exchange/JSONExporter.cpp
+++ polly/lib/Exchange/JSONExporter.cpp
@@ -178,7 +178,7 @@
 
   // Write to file.
   std::error_code EC;
-  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_Text);
+  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_TextWithCRLF);
 
   std::string FunctionName = S.getFunction().getName().str();
   errs() << "Writing JScop '" << S.getNameStr() << "' in function '"
Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1253,7 +1253,7 @@
   path::append(FilePathname, "test");
 
   {
-raw_fd_ostream File(FilePathname, EC, sys::fs::OF_Text);
+raw_fd_ostream File(FilePathname, EC, sys::fs::OF_TextWithCRLF);
 ASSERT_NO_ERROR(EC);
 File << '\n';
   }
Index: llvm/tools/verify-uselistorder/verify-uselistorder.cpp
===
--- llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++ llvm/tools/verify-uselistorder/verify-uselistorder.cpp
@@ -136,7 +136,7 @@
 bool TempFile::writeAssembly(const Module ) const {
   LLVM_DEBUG(dbgs() << " - write assembly\n");
   std::error_code EC;
-  raw_fd_ostream OS(Filename, EC, sys::fs::OF_Text);
+  raw_fd_ostream OS(Filename, EC, sys::fs::OF_TextWithCRLF);
   if (EC) {
 errs() << "verify-uselistorder: error: " << EC.message() << "\n";
 return true;
Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -700,8 +700,8 @@
   OutputFilename = "-";
 
 std::error_code EC;
-sys::fs::OpenFlags Flags = OutputAssembly ? sys::fs::OF_Text
-  : sys::fs::OF_None;
+sys::fs::OpenFlags Flags =
+OutputAssembly ? sys::fs::OF_TextWithCRLF : sys::fs::OF_None;
 Out.reset(new ToolOutputFile(OutputFilename, EC, Flags));

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added a comment.

In D99426#2664841 , @MaskRay wrote:

>   /// The file should be opened in text mode on platforms like z/OS that make
>   /// this distinction.
>   OF_Text = 1,
>   F_Text = 1, // For compatibility
>   
>   /// The file should use a carriage linefeed '\r\n'.
>   /// Only makes a difference on windows.
>   OF_CRLF = 2,
>   
>   /// The file should be opened in text mode and use a carriage linefeed 
> '\r\n'
>   /// on platforms that make this distinction.
>   OF_TextWithCRLF = OF_Text | OF_CRLF,
>
> `OF_TextWithCRLF` needs to say what platforms make a difference.
>
> Can you mention in the description for Windows and z/OS, how these flags make 
> a difference, and how developers should use these flags for portability?
> It's still a bit unclear to me.
>
> e.g. when I need to use `OF_CRLR` instead of `OF_Text`?

So developers should use either the OF_Text or OF_TextWithCRLF for text files 
and OF_None for binary files. If the developer doesn't want carriage returns on 
Windows, they should use OF_Text, if they do want carriage returns on Windows, 
they should use OF_TextWithCRLF.

So this is the behaviour per platform with my patch:

z/OS:
OF_None: open in binary mode
OF_Text : open in text mode
OF_TextWithCRLF: open in text mode

Windows:
OF_None: open file with no carriage return
OF_Text: open file with no carriage return
OF_TextWithCRLF: open file with carriage return

This is the old behaviour for comparison:
z/OS:
OF_None: open in binary mode
OF_Text: open in text mode

Windows:
OF_None: open file with no carriage return
OF_Text: open file with carriage return


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added a comment.

@MaskRay is there still any confusion about the problem this patch is trying to 
solve and concerns about the renaming?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay added a comment.

  /// The file should be opened in text mode on platforms like z/OS that make
  /// this distinction.
  OF_Text = 1,
  F_Text = 1, // For compatibility
  
  /// The file should use a carriage linefeed '\r\n'.
  /// Only makes a difference on windows.
  OF_CRLF = 2,
  
  /// The file should be opened in text mode and use a carriage linefeed '\r\n'
  /// on platforms that make this distinction.
  OF_TextWithCRLF = OF_Text | OF_CRLF,

`OF_TextWithCRLF` needs to say what platforms make a difference.

Can you mention in the description for Windows and z/OS, how these flags make a 
difference.
It's still a bit unclear to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan updated this revision to Diff 334220.
abhina.sreeskantharajan added a comment.

Specified z/OS in FileSystem.h comments, added an assertion in Window's Path.inc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

Files:
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/modularize/ModuleAssistant.cpp
  clang-tools-extra/pp-trace/PPTrace.cpp
  clang/lib/ARCMigrate/PlistReporter.cpp
  clang/lib/Driver/Compilation.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/DependencyGraph.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/driver/cc1as_main.cpp
  flang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/DriverUtils.cpp
  lld/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Utility/GDBRemote.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/tools/lldb-server/LLDBServerUtilities.cpp
  llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/CodeGen/RegAllocPBQP.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/ProfileData/GCOV.cpp
  llvm/lib/ProfileData/SampleProfWriter.cpp
  llvm/lib/Support/FileCollector.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/Unix/Program.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-cxxmap/llvm-cxxmap.cpp
  llvm/tools/llvm-dis/llvm-dis.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
  llvm/tools/llvm-link/llvm-link.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-opt-report/OptReport.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-xray/xray-account.cpp
  llvm/tools/llvm-xray/xray-converter.cpp
  llvm/tools/llvm-xray/xray-extract.cpp
  llvm/tools/llvm-xray/xray-graph-diff.cpp
  llvm/tools/llvm-xray/xray-graph.cpp
  llvm/tools/opt/opt.cpp
  llvm/tools/verify-uselistorder/verify-uselistorder.cpp
  llvm/unittests/Support/Path.cpp
  polly/lib/Exchange/JSONExporter.cpp

Index: polly/lib/Exchange/JSONExporter.cpp
===
--- polly/lib/Exchange/JSONExporter.cpp
+++ polly/lib/Exchange/JSONExporter.cpp
@@ -178,7 +178,7 @@
 
   // Write to file.
   std::error_code EC;
-  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_Text);
+  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_TextWithCRLF);
 
   std::string FunctionName = S.getFunction().getName().str();
   errs() << "Writing JScop '" << S.getNameStr() << "' in function '"
Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1253,7 +1253,7 @@
   path::append(FilePathname, "test");
 
   {
-raw_fd_ostream File(FilePathname, EC, sys::fs::OF_Text);
+raw_fd_ostream File(FilePathname, EC, sys::fs::OF_TextWithCRLF);
 ASSERT_NO_ERROR(EC);
 File << '\n';
   }
Index: llvm/tools/verify-uselistorder/verify-uselistorder.cpp
===
--- llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++ llvm/tools/verify-uselistorder/verify-uselistorder.cpp
@@ -136,7 +136,7 @@
 bool TempFile::writeAssembly(const Module ) const {
   LLVM_DEBUG(dbgs() << " - write assembly\n");
   std::error_code EC;
-  raw_fd_ostream OS(Filename, EC, sys::fs::OF_Text);
+  raw_fd_ostream OS(Filename, EC, sys::fs::OF_TextWithCRLF);
   if (EC) {
 errs() << "verify-uselistorder: error: " << EC.message() << "\n";
 return true;
Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -700,8 +700,8 @@
   OutputFilename = "-";
 
 std::error_code EC;
-sys::fs::OpenFlags Flags = OutputAssembly ? sys::fs::OF_Text
-  : sys::fs::OF_None;
+sys::fs::OpenFlags Flags =
+OutputAssembly ? sys::fs::OF_TextWithCRLF : sys::fs::OF_None;
 Out.reset(new

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan marked 2 inline comments as done.
abhina.sreeskantharajan added inline comments.



Comment at: llvm/include/llvm/Support/FileSystem.h:757
+  /// OF_TextWithCRLF = OF_Text | OF_CRLF
+  OF_TextWithCRLF = 3,
+

amccarth wrote:
> Nit:  If it were me, I'd put the `OF_Text | OF_CRLF` directly in the code (in 
> place of the `3`) instead of in the comment.
Thanks, your solution will also prevent future errors if there is any 
renumbering


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added inline comments.



Comment at: llvm/lib/Support/Windows/Path.inc:1086
 
-  if (Flags & OF_Text)
+  if (Flags & OF_CRLF)
 CrtOpenFlags |= _O_TEXT;

amccarth wrote:
> I assume it would be weird to set `OF_CRLF` without also setting `OF_Text`, 
> so this change seems right for correctness.  But maybe this Windows-only code 
> would better express the intent if it were written:
> 
> ```
> if (Flags & (OF_CRLF | OF_Text))
> ```
> 
> Another idea would be to assert if `OF_CRLF` is set but `OF_Text` is not.
> 
> Or maybe it's fine as it is.
I chose to create OF_CRLF flag because I only want Windows to turn on text mode 
for OF_TextWithCRLF and not OF_Text. 
This solution would return true even if Flags was just OF_Text. 
```
if (Flags & (OF_CRLF | OF_Text))
```

I think adding an assertion here would be a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Adrian McCarthy via Phabricator via lldb-commits 
amccarth added a comment.

I like the composability enabled by making `OF_Text` and `OF_CRLF` independent. 
 I wonder, though, if there's a chokepoint where we could/should assert if the 
caller uses `OF_CRLF` without `OF_Text`, which would be suspicious.

I'm not concerned by the number of files touched, since it's mostly a 
mechanical refactor.  If anyone's worried that the mechanical changes obscure 
the behavior change, this _could_ be broken into an NFC refactor patch for the 
renaming followed by a patch that implements the behavioral distinction.  But 
I'm not going to insist on that.




Comment at: llvm/include/llvm/Support/FileSystem.h:746
   /// The file should be opened in text mode on platforms that make this
   /// distinction.
   OF_Text = 1,

Don't be shy to give examples in the comments, as they can illuminate the 
motivation for the design.

```
... on platforms, like SystemZ, that distinguish between text and binary files.
```



Comment at: llvm/include/llvm/Support/FileSystem.h:757
+  /// OF_TextWithCRLF = OF_Text | OF_CRLF
+  OF_TextWithCRLF = 3,
+

Nit:  If it were me, I'd put the `OF_Text | OF_CRLF` directly in the code (in 
place of the `3`) instead of in the comment.



Comment at: llvm/lib/Support/Windows/Path.inc:1086
 
-  if (Flags & OF_Text)
+  if (Flags & OF_CRLF)
 CrtOpenFlags |= _O_TEXT;

I assume it would be weird to set `OF_CRLF` without also setting `OF_Text`, so 
this change seems right for correctness.  But maybe this Windows-only code 
would better express the intent if it were written:

```
if (Flags & (OF_CRLF | OF_Text))
```

Another idea would be to assert if `OF_CRLF` is set but `OF_Text` is not.

Or maybe it's fine as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan updated this revision to Diff 334110.
abhina.sreeskantharajan edited the summary of this revision.
abhina.sreeskantharajan added a comment.

Set OF_Text for llvm/tools/dsymutil/DwarfLinkerForBinary.cpp instead of 
OF_TextWithCRLF. This was also another file I recently changed to text from 
binary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

Files:
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/modularize/ModuleAssistant.cpp
  clang-tools-extra/pp-trace/PPTrace.cpp
  clang/lib/ARCMigrate/PlistReporter.cpp
  clang/lib/Driver/Compilation.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/DependencyGraph.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/driver/cc1as_main.cpp
  flang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/DriverUtils.cpp
  lld/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Utility/GDBRemote.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/tools/lldb-server/LLDBServerUtilities.cpp
  llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/CodeGen/RegAllocPBQP.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/ProfileData/GCOV.cpp
  llvm/lib/ProfileData/SampleProfWriter.cpp
  llvm/lib/Support/FileCollector.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/Unix/Program.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-cxxmap/llvm-cxxmap.cpp
  llvm/tools/llvm-dis/llvm-dis.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
  llvm/tools/llvm-link/llvm-link.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-opt-report/OptReport.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-xray/xray-account.cpp
  llvm/tools/llvm-xray/xray-converter.cpp
  llvm/tools/llvm-xray/xray-extract.cpp
  llvm/tools/llvm-xray/xray-graph-diff.cpp
  llvm/tools/llvm-xray/xray-graph.cpp
  llvm/tools/opt/opt.cpp
  llvm/tools/verify-uselistorder/verify-uselistorder.cpp
  llvm/unittests/Support/Path.cpp
  polly/lib/Exchange/JSONExporter.cpp

Index: polly/lib/Exchange/JSONExporter.cpp
===
--- polly/lib/Exchange/JSONExporter.cpp
+++ polly/lib/Exchange/JSONExporter.cpp
@@ -178,7 +178,7 @@
 
   // Write to file.
   std::error_code EC;
-  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_Text);
+  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_TextWithCRLF);
 
   std::string FunctionName = S.getFunction().getName().str();
   errs() << "Writing JScop '" << S.getNameStr() << "' in function '"
Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1253,7 +1253,7 @@
   path::append(FilePathname, "test");
 
   {
-raw_fd_ostream File(FilePathname, EC, sys::fs::OF_Text);
+raw_fd_ostream File(FilePathname, EC, sys::fs::OF_TextWithCRLF);
 ASSERT_NO_ERROR(EC);
 File << '\n';
   }
Index: llvm/tools/verify-uselistorder/verify-uselistorder.cpp
===
--- llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++ llvm/tools/verify-uselistorder/verify-uselistorder.cpp
@@ -136,7 +136,7 @@
 bool TempFile::writeAssembly(const Module ) const {
   LLVM_DEBUG(dbgs() << " - write assembly\n");
   std::error_code EC;
-  raw_fd_ostream OS(Filename, EC, sys::fs::OF_Text);
+  raw_fd_ostream OS(Filename, EC, sys::fs::OF_TextWithCRLF);
   if (EC) {
 errs() << "verify-uselistorder: error: " << EC.message() << "\n";
 return true;
Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -700,8 +700,8 @@
   OutputFilename = "-";
 
 std::error_code EC;
-sys::fs::OpenFlags Flags = OutputAssembly ? sys::fs::OF_Text
-  :

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added a comment.

In D99426#2656582 , @MaskRay wrote:

> In D99426#2656217 , @rnk wrote:
>
>> In D99426#2653725 , @MaskRay wrote:
>>
>>> This touches a lot of files. I am a bit worried that it would not be easy 
>>> for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.
>>
>> Right, we need to separate text-ness for System/Z EBCDIC re-encoding from 
>> Windows CRLF translation. I think it's best to have a separate, positive 
>> CRLF bit, and to make that spelling longer than OF_Text. I think, in 
>> general, more problems are caused by extra unintended CRLF than by missing 
>> carriage returns.
>
> OK.
>
>> OF_CRLF which indicates that CRLF translation is used.
>> OF_TextWithCRLF = OF_Text | OF_CRLF indicates that the file is text and uses 
>> CRLF translation.
>
> My confusion came from I do not know what "CRLF translation" in the 
> description refers to. So it seems like EBCDIC->ASCII translation for CRLF? I 
> thought it was related to Windows.
>
> The current comment in the source code should be clarified too:
>
>   /// The file should be opened with CRLF translation on platforms that
>   /// make this distinction.
>   OF_CRLF = 2,
>   OF_TextWithCRLF = 3,

Sorry for causing confusion. CRLF in text files is a Windows-only problem, it 
doesn't affect z/OS. On z/OS we only need text files to be marked accurately as 
text. I updated the description and comments in FileSystem.h to be more clear.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan updated this revision to Diff 333965.
abhina.sreeskantharajan edited the summary of this revision.
abhina.sreeskantharajan added a comment.

mention OF_CRLF is Windows-only


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

Files:
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/modularize/ModuleAssistant.cpp
  clang-tools-extra/pp-trace/PPTrace.cpp
  clang/lib/ARCMigrate/PlistReporter.cpp
  clang/lib/Driver/Compilation.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/DependencyGraph.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/driver/cc1as_main.cpp
  flang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/DriverUtils.cpp
  lld/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Utility/GDBRemote.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/tools/lldb-server/LLDBServerUtilities.cpp
  llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/CodeGen/RegAllocPBQP.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/ProfileData/GCOV.cpp
  llvm/lib/ProfileData/SampleProfWriter.cpp
  llvm/lib/Support/FileCollector.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/Unix/Program.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-cxxmap/llvm-cxxmap.cpp
  llvm/tools/llvm-dis/llvm-dis.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
  llvm/tools/llvm-link/llvm-link.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-opt-report/OptReport.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-xray/xray-account.cpp
  llvm/tools/llvm-xray/xray-converter.cpp
  llvm/tools/llvm-xray/xray-extract.cpp
  llvm/tools/llvm-xray/xray-graph-diff.cpp
  llvm/tools/llvm-xray/xray-graph.cpp
  llvm/tools/opt/opt.cpp
  llvm/tools/verify-uselistorder/verify-uselistorder.cpp
  llvm/unittests/Support/Path.cpp
  polly/lib/Exchange/JSONExporter.cpp

Index: polly/lib/Exchange/JSONExporter.cpp
===
--- polly/lib/Exchange/JSONExporter.cpp
+++ polly/lib/Exchange/JSONExporter.cpp
@@ -178,7 +178,7 @@
 
   // Write to file.
   std::error_code EC;
-  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_Text);
+  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_TextWithCRLF);
 
   std::string FunctionName = S.getFunction().getName().str();
   errs() << "Writing JScop '" << S.getNameStr() << "' in function '"
Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1253,7 +1253,7 @@
   path::append(FilePathname, "test");
 
   {
-raw_fd_ostream File(FilePathname, EC, sys::fs::OF_Text);
+raw_fd_ostream File(FilePathname, EC, sys::fs::OF_TextWithCRLF);
 ASSERT_NO_ERROR(EC);
 File << '\n';
   }
Index: llvm/tools/verify-uselistorder/verify-uselistorder.cpp
===
--- llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++ llvm/tools/verify-uselistorder/verify-uselistorder.cpp
@@ -136,7 +136,7 @@
 bool TempFile::writeAssembly(const Module ) const {
   LLVM_DEBUG(dbgs() << " - write assembly\n");
   std::error_code EC;
-  raw_fd_ostream OS(Filename, EC, sys::fs::OF_Text);
+  raw_fd_ostream OS(Filename, EC, sys::fs::OF_TextWithCRLF);
   if (EC) {
 errs() << "verify-uselistorder: error: " << EC.message() << "\n";
 return true;
Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -700,8 +700,8 @@
   OutputFilename = "-";
 
 std::error_code EC;
-sys::fs::OpenFlags Flags = OutputAssembly ? sys::fs::OF_Text
-  : sys::fs::OF_None;
+sys::fs::OpenFlags Flags =
+OutputAssembly ?

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan updated this revision to Diff 333960.
abhina.sreeskantharajan edited the summary of this revision.
abhina.sreeskantharajan added a comment.

Rebase + I updated the comments in FileSystem.h to be a bit more descriptive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

Files:
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/modularize/ModuleAssistant.cpp
  clang-tools-extra/pp-trace/PPTrace.cpp
  clang/lib/ARCMigrate/PlistReporter.cpp
  clang/lib/Driver/Compilation.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/DependencyGraph.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/driver/cc1as_main.cpp
  flang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/DriverUtils.cpp
  lld/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Utility/GDBRemote.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/tools/lldb-server/LLDBServerUtilities.cpp
  llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/CodeGen/RegAllocPBQP.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/ProfileData/GCOV.cpp
  llvm/lib/ProfileData/SampleProfWriter.cpp
  llvm/lib/Support/FileCollector.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/Unix/Program.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-cxxmap/llvm-cxxmap.cpp
  llvm/tools/llvm-dis/llvm-dis.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
  llvm/tools/llvm-link/llvm-link.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-opt-report/OptReport.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-xray/xray-account.cpp
  llvm/tools/llvm-xray/xray-converter.cpp
  llvm/tools/llvm-xray/xray-extract.cpp
  llvm/tools/llvm-xray/xray-graph-diff.cpp
  llvm/tools/llvm-xray/xray-graph.cpp
  llvm/tools/opt/opt.cpp
  llvm/tools/verify-uselistorder/verify-uselistorder.cpp
  llvm/unittests/Support/Path.cpp
  polly/lib/Exchange/JSONExporter.cpp

Index: polly/lib/Exchange/JSONExporter.cpp
===
--- polly/lib/Exchange/JSONExporter.cpp
+++ polly/lib/Exchange/JSONExporter.cpp
@@ -178,7 +178,7 @@
 
   // Write to file.
   std::error_code EC;
-  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_Text);
+  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_TextWithCRLF);
 
   std::string FunctionName = S.getFunction().getName().str();
   errs() << "Writing JScop '" << S.getNameStr() << "' in function '"
Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1253,7 +1253,7 @@
   path::append(FilePathname, "test");
 
   {
-raw_fd_ostream File(FilePathname, EC, sys::fs::OF_Text);
+raw_fd_ostream File(FilePathname, EC, sys::fs::OF_TextWithCRLF);
 ASSERT_NO_ERROR(EC);
 File << '\n';
   }
Index: llvm/tools/verify-uselistorder/verify-uselistorder.cpp
===
--- llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++ llvm/tools/verify-uselistorder/verify-uselistorder.cpp
@@ -136,7 +136,7 @@
 bool TempFile::writeAssembly(const Module ) const {
   LLVM_DEBUG(dbgs() << " - write assembly\n");
   std::error_code EC;
-  raw_fd_ostream OS(Filename, EC, sys::fs::OF_Text);
+  raw_fd_ostream OS(Filename, EC, sys::fs::OF_TextWithCRLF);
   if (EC) {
 errs() << "verify-uselistorder: error: " << EC.message() << "\n";
 return true;
Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -700,8 +700,8 @@
   OutputFilename = "-";
 
 std::error_code EC;
-sys::fs::OpenFlags Flags = OutputAssembly ? sys::fs::OF_Text
-  : sys::fs::OF_None;
+sys::fs::OpenFlags Flags =

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Reid Kleckner via Phabricator via lldb-commits 
rnk added a comment.

In D99426#2653725 , @MaskRay wrote:

> This touches a lot of files. I am a bit worried that it would not be easy for 
> a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.

Right, we need to separate text-ness for System/Z EBCDIC re-encoding from 
Windows CRLF translation. I think it's best to have a separate, positive CRLF 
bit, and to make that spelling longer than OF_Text. I think, in general, more 
problems are caused by extra unintended CRLF than by missing carriage returns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-04-01 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay added a comment.

In D99426#2656217 , @rnk wrote:

> In D99426#2653725 , @MaskRay wrote:
>
>> This touches a lot of files. I am a bit worried that it would not be easy 
>> for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.
>
> Right, we need to separate text-ness for System/Z EBCDIC re-encoding from 
> Windows CRLF translation. I think it's best to have a separate, positive CRLF 
> bit, and to make that spelling longer than OF_Text. I think, in general, more 
> problems are caused by extra unintended CRLF than by missing carriage returns.

OK.

> OF_CRLF which indicates that CRLF translation is used.
> OF_TextWithCRLF = OF_Text | OF_CRLF indicates that the file is text and uses 
> CRLF translation.

My confusion came from I do not know what "CRLF translation" in the description 
refers to. So it seems like EBCDIC->ASCII translation for CRLF? I thought it 
was related to Windows.

The current comment in the source code should be clarified too:

  /// The file should be opened with CRLF translation on platforms that
  /// make this distinction.
  OF_CRLF = 2,
  OF_TextWithCRLF = 3,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-03-29 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added a comment.

In D99426#2653949 , @MaskRay wrote:

> In D99426#2653869 , 
> @abhina.sreeskantharajan wrote:
>
>> In D99426#2653725 , @MaskRay wrote:
>>
>>> This touches a lot of files. I am a bit worried that it would not be easy 
>>> for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.
>>>
 On SystemZ we need to open text files in text mode,
>>>
>>> Why can't it be served without CRLF translation?
>>
>> So this is a quick summary of the problem we're facing: 
>> On SystemZ, we can accept both OF_Text or OF_TextWithCRLF, it makes no 
>> difference. However, there are many text files that are marked as 
>> binary/OF_None to suppress CRLF translation on Windows. (e.g. patches like 
>> this one 
>> https://reviews.llvm.org/rGe78a7a0ecddc747129512fabf4836e22d1805f00). On 
>> SystemZ we need the text files to be marked as text and have made patches in 
>> the past to do so (e.g. https://reviews.llvm.org/D67696). These efforts are 
>> conflicting with each other, so Reid suggested this solution where we create 
>> a new flag that will set text mode and turn on CRLF translation on Windows 
>> platform. Then we will be able to mark all remaining text files with OF_Text 
>> and not turn on CRLF translation for Windows.
>>
>> I agree this patch is touching a lot of files,  I can maybe do the opposite 
>> and create a flag called OF_TextWithoutCRLF. I'm open to any new solutions 
>> to this problem.
>
> Hmm. I am still confused after seeting 
> rGe78a7a0ecddc747129512fabf4836e22d1805f00 
>  (does 
> it imply that using OF_None on Windows is fine?)
> If `OF_None` is used on SystemZ, does that regress functionality?

On Windows, OF_None suppresses CRLF translation and OF_Text will allow CRLF 
translation. On SystemZ it does regress functionality by causing file encoding 
issues. z/OS uses a file's text/binary mode to determine whether to perform 
translation to the system encoding EBCDIC if needed. (On binary files, no 
translation should be performed)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-03-29 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan added a comment.

In D99426#2653725 , @MaskRay wrote:

> This touches a lot of files. I am a bit worried that it would not be easy for 
> a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.
>
>> On SystemZ we need to open text files in text mode,
>
> Why can't it be served without CRLF translation?

So this is a quick summary of the problem we're facing: 
On SystemZ, we can accept both OF_Text or OF_TextWithCRLF, it makes no 
difference. However, there are many text files that are marked as 
binary/OF_None to suppress CRLF translation on Windows. (e.g. patches like this 
one https://reviews.llvm.org/rGe78a7a0ecddc747129512fabf4836e22d1805f00). On 
SystemZ we need the text files to be marked as text and have made patches in 
the past to do so (e.g. https://reviews.llvm.org/D67696). These efforts are 
conflicting with each other, so Reid suggested this solution where we create a 
new flag that will set text mode and turn on CRLF translation on Windows 
platform. Then we will be able to mark all remaining text files with OF_Text 
and not turn on CRLF translation for Windows.

I agree this patch is touching a lot of files,  I can maybe do the opposite and 
create a flag called OF_TextWithoutCRLF. I'm open to any new solutions to this 
problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-03-29 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay added a comment.

In D99426#2653869 , 
@abhina.sreeskantharajan wrote:

> In D99426#2653725 , @MaskRay wrote:
>
>> This touches a lot of files. I am a bit worried that it would not be easy 
>> for a contributor to know OF_TextWithCRLF is needed to make SystemZ happy.
>>
>>> On SystemZ we need to open text files in text mode,
>>
>> Why can't it be served without CRLF translation?
>
> So this is a quick summary of the problem we're facing: 
> On SystemZ, we can accept both OF_Text or OF_TextWithCRLF, it makes no 
> difference. However, there are many text files that are marked as 
> binary/OF_None to suppress CRLF translation on Windows. (e.g. patches like 
> this one 
> https://reviews.llvm.org/rGe78a7a0ecddc747129512fabf4836e22d1805f00). On 
> SystemZ we need the text files to be marked as text and have made patches in 
> the past to do so (e.g. https://reviews.llvm.org/D67696). These efforts are 
> conflicting with each other, so Reid suggested this solution where we create 
> a new flag that will set text mode and turn on CRLF translation on Windows 
> platform. Then we will be able to mark all remaining text files with OF_Text 
> and not turn on CRLF translation for Windows.
>
> I agree this patch is touching a lot of files,  I can maybe do the opposite 
> and create a flag called OF_TextWithoutCRLF. I'm open to any new solutions to 
> this problem.

Hmm. I am still confused after seeting 
rGe78a7a0ecddc747129512fabf4836e22d1805f00 
 (does it 
imply that using OF_None on Windows is fine?)
If `OF_None` is used on SystemZ, does that regress functionality?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-03-29 Thread Fangrui Song via Phabricator via lldb-commits 
MaskRay added a comment.

This touches a lot of files. I am a bit worried that it would not be easy for a 
contributor to know OF_TextWithCRLF is needed to make SystemZ happy.

> On SystemZ we need to open text files in text mode,

Why can't it be served without CRLF translation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99426

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

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

2021-03-29 Thread Abhina Sree via Phabricator via lldb-commits 
abhina.sreeskantharajan created this revision.
Herald added subscribers: dexonsmith, wenlei, okura, kuter, cmtice, martong, 
steven_wu, gbedwell, hiraditya, qcolombet, MatzeB.
Herald added a reviewer: bollu.
Herald added a reviewer: JDevlieghere.
Herald added a reviewer: andreadb.
Herald added a reviewer: jhenderson.
abhina.sreeskantharajan requested review of this revision.
Herald added subscribers: llvm-commits, lldb-commits, cfe-commits, bbn, MaskRay.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: sstefan1.
Herald added a reviewer: baziotis.
Herald added projects: clang, LLDB, LLVM, clang-tools-extra.

On SystemZ we need to open text files in text mode, but on Windows this may not 
be desirable if we want to suppress CRLF translation. This patch adds two new 
flags

- OF_CRLF which indicates that CRLF translation is used.
- OF_TextWithCRLF = OF_Text | OF_CRLF indicates that the file is text and uses 
CRLF translation.

Major change in llvm/lib/Support/Windows/Path.inc to only set text mode if the 
OF_CRLF is set.

  if (Flags & OF_CRLF)
CrtOpenFlags |= _O_TEXT;




Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D99426

Files:
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/modularize/ModuleAssistant.cpp
  clang-tools-extra/pp-trace/PPTrace.cpp
  clang/lib/ARCMigrate/PlistReporter.cpp
  clang/lib/Driver/Compilation.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/DependencyFile.cpp
  clang/lib/Frontend/DependencyGraph.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Frontend/HeaderIncludeGen.cpp
  clang/lib/Frontend/ModuleDependencyCollector.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/driver/cc1as_main.cpp
  flang/lib/Frontend/CompilerInstance.cpp
  lld/COFF/DriverUtils.cpp
  lld/lib/ReaderWriter/YAML/ReaderWriterYAML.cpp
  lldb/include/lldb/Utility/ReproducerProvider.h
  lldb/source/Utility/GDBRemote.cpp
  lldb/source/Utility/ReproducerProvider.cpp
  lldb/tools/lldb-server/LLDBServerUtilities.cpp
  llvm/include/llvm/Analysis/DOTGraphTraitsPass.h
  llvm/include/llvm/Support/FileSystem.h
  llvm/lib/CodeGen/RegAllocPBQP.cpp
  llvm/lib/IR/Core.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/MC/MCParser/DarwinAsmParser.cpp
  llvm/lib/ProfileData/GCOV.cpp
  llvm/lib/ProfileData/SampleProfWriter.cpp
  llvm/lib/Support/FileCollector.cpp
  llvm/lib/Support/MemoryBuffer.cpp
  llvm/lib/Support/TimeProfiler.cpp
  llvm/lib/Support/Timer.cpp
  llvm/lib/Support/Unix/Program.inc
  llvm/lib/Support/Windows/Path.inc
  llvm/lib/Support/Windows/Program.inc
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/Utils/Debugify.cpp
  llvm/tools/dsymutil/DwarfLinkerForBinary.cpp
  llvm/tools/dsymutil/dsymutil.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-cxxmap/llvm-cxxmap.cpp
  llvm/tools/llvm-dis/llvm-dis.cpp
  llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
  llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
  llvm/tools/llvm-link/llvm-link.cpp
  llvm/tools/llvm-mc/llvm-mc.cpp
  llvm/tools/llvm-mca/llvm-mca.cpp
  llvm/tools/llvm-opt-report/OptReport.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-xray/xray-account.cpp
  llvm/tools/llvm-xray/xray-converter.cpp
  llvm/tools/llvm-xray/xray-extract.cpp
  llvm/tools/llvm-xray/xray-graph-diff.cpp
  llvm/tools/llvm-xray/xray-graph.cpp
  llvm/tools/opt/opt.cpp
  llvm/tools/verify-uselistorder/verify-uselistorder.cpp
  llvm/unittests/Support/Path.cpp
  polly/lib/Exchange/JSONExporter.cpp

Index: polly/lib/Exchange/JSONExporter.cpp
===
--- polly/lib/Exchange/JSONExporter.cpp
+++ polly/lib/Exchange/JSONExporter.cpp
@@ -178,7 +178,7 @@
 
   // Write to file.
   std::error_code EC;
-  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_Text);
+  ToolOutputFile F(FileName, EC, llvm::sys::fs::OF_TextWithCRLF);
 
   std::string FunctionName = S.getFunction().getName().str();
   errs() << "Writing JScop '" << S.getNameStr() << "' in function '"
Index: llvm/unittests/Support/Path.cpp
===
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1253,7 +1253,7 @@
   path::append(FilePathname, "test");
 
   {
-raw_fd_ostream File(FilePathname, EC, sys::fs::OF_Text);
+raw_fd_ostream File(FilePathname, EC, sys::fs::OF_TextWithCRLF);
 ASSERT_NO_ERROR(EC);
 File << '\n';
   }
Index: llvm/tools/verify-uselistorder/verify-uselistorder.cpp
===
--- llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++