This is an archive of the discontinued LLVM Phabricator instance.

[tools] Mark output of tools as text if it is really text
ClosedPublic

Authored by Kai on Sep 18 2019, 1:12 AM.

Details

Summary

Several LLVM tools write text files/streams without using OF_Text.
This can cause problems on platforms which distinguish between
text and binary output. This PR adds the OF_Text flag for the
following tools:

  • llvm-dis
  • llvm-dwarfdump
  • llvm-mca
  • llvm-mc (assembler files only)
  • opt (assembler files only)
  • RemarkStreamer (used e.g. by opt)

Most notably, llc already makes the distinction between text and binary output.

Diff Detail

Event Timeline

Kai created this revision.Sep 18 2019, 1:12 AM
thegameg added inline comments.
llvm/lib/IR/RemarkStreamer.cpp
135

Depending on RemarksFormat, the output can also be LLVM Bitstream. Should this set OF_Text only if the format is YAML?

rnk added a comment.Sep 18 2019, 10:47 AM

The main effect that I'm aware of here is that this will do CRLF conversion on Windows. Are we sure we really want that? I just did the opposite to TableGen output in rL371683. In general, I think the majority of tools that write text use OF_Text, so your change makes this minority of tools consistent with the other ones.

Kai updated this revision to Diff 220855.Sep 19 2019, 6:15 AM
Kai marked an inline comment as done.

Use OF_Text for the RemarkStreamer only if format is YAML.

Kai added a comment.Sep 19 2019, 6:37 AM
In D67696#1674254, @rnk wrote:

The main effect that I'm aware of here is that this will do CRLF conversion on Windows. Are we sure we really want that? I just did the opposite to TableGen output in rL371683. In general, I think the majority of tools that write text use OF_Text, so your change makes this minority of tools consistent with the other ones.

I am on a platform where the difference between text and binary streams is much greater than on Windows. Therefore I really appreciate the consistency.
Yes, I noted your commit. It had surprising effects...

llvm/lib/IR/RemarkStreamer.cpp
135

Ah - thanks. Of course it should only be set if the format is YAML.

Kai updated this revision to Diff 220900.Sep 19 2019, 1:01 PM

Fixed an error introduced in the previous revision.

Kai updated this revision to Diff 222134.Sep 27 2019, 5:07 AM
Kai marked an inline comment as done.
Kai edited the summary of this revision. (Show Details)
  • Add OF_Text for opt assembly output
  • Fix RemarkStreamer
  • Use OF_None instead of F_None in new code
rnk accepted this revision.Oct 3 2019, 5:11 PM

lgtm

This revision is now accepted and ready to land.Oct 3 2019, 5:11 PM

Do we really want to output \r\n on Windows? There is all of one program (notepad.exe) that doesn't support \n on Windows.

MaskRay added a subscriber: MaskRay.Oct 4 2019, 1:30 AM
In D67696#1675194, @Kai wrote:
In D67696#1674254, @rnk wrote:

The main effect that I'm aware of here is that this will do CRLF conversion on Windows. Are we sure we really want that? I just did the opposite to TableGen output in rL371683. In general, I think the majority of tools that write text use OF_Text, so your change makes this minority of tools consistent with the other ones.

I am on a platform where the difference between text and binary streams is much greater than on Windows. Therefore I really appreciate the consistency.
Yes, I noted your commit. It had surprising effects...

Can you summarize the differences on that platform? z/OS?

While b mode is allowed for fopen in ISO C, on POSIX-conforming systems:

The character 'b' shall have no effect, but is allowed for ISO C standard conformance.

This revision was automatically updated to reflect the committed changes.