Page MenuHomePhabricator

[SystemZ][z/OS] Distinguish between text and binary files on z/OS
ClosedPublic

Authored by abhina.sreeskantharajan on Mar 2 2021, 10:27 AM.

Details

Summary

This patch consists of the initial changes to help distinguish between text and binary content correctly on z/OS. I would like to get feedback from Windows users on setting OF_None for all ToolOutputFiles. This seems to have been done as an optimization to prevent CRLF translation on Windows in the past.

Diff Detail

Event Timeline

abhina.sreeskantharajan requested review of this revision.Mar 2 2021, 10:27 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 2 2021, 10:27 AM

Remove some changes that cause lit failures.

abhina.sreeskantharajan edited the summary of this revision. (Show Details)Mar 4 2021, 10:34 AM
rnk added a comment.Mar 4 2021, 11:28 AM

Yes, we took steps to suppress CRLF generation. Part of that was to avoid Windows-only test failures from trailing CR whitespace interacting with tools like diff.

From a Windows point of view, this LGTM.

clang/lib/Frontend/FrontendActions.cpp
840

The preceding block that detects the type of line separator seems ripe for factoring out into a separate function. It's a lot of low-level detail that visually outweighs the primary intent of this method.

But I'm fine with the change as it doesn't impact Windows and--as far as I understand--Posix platforms don't distinguish between text and binary mode.

clang/lib/Frontend/FrontendActions.cpp
840

Right, my intention was not to change the current behaviour, but only limit it to Windows which is the only affected platform.

If there are no concerns about setting OF_None flag for all ToolOutputFiles on Windows only, I would also like to get your reviews for this patch

zibi added a comment.Mon, Mar 15, 9:29 AM

LGTM, I just wonder if we can make an extra parameter to be default. I notice some places that is a default parameter but not in all instances. With default parameter some of the calls might be simplified if there is no need to override it.

clang/lib/Frontend/FrontendActions.cpp
812

If we don't need Triple anywhere else except here we can remove the new for local variable and do crate it as part of the test.

if (HostTriple(LLVM_HOST_TRIPLE).isOSWindows()) {
...

or even

bool BinaryMode = (HostTriple(LLVM_HOST_TRIPLE).isOSWindows())  ? true : false;
llvm/lib/Support/ToolOutputFile.cpp
50

Can we avoid creating HostTriple here?

Removing llvm::Triple variables

abhina.sreeskantharajan marked 2 inline comments as done.Mon, Mar 15, 11:49 AM

LGTM, I just wonder if we can make an extra parameter to be default. I notice some places that is a default parameter but not in all instances. With default parameter some of the calls might be simplified if there is no need to override it.

Thanks for reviewing. Do you have an example of which function is missing a default? The functions I modified in the .h files all have defaults to OF_None or binary. I can make another patch for additional changes.

zibi added inline comments.Mon, Mar 15, 12:20 PM
clang/lib/Frontend/CompilerInstance.cpp
819

The llvm::sys::fs::all_read | llvm::sys::fs::all_write seems to be a default so if we make that parameter last we won't need to pass it and worry about the mode parameter which would be the second last default parameter. Switch parameters only when you determine that indeed tag is more frequent parameter which need to be set comparing to mode parameter.

Sorry Zibi, I misunderstood your comment. I updated the patch to switch around the Flag and Mode defaults in createUniqueFile because the flags was set explicitly more often than mode. I also had to reorder the args in createUniqueEntity to match the ordering.

abhina.sreeskantharajan marked an inline comment as done.Tue, Mar 16, 5:31 AM
abhina.sreeskantharajan added inline comments.
clang/lib/Frontend/CompilerInstance.cpp
819

Thanks, I've reordered the args as you suggested.

abhina.sreeskantharajan marked an inline comment as done.Tue, Mar 16, 5:31 AM
zibi accepted this revision.Tue, Mar 16, 6:35 AM

LTGM, thx for an extra mile, Abhina.

This revision is now accepted and ready to land.Tue, Mar 16, 6:35 AM

NFC edit: Fix formatting/lint

This revision was landed with ongoing or failed builds.Fri, Mar 19, 5:10 AM
This revision was automatically updated to reflect the committed changes.

The recommended named parameter form is probably /*Param=*/something. It is liked by both https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html and clang-format.
Another form look to me as well, if both the two tools like it.

The recommended named parameter form is probably /*Param=*/something. It is liked by both https://clang.llvm.org/extra/clang-tidy/checks/bugprone-argument-comment.html and clang-format.
Another form look to me as well, if both the two tools like it.

Thanks, I've created the following patch https://reviews.llvm.org/D99072 to fix the parameter formatting.

Hi Abhina,

I think this patch broke Windows on ARM bots, the first failure observed 6
days ago in this build:

https://lab.llvm.org/buildbot/#/builders/65/builds/1245

I don't really get what the problem is, but the issue is related to
tablegen generated file AbstractTypeReader.inc and your patch is the only
one in the set which is touching that area.

Does it ring a bell for you ?

Cheers,
Yvan

Hi Abhina,

I think this patch broke Windows on ARM bots, the first failure observed 6
days ago in this build:

https://lab.llvm.org/buildbot/#/builders/65/builds/1245

I don't really get what the problem is, but the issue is related to
tablegen generated file AbstractTypeReader.inc and your patch is the only
one in the set which is touching that area.

Does it ring a bell for you ?

Cheers,
Yvan

Hi, thanks for bringing this to my attention. Since only Windows seems to have this error, it may be caused by my patch. I suspect that setting OF_None flag/Binary mode for Windows in getFile() should fix it if that is the case. I'll work on a patch to hopefully fix the issue.

When changing an IO stream's mode from binary to text breaks only Windows, it's likely due to the fact that Windows uses CR+LF as the newline indicator. Not only are the CRs sometimes unexpected, they can also throw off code that tries to seek to a computed file position.

When changing an IO stream's mode from binary to text breaks only Windows, it's likely due to the fact that Windows uses CR+LF as the newline indicator. Not only are the CRs sometimes unexpected, they can also throw off code that tries to seek to a computed file position.

Yes, this is probably the cause. I created a patch to set Binary mode on Windows only https://reviews.llvm.org/D99363 which should workaround the issue and unblock the bot. I do plan to investigate a better fix.