[Support] Add getFileOrSTDIN which calls a callback if stdin is a tty

[abrachet]

Summary

Adds an overload of MemoryBuffer::getFileOrSTDIN which takes a callback ( void(*)(void) ) which is called if Filename is "-" and stdin has not had a file or pipe redirected to it, and exiting with return code 1. This is useful for programs which wish to read from stdin, but where it doesn't make sense to read from a character device. These programs can pass a callback to getFileOrSTDIN to do something like print help message.

Notably, although cl::printHelpMessage has two optional booleans, it cannot be converted to void(void). It's pretty awkward considering having inline void PrintHelpMessage() { PrintHelpMessage(); } will upset the compiler, but it cannot convert to void(void). I'm not sure what the easiest way to work around this is. Perhaps getFileOrSTDIN should not call exit on its own but let the callback do it and a PrintHelpAndExit function could be added to CommandLine.h. cl::printHelpMessage is an obvious candidate for this use case.

This patch depends on D63858 and uses sys::fs::is_tty in it currently.

[abrachet]

llvm/unittests/Support/MemoryBufferTest.cpp

311

This test is actually failing, it doesn't change the int, but it does print "message" according to EXPECT_EXIT, I also tried other things like calling exit from inside the lamda with different exit status, and and it picks up that it wasn't exit code 1. So the ChangeFunc's operator() is being called, but TestForChange is still equal to 0 gtest says. I hope its just a silly mistake with the lambda, but surely the compiler would have complained. I'm really at a loss for this one. Any ideas?

[jhenderson]

llvm/include/llvm/Support/MemoryBuffer.h

130

ie -> i.e.

134

Do you actually need an overload? Can this just be added as a default parameter for the other getFileOrSTDIN?

llvm/lib/Support/MemoryBuffer.cpp

163

This shoudn't be calling std::exit. Either it should always be an error if the code gets here (in which case TtyStdin return an error code), or you need to have TtyStdin return a valid MemoryBuffer. Either way, the lambda signature is wrong.

If TtyStdin is intended to handle errors, then it needs renaming to clearly indicate this. Perhaps giving it a name indicating when it is used would make it clearer.

llvm/unittests/Support/MemoryBufferTest.cpp

19

Have you tried this on Windows. I suspect you'll find it won't work as is.

[abrachet]

Changed lambdas return type.

[abrachet]

llvm/include/llvm/Support/MemoryBuffer.h

134

I think so. The alternative is to put it before FileSize, which isn't great because a caller might actually be specifying this somewhere, and at the end is worse if they need to specify FileSize and RequiresNullTerminator.

llvm/unittests/Support/MemoryBufferTest.cpp

19

I'm guessing there is more to this than dup2 being called _dup2 and defined in io.h. I'll look into testing this on Windows. Also probably I need to #define dup2 not declare it. dup2 should be defined in io.h, but Microsoft say it is deprecated. I'll change this once I have a better understanding of how Windows file redirection works.

[abrachet]

Use #define dup2 _dup2 instead of auto dup2 = _dup2, which would not have worked. Also temporarily comment out testing if the lambda changes a local variable.

[jhenderson]

I've made more comments, but in researching dup2 options, I stumbled on StandardInIsUserInput which might make this whole function redundant. Does it satisfy your use-case?

llvm/include/llvm/Support/MemoryBuffer.h

131

"return its result." (It could be a nullptr for example).

134

But can't the people who want to call this new overload just explicitly specify the FileSize and RequiresNullTerminator fields, and then provide the callback as the final argument? Other call sites would be completely unaffected.

llvm/lib/Support/MemoryBuffer.cpp

143

If you really feel there is a need for a second overload, this version should call the new version, with a do-nothing function of some kind.

155

How about OnUnredirectedInput?

llvm/unittests/Support/MemoryBufferTest.cpp

311

Does this need to be EXPECT_EXIT with your updated version?

I suspect the problem previously was the call of std::exit - I think EXPECT_EXIT spawns a separate process to run the test, which means that the state used in the lambda will be unrelated to that used in the code under test.

[abrachet]

I stumbled on StandardInIsUserInput which might make this whole function redundant

Ah thanks James, can't believe I didn't find that before. Do you mean this function getFileOrSTDIN with the callback, or sys::fs::is_tty? For is_tty, yes, but I'm not sure for getFileOrSTDIN. I will ask on the list I originally started this discussion on if the tools calling if (StandardInIsUserInput) is better than doing this through getFileOrSTDIN, it might be, actually cause not all such tools will use this. Good suggestion.

Also, I found this function RedirectIO here for Windows and Unix they are both static and actually have different signatures, actually. But I bring it up because the Windows version uses DuplicateHandle. I'm guessing this test worked on Windows though or you would have said something. But I could use this if it is preferable to dup2.

[jhenderson]

In D63859#1570505, @abrachet wrote:

I stumbled on StandardInIsUserInput which might make this whole function redundant

Ah thanks James, can't believe I didn't find that before. Do you mean this function getFileOrSTDIN with the callback, or sys::fs::is_tty? For is_tty, yes, but I'm not sure for getFileOrSTDIN. I will ask on the list I originally started this discussion on if the tools calling if (StandardInIsUserInput) is better than doing this through getFileOrSTDIN, it might be, actually cause not all such tools will use this. Good suggestion.

It really depends on your exact use-case, and it's somewhat hard to say which is the right thing to do based on that. Personally, I think you could do if (StandardInIsUserInput) in the caller, and then call getFileorSTDIN as appropriate afterwards.

Also, I found this function RedirectIO here for Windows and Unix they are both static and actually have different signatures, actually. But I bring it up because the Windows version uses DuplicateHandle. I'm guessing this test worked on Windows though or you would have said something. But I could use this if it is preferable to dup2.

It's probably preferable to use existing things rather than try to use the low-level APIs like dup2 that are not particularly portable.

[MaskRay]

I am a bit dubious about the change. Why is a callback needed? Doesn't performing operations in the call sites meet your needs?

[abrachet]

I am a bit dubious about the change. Why is a callback needed? Doesn't performing operations in the call sites meet your needs?

Yeah I think you two are right. It is better perform that check outside of this function when its needed.

llvm/include/llvm/Support/MemoryBuffer.h

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CBindingWrapping.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include <cstddef>
 #include <cstdint>
+#include <functional>
 #include <memory>
 
 namespace llvm {
 
 class MemoryBufferRef;
 
 /// This interface provides simple read-only access to a block of memory, and
 /// provides simple methods for reading files and standard input into a memory
public:
   static ErrorOr<std::unique_ptr<MemoryBuffer>> getSTDIN();
 
   /// Open the specified file as a MemoryBuffer, or open stdin if the Filename
   /// is "-".
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFileOrSTDIN(const Twine &Filename, int64_t FileSize = -1,
                  bool RequiresNullTerminator = true);
 
+  /// If Filename is "-" and stdin refers to a terminal, i.e. input has not been
+  /// redirected in, call NotRedirectedInput and return its MemoryBuffer.
+  /// Otherwise acts the same as
+  /// MemoryBuffer::getFileOrSTDIN(const Twine &, int64_t, bool).
+  static ErrorOr<std::unique_ptr<MemoryBuffer>> getFileOrSTDIN(
+      const Twine &Filename,
+      std::function<std::unique_ptr<MemoryBuffer>(void)> NotRedirectedInput,
+      int64_t FileSize = -1, bool RequiresNullTerminator = true);
+
   /// Map a subrange of the specified file as a MemoryBuffer.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFileSlice(const Twine &Filename, uint64_t MapSize, uint64_t Offset,
                bool IsVolatile = false);
 
   //===--------------------------------------------------------------------===//
   // Provided for performance analysis.
   //===--------------------------------------------------------------------===//

llvm/lib/Support/MemoryBuffer.cpp

 MemoryBuffer::getMemBufferCopy(StringRef InputData, const Twine &BufferName) {
   auto Buf = getMemBufferCopyImpl(InputData, BufferName);
   if (Buf)
     return std::move(*Buf);
   return nullptr;
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFileOrSTDIN(const Twine &Filename, int64_t FileSize,
                              bool RequiresNullTerminator) {
   SmallString<256> NameBuf;
   StringRef NameRef = Filename.toStringRef(NameBuf);
 
   if (NameRef == "-")
     return getSTDIN();
   return getFile(Filename, FileSize, RequiresNullTerminator);
 }
 
+ErrorOr<std::unique_ptr<MemoryBuffer>> MemoryBuffer::getFileOrSTDIN(
+    const Twine &Filename,
+    std::function<std::unique_ptr<MemoryBuffer>(void)> NotRedirectedInput,
+    int64_t FileSize, bool RequiresNullTerminator) {
+  SmallString<256> NameBuf;
+  StringRef NameRef = Filename.toStringRef(NameBuf);
+
+  if (NameRef == "-") {
+    if (sys::fs::is_tty(0))
+      return NotRedirectedInput();
+    return getSTDIN();
+  }
+  return getFile(Filename, FileSize, RequiresNullTerminator);
+}
+
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFileSlice(const Twine &FilePath, uint64_t MapSize,
                            uint64_t Offset, bool IsVolatile) {
   return getFileAux<MemoryBuffer>(FilePath, -1, MapSize, Offset, false,
                                   IsVolatile);
 }
 
 //===----------------------------------------------------------------------===//

llvm/unittests/Support/MemoryBufferTest.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
+#include <cstdlib>
+#include <memory>
+#ifdef _WIN32
+#include <io.h>
+#define dup2 _dup2;
+#else
+#include <unistd.h>
+#endif
 
 using namespace llvm;
 
 namespace {
 
 class MemoryBufferTest : public testing::Test {
 protected:
   MemoryBufferTest()
TEST_F(MemoryBufferTest, writeThroughFile) {
   }
 
   auto MBOrError = MemoryBuffer::getFile(TestPath);
   ASSERT_FALSE(MBOrError.getError());
   auto &MB = **MBOrError;
   ASSERT_EQ(16u, MB.getBufferSize());
   EXPECT_EQ("xxxxxxxxxxxxxxxx", MB.getBuffer());
 }
+
+TEST_F(MemoryBufferTest, getFileOrSTDIN) {
+  int TestForChange = 0;
+  auto ChangeFunc = [&TestForChange]() -> std::unique_ptr<MemoryBuffer> {
+    errs() << "message";
+    // TestForChange = 1;
+    std::exit(1);
+    return nullptr;
+  };
+  // See if stdin is a tty, otherwise skip this test.
+  if (sys::fs::is_tty(0)) {
+    EXPECT_EXIT(MemoryBuffer::getFileOrSTDIN("-", ChangeFunc),
+                ::testing::ExitedWithCode(1), "message")
+        << "Reading from tty should have exited with exit code 1";
+    // Currently the ChangeFunc isn't chaning TestForChange, I don't know why
+    // ASSERT_EQ(TestForChange, 1) << "Callback not called";
+    TestForChange = 0;
+  }
+
+  auto TempFile = sys::fs::TempFile::create("%%%%%%%%");
+  if (!TempFile)
+    return;
+
+  if (::dup2(TempFile->FD, 0))
+    return;
+
+  if (sys::fs::is_tty(0))
+    return;
+
+  MemoryBuffer::getFileOrSTDIN("-", ChangeFunc);
+  ASSERT_EQ(TestForChange, 0) << "Callback should not have been called";
+
+  // Need to check Error, also need to do something in if or compiler will warn.
+  if (Error E = TempFile->discard())
+    return;
+}
 }