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

[dgg5503]

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.

[dgg5503]

Ping.

[probinson]

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

[MaskRay]

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]

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.

[dgg5503]

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.

[MaskRay]

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]

@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]

llvm/lib/Support/raw_ostream.cpp

795

Why not just use raw_fd_ostream::is_displayed() ?

[dgg5503]

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

[MaskRay]

llvm/lib/Support/raw_ostream.cpp

795

raw_fd_ostream::is_displayed -> is_displayed

[dgg5503]

@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:

In D75278#1910426, @dgg5503 wrote:

@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]

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

This sentence of the git description needs an update.

[dgg5503]

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

[MaskRay]

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

[dgg5503]

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

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

llvm/lib/Support/raw_ostream.cpp

#elif !defined(__minix)
   assert(FD >= 0 && "File not yet open!");
   struct stat statbuf;
   if (fstat(FD, &statbuf) != 0)
     return 0;
 
   // If this is a terminal, don't use buffering. Line buffering
   // would be a more traditional thing to do, but it's not worth
   // the complexity.
-  if (S_ISCHR(statbuf.st_mode) && isatty(FD))
+  if (S_ISCHR(statbuf.st_mode) && is_displayed())
     return 0;
   // Return the preferred block size.
   return statbuf.st_blksize;
 #else
   return raw_ostream::preferred_buffer_size();
 #endif
 }