Page MenuHomePhabricator

Use FileDescriptorIsDisplayed in place of direct call to isatty in support library
ClosedPublic

Authored by dgg5503 on Feb 27 2020, 11:25 AM.

Details

Summary

Currently, when building with the Unix support library and isatty does
not exist for the target platform (i.e. HAVE_ISATTY is false),
compilation of the file raw_ostream.cpp will fail due to direct use of
isatty in the function raw_fd_ostream::preferred_buffer_size(). This
patch simply replaces the direct use of isatty with
raw_fd_ostream::is_displayed which calls a support function that abstracts
away the potentially target specific function isatty by accounting for its
existence and returning a default value.

Diff Detail

Event Timeline

dgg5503 created this revision.Feb 27 2020, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2020, 11:25 AM
probinson accepted this revision.Mar 5 2020, 10:55 AM
probinson added a subscriber: probinson.

FileDescriptorIsDisplayed() basically calls isatty() when it's present, so this should be NFC on any host with that syscall.
LGTM.

This revision is now accepted and ready to land.Mar 5 2020, 10:55 AM

System V Release 4 had isatty and isatty is in POSIX.1-2001 includes isatty. Which OS are your porting to? (Not necessarily an objection, but I am slightly concerned of we may see many future subtle changes like this.)

MaskRay requested changes to this revision.EditedMar 5 2020, 11:37 AM

At the least, the target OS should be mentioned.

A FreeBSD dev said that it's a "fix your libc" situation.

If unistd.h does not provide isatty, it will be difficult to imagine how many other things will break. Supporting an exotic OS will lay the burden on developers who maintain the file.

This revision now requires changes to proceed.Mar 5 2020, 11:37 AM

Hi @MaskRay,

At the least, the target OS should be mentioned.

This change is required for the PlayStation 4 operating system. I suppose the OS would be considered exotic so it's understandable if this change isn't desired. However, when I saw isatty being directly called in this function, it wasn't immediately obvious to me that the preprocessor block guaranteed that section of code to be POSIX compliant. This was also the only other spot in the LLVM folder that called isatty directly except for some libraries under llvm/utils and the function FileDescriptorIsDisplayed under the Unix platform for the Support lib. I figured this was simply an oversight especially since in FileDescriptorIsDisplayed, HAVE_ISATTY guards the use of isatty when not available on the target Unix-like system.

A FreeBSD dev said that it's a "fix your libc" situation.
If unistd.h does not provide isatty, it will be difficult to imagine how many other things will break. Supporting an exotic OS will lay the burden on developers who maintain the file.

I see in D75707 that you are removing HAVE_ISATTY, so I guess it was a mistake to have the guard in the Unix support library? I'd be ok waiting for the result of D75707 before continuing discussion here.

Thank you for the clarification in D75707. I hoped the summary did not beat the bush...

I know any suggestion on this topic may be belated.. but you could simply do

int isatty(int fd) {
  return 0;
}

in your libc. If you enable Identical Code Folding, it may have 0 size cost after stripping.

LGTM once you made it clear in the code and the summary.

dgg5503 added a comment.EditedMar 6 2020, 2:29 PM

@MaskRay thanks for the feedback. Before I update the summary and code with a comment, I'd like to first confirm that from our discussion about this function, are Windows, Minix, and fully POSIX compliant OS the only hosts supported by LLVM so far?

ychen added a subscriber: ychen.Mar 6 2020, 7:42 PM
ychen added inline comments.
llvm/lib/Support/raw_ostream.cpp
795

Why not just use raw_fd_ostream::is_displayed() ?

dgg5503 updated this revision to Diff 249163.Mar 9 2020, 10:50 AM

Changed sys::Process::FileDescriptorIsDisplayed(FD) to raw_fd_ostream::is_displayed().

dgg5503 marked an inline comment as done.Mar 9 2020, 12:29 PM
MaskRay accepted this revision.Mar 9 2020, 12:58 PM
MaskRay added inline comments.
llvm/lib/Support/raw_ostream.cpp
795

raw_fd_ostream::is_displayed -> is_displayed

This revision is now accepted and ready to land.Mar 9 2020, 12:58 PM
dgg5503 updated this revision to Diff 249199.Mar 9 2020, 1:21 PM
dgg5503 marked an inline comment as done.

@MaskRay thanks for the approval. I just want to make sure you saw my earlier question since I haven't yet updated the description or added a comment:

@MaskRay thanks for the feedback. Before I update the summary and code with a comment, I'd like to first confirm that from our discussion about this function, are Windows, Minix, and fully POSIX compliant OS the only hosts supported by LLVM so far?

MaskRay accepted this revision.Mar 9 2020, 3:52 PM

This patch simply replaces the direct use of isatty with sys::Process::FileDescriptorIsDisplayed

This sentence of the git description needs an update.

dgg5503 edited the summary of this revision. (Show Details)Mar 9 2020, 5:04 PM
dgg5503 edited the summary of this revision. (Show Details)Mar 9 2020, 6:48 PM

@MaskRay, is my updated description okay? If so, can someone with commit access please push this change?

It is great. Can you provide your name and email address so that your contribution can be properly attributed?

@MaskRay sure.
Name: Douglas Gliner
E-mail:

Sorry for the image, just paranoid about those web crawlers!

This revision was automatically updated to reflect the committed changes.