[Support] Add TempFailureRetry helper function

[labath]

Summary

This function retries an operation if it was interrupted by a signal
(failed with EINTR). It's inspired by the TEMP_FAILURE_RETRY macro in
glibc, but I've turned that into a template function. I've also added a
fail-value argument, to enable the function to be used with e.g.
fopen(3), which is documented to fail for any reason that open(2) can
fail (which includes EINTR).

The main user of this function will be lldb, but there were also a
couple of uses within llvm that I could simplify using this function.

[labath]

Ping. :)

Any thoughts on this? Or suggestions for other reviewers?

[joerg]

include/llvm/Support/Errno.h

39 ↗

(On Diff #101404)

Maybe RetryAfterSignal. Is FailT as template really necessary or can you use the decltype(F(As...)) directly?

I like the approach otherwise.

[labath]

I've renamed the function.

Decltype trick will not work for the fail value because F and As... are not yet
defined at that point (although this isn't the reason why I originally added the
extra template param). It's possible to achieve it with std::result_of though.

I've replaced the other uses of decltype with result_of as I think it would be
confusing to use both.

[labath]

Is this good to go now? :)

[EricWF]

include/llvm/Support/Errno.h

32 ↗

(On Diff #102545)

The value-categories seem all messed up here. There is no reason to be using && anywhere, since all of the arguments and the function are lvalues. Personally I would write this function as:

template <typename Fun, typename... Args,
          typename ResultT = typename std::result_of<Fun const& (const Args&...)>::type>
inline ResultT RetryAfterSignal(ResultT Fail, const Fun &F, const Args &... As) {
  ResultT Res;
  do
    Res = F(As...);
  while (Res == Fail && errno == EINTR);
  return Res;
}

[labath]

Fix the function, as suggested by EricWF.

include/llvm/Support/Errno.h

32 ↗

(On Diff #102545)

Thanks, that looks much better. I'll have to remember the extra template arg trick.

Using the correct value category is still somewhat of a mystery to me.

[labath]

If there are no further comments, can I commit this?

[joerg]

Go ahead.

[labath]

Thank you.

[labath]

include/llvm/Support/Errno.h

32 ↗

(On Diff #102545)

When I started using this function, I noticed that the ResultT trick did not work after all, because the template argument deduction would override the default template value. My attempt to fix that in r306003 uncovered another issue -- libc++'s implementation of std::result_of does not handle the prototype of open(2) correctly (it's a variadic function), so I've reverted this patch until that is sorted.

At this point I don't see any other solution than to go back to the initial implementation of having a separate FailT template argument, and using decltype instead of std::result_of. Do you have a better idea?

[joerg]

Do you need someone to commit this?

[labath]

In D33895#788044, @joerg wrote:

Do you need someone to commit this?

Thanks, but I have already committed it, and then backed it out because I ran into a wall with the std::result_of idea. :)
I reopened to review to see if you'd be ok with the original implementation with FailT and decltype.

[labath]

Go back to the decltype version

llvm/trunk/include/llvm/Support/Errno.h

 // This file declares some portable and convenient functions to deal with errno.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_ERRNO_H
 #define LLVM_SUPPORT_ERRNO_H
 
 #include <string>
+#include <type_traits>
 
 namespace llvm {
 namespace sys {
 
 /// Returns a string representation of the errno value, using whatever
 /// thread-safe variant of strerror() is available.  Be sure to call this
 /// immediately after the function that set errno, or errno may have been
 /// overwritten by an intervening call.
 std::string StrError();
 
 /// Like the no-argument version above, but uses \p errnum instead of errno.
 std::string StrError(int errnum);
 
+template <typename Fun, typename... Args,
+          typename ResultT =
+              typename std::result_of<Fun const &(const Args &...)>::type>
+inline ResultT RetryAfterSignal(ResultT Fail, const Fun &F,
+                                const Args &... As) {
+  ResultT Res;
+  do
+    Res = F(As...);
+  while (Res == Fail && errno == EINTR);
+  return Res;
+}
+
 }  // namespace sys
 }  // namespace llvm
 
 #endif  // LLVM_SYSTEM_ERRNO_H

llvm/trunk/lib/Support/MemoryBuffer.cpp

 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getMemoryBufferForStream(int FD, const Twine &BufferName) {
   const ssize_t ChunkSize = 4096*4;
   SmallString<ChunkSize> Buffer;
   ssize_t ReadBytes;
   // Read into Buffer until we hit EOF.
   do {
     Buffer.reserve(Buffer.size() + ChunkSize);
-    ReadBytes = read(FD, Buffer.end(), ChunkSize);
-    if (ReadBytes == -1) {
-      if (errno == EINTR) continue;
+    ReadBytes = sys::RetryAfterSignal(-1, read, FD, Buffer.end(), ChunkSize);
+    if (ReadBytes == -1)
       return std::error_code(errno, std::generic_category());
-    }
     Buffer.set_size(Buffer.size() + ReadBytes);
   } while (ReadBytes != 0);
 
   return MemoryBuffer::getMemBufferCopy(Buffer, BufferName);
 }
 
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
getOpenFileImpl(int FD, const Twine &Filename, uint64_t FileSize,
   size_t BytesLeft = MapSize;
 #ifndef HAVE_PREAD
   if (lseek(FD, Offset, SEEK_SET) == -1)
     return std::error_code(errno, std::generic_category());
 #endif
 
   while (BytesLeft) {
 #ifdef HAVE_PREAD
-    ssize_t NumRead = ::pread(FD, BufPtr, BytesLeft, MapSize-BytesLeft+Offset);
+    ssize_t NumRead = sys::RetryAfterSignal(-1, ::pread, FD, BufPtr, BytesLeft,
+                                            MapSize - BytesLeft + Offset);
 #else
-    ssize_t NumRead = ::read(FD, BufPtr, BytesLeft);
+    ssize_t NumRead = sys::RetryAfterSignal(-1, ::read, FD, BufPtr, BytesLeft);
 #endif
     if (NumRead == -1) {
-      if (errno == EINTR)
-        continue;
       // Error while reading.
       return std::error_code(errno, std::generic_category());
     }
     if (NumRead == 0) {
       memset(BufPtr, 0, BytesLeft); // zero-initialize rest of the buffer.
       break;
     }
     BytesLeft -= NumRead;

llvm/trunk/lib/Support/Unix/Path.inc

 std::error_code openFileForRead(const Twine &Name, int &ResultFD,
                                 SmallVectorImpl<char> *RealPath) {
   SmallString<128> Storage;
   StringRef P = Name.toNullTerminatedStringRef(Storage);
   int OpenFlags = O_RDONLY;
 #ifdef O_CLOEXEC
   OpenFlags |= O_CLOEXEC;
 #endif
-  while ((ResultFD = open(P.begin(), OpenFlags)) < 0) {
-    if (errno != EINTR)
-      return std::error_code(errno, std::generic_category());
-  }
+  if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags)) < 0)
 #ifndef O_CLOEXEC
   int r = fcntl(ResultFD, F_SETFD, FD_CLOEXEC);
   (void)r;
   assert(r == 0 && "fcntl(F_SETFD, FD_CLOEXEC) failed");
 #endif
   // Attempt to get the real name of the file, if the user asked
   if(!RealPath)
     return std::error_code();
#endif
   else
     OpenFlags |= O_TRUNC;
 
   if (Flags & F_Excl)
     OpenFlags |= O_EXCL;
 
   SmallString<128> Storage;
   StringRef P = Name.toNullTerminatedStringRef(Storage);
-  while ((ResultFD = open(P.begin(), OpenFlags, Mode)) < 0) {
-    if (errno != EINTR)
-      return std::error_code(errno, std::generic_category());
-  }
+  if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags, Mode)) < 0)
 #ifndef O_CLOEXEC
   int r = fcntl(ResultFD, F_SETFD, FD_CLOEXEC);
   (void)r;
   assert(r == 0 && "fcntl(F_SETFD, FD_CLOEXEC) failed");
 #endif
   return std::error_code();
 }
 

llvm/trunk/lib/Support/Unix/Process.inc

 
 std::error_code Process::FixupStandardFileDescriptors() {
   int NullFD = -1;
   FDCloser FDC(NullFD);
   const int StandardFDs[] = {STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO};
   for (int StandardFD : StandardFDs) {
     struct stat st;
     errno = 0;
-    while (fstat(StandardFD, &st) < 0) {
+    if (RetryAfterSignal(-1, fstat, StandardFD, &st) < 0) {
       assert(errno && "expected errno to be set if fstat failed!");
       // fstat should return EBADF if the file descriptor is closed.
-      if (errno == EBADF)
-        break;
-      // retry fstat if we got EINTR, otherwise bubble up the failure.
-      if (errno != EINTR)
+      if (errno != EBADF)
         return std::error_code(errno, std::generic_category());
     }
     // if fstat succeeds, move on to the next FD.
     if (!errno)
       continue;
     assert(errno == EBADF && "expected errno to have EBADF at this point!");
 
     if (NullFD < 0) {
-      while ((NullFD = open("/dev/null", O_RDWR)) < 0) {
-        if (errno == EINTR)
-          continue;
+      if ((NullFD = RetryAfterSignal(-1, open, "/dev/null", O_RDWR)) < 0)
         return std::error_code(errno, std::generic_category());
-      }
-    }
 
     if (NullFD == StandardFD)
       FDC.keepOpen();
     else if (dup2(NullFD, StandardFD) < 0)
       return std::error_code(errno, std::generic_category());
   }
   return std::error_code();
 }

llvm/trunk/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   Chrono.cpp
   CommandLineTest.cpp
   CompressionTest.cpp
   ConvertUTFTest.cpp
   DataExtractorTest.cpp
   DebugTest.cpp
   EndianStreamTest.cpp
   EndianTest.cpp
+  ErrnoTest.cpp
   ErrorOrTest.cpp
   ErrorTest.cpp
   FileOutputBufferTest.cpp
   FormatVariadicTest.cpp
   GlobPatternTest.cpp
   Host.cpp
   LEB128Test.cpp
   LineIteratorTest.cpp

llvm/trunk/unittests/Support/ErrnoTest.cpp

+//===- ErrnoTest.cpp - Error handling unit tests --------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Errno.h"
+#include "gtest/gtest.h"
+
+using namespace llvm::sys;
+
+TEST(ErrnoTest, RetryAfterSignal) {
+  EXPECT_EQ(1, RetryAfterSignal(-1, [] { return 1; }));
+
+  EXPECT_EQ(-1, RetryAfterSignal(-1, [] {
+    errno = EAGAIN;
+    return -1;
+  }));
+  EXPECT_EQ(EAGAIN, errno);
+
+  unsigned calls = 0;
+  EXPECT_EQ(1, RetryAfterSignal(-1, [&calls] {
+              errno = EINTR;
+              ++calls;
+              return calls == 1 ? -1 : 1;
+            }));
+  EXPECT_EQ(2u, calls);
+
+  EXPECT_EQ(1, RetryAfterSignal(-1, [](int x) { return x; }, 1));
+}