[Support] Add functions that return native file handles on Windows instead of fds.

[zturner]

Summary

Windows' CRT has a limit of 512 open file descriptors, and fds which are generated by converting a HANDLE via _get_osfhandle count towards this limit as well.

Regardless, often you find yourself marshalling back and forth between native HANDLE objects and fds anyway. If we know from the getgo that we're going to need to work directly with the handle, we can cut out the marshalling layer while also not contributing to filling up the CRT's very limited handle table.

On Unix these functions just delegate directly to the existing set of functions since an fd *is* the native file type. It would be nice, very long term, if we could convert most uses of fds to native_file_t.

[amccarth]

I like the direction. Just a question about the implicit type conversion.

llvm/include/llvm/Support/FileSystem.h

848 ↗

(On Diff #149649)

s/the freeing//

881 ↗

(On Diff #149649)

s/the freeing//

llvm/lib/Support/Windows/Path.inc

1070 ↗

(On Diff #149649)

Does this work? Function returns Expected<native_file_t> which is Expected<void *>. Can you assign that to Expected<HANDLE> and get the implicit (reinterpret) cast from void * to HANDLE?

[zturner]

llvm/lib/Support/Windows/Path.inc

1070 ↗

(On Diff #149649)

HANDLE and void* are literally the same type, so yes it should work. From winnt.h:

#ifdef STRICT
typedef void *HANDLE;
#else
typedef PVOID HANDLE;
#endif

[amccarth]

llvm/lib/Support/Windows/Path.inc

1070 ↗

(On Diff #149649)

Oh, duh! I should have realized that.

[rnk]

We discussed this in person, and I think the best way forward is to use this new typedef everywhere in all code. As a migration path, I think it would be best to rename the existing integer FD APIs to openFileDescriptorForRead/Write. Then we can add back openFileForRead and have it return the new handle / FD wrapper typedef. The new typedef be called file_t so we don't have to use the word "native" everywhere and we can just talk about "files".

[zturner]

Changes from first revision:

1. Renamed native_file_t -> file_t
2. Added a global constant kInvalidFile that is -1 on Unix and INVALID_FILE_HANDLE on Windows.
3. Added a closeFile(file_t &) method that calls ::close or ::CloseHandle, and then sets the parameter to kInvalidFile so it can't be re-used.

The in/out parameter of #3 might be objectionable, so if anyone has a strong opinion I can just pass by value and not modify the argument on output.

llvm/trunk/include/llvm/Support/FileSystem.h

 #ifdef HAVE_SYS_STAT_H
 #include <sys/stat.h>
 #endif
 
 namespace llvm {
 namespace sys {
 namespace fs {
 
+#if defined(_WIN32)
+// A Win32 HANDLE is a typedef of void*
+using file_t = void *;
+#else
+using file_t = int;
+#endif
+
+extern const file_t kInvalidFile;
+
 /// An enumeration for the file system's view of the type.
 enum class file_type {
   status_error,
   file_not_found,
   regular_file,
   directory_file,
   symlink_file,
   block_file,

 /// @param Flags Additional flags used to determine whether the file should be
 ///              opened in, for example, read-write or in write-only mode.
 /// @param Mode The access permissions of the file, represented in octal.
 /// @returns errc::success if \a Name has been opened, otherwise a
 ///          platform-specific error_code.
 std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
                                  OpenFlags Flags, unsigned Mode = 0666);
 
+/// @brief Opens the file with the given name in a write-only or read-write
+/// mode, returning its open file descriptor. If the file does not exist, it
+/// is created.
+///
+/// The caller is responsible for closing the freeing the file once they are
+/// finished with it.
+///
+/// @param Name The path of the file to open, relative or absolute.
+/// @param Flags Additional flags used to determine whether the file should be
+///              opened in, for example, read-write or in write-only mode.
+/// @param Mode The access permissions of the file, represented in octal.
+/// @returns a platform-specific file descriptor if \a Name has been opened,
+///          otherwise an error object.
+Expected<file_t> openNativeFileForWrite(const Twine &Name, OpenFlags Flags,
+                                        unsigned Mode = 0666);
+
 /// @brief Opens the file with the given name in a read-only mode, returning
 /// its open file descriptor.
 ///
 /// The caller is responsible for closing the file descriptor once they are
 /// finished with it.
 ///
 /// @param Name The path of the file to open, relative or absolute.
 /// @param ResultFD If the file could be opened successfully, its descriptor
 ///                 is stored in this location. Otherwise, this is set to -1.
 /// @param RealPath If nonnull, extra work is done to determine the real path
 ///                 of the opened file, and that path is stored in this
 ///                 location.
 /// @returns errc::success if \a Name has been opened, otherwise a
 ///          platform-specific error_code.
 std::error_code openFileForRead(const Twine &Name, int &ResultFD,
                                 SmallVectorImpl<char> *RealPath = nullptr);
 
+/// @brief Opens the file with the given name in a read-only mode, returning
+/// its open file descriptor.
+///
+/// The caller is responsible for closing the freeing the file once they are
+/// finished with it.
+///
+/// @param Name The path of the file to open, relative or absolute.
+/// @param RealPath If nonnull, extra work is done to determine the real path
+///                 of the opened file, and that path is stored in this
+///                 location.
+/// @returns a platform-specific file descriptor if \a Name has been opened,
+///          otherwise an error object.
+Expected<file_t>
+openNativeFileForRead(const Twine &Name,
+                      SmallVectorImpl<char> *RealPath = nullptr);
+
+/// @brief Close the file object.  This should be used instead of ::close for
+/// portability.
+///
+/// @param F On input, this is the file to close.  On output, the file is
+/// set to kInvalidFile.
+void closeFile(file_t &F);
+
 std::error_code getUniqueID(const Twine Path, UniqueID &Result);
 
 /// Get disk space usage information.
 ///
 /// Note: Users must be careful about "Time Of Check, Time Of Use" kind of bug.
 /// Note: Windows reports results according to the quota allocated to the user.
 ///
 /// @param Path Input path.

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

 #define STATVFS_F_FLAG(vfs) (vfs).f_flags
 #endif
 
 using namespace llvm;
 
 namespace llvm {
 namespace sys  {
 namespace fs {
+
+const file_t kInvalidFile = -1;
+
 #if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) ||     \
     defined(__minix) || defined(__FreeBSD_kernel__) || defined(__linux__) ||   \
     defined(__CYGWIN__) || defined(__DragonFly__) || defined(_AIX)
 static int
 test_dir(char ret[PATH_MAX], const char *dir, const char *bin)
 {
   struct stat sb;
   char fullpath[PATH_MAX];
if (hasProcSelfFD()) {
     // Use ::realpath to get the real path name
     if (::realpath(P.begin(), Buffer) != nullptr)
       RealPath->append(Buffer, Buffer + strlen(Buffer));
   }
 #endif
   return std::error_code();
 }
 
+Expected<file_t> openNativeFileForRead(const Twine &Name,
+                                       SmallVectorImpl<char> *RealPath) {
+  file_t ResultFD;
+  std::error_code EC = openFileForRead(Name, ResultFD, RealPath);
+  if (EC)
+    return errorCodeToError(EC);
+  return ResultFD;
+}
+
 std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
                             sys::fs::OpenFlags Flags, unsigned Mode) {
   // Verify that we don't have both "append" and "excl".
   assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) &&
          "Cannot specify both 'excl' and 'append' file creation flags!");
 
   int OpenFlags = O_CREAT;
 

 #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();
 }
 
+Expected<file_t> openNativeFileForWrite(const Twine &Name, OpenFlags Flags,
+                                        unsigned Mode) {
+  file_t ResultFD;
+  std::error_code EC = openFileForWrite(Name, ResultFD, Flags, Mode);
+  if (EC)
+    return errorCodeToError(EC);
+  return ResultFD;
+}
+
+void closeFile(file_t &F) {
+  ::close(F);
+  F = kInvalidFile;
+}
+
 template <typename T>
 static std::error_code remove_directories_impl(const T &Entry,
                                                bool IgnoreErrors) {
   std::error_code EC;
   directory_iterator Begin(Entry, EC, false);
   directory_iterator End;
   while (Begin != End) {
     auto &Item = *Begin;

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

std::error_code widenPath(const Twine &Path8,
 
   // Just use the caller's original path.
   return UTF8ToUTF16(Path8Str, Path16);
 }
 } // end namespace path
 
 namespace fs {
 
+const file_t kInvalidFile = INVALID_HANDLE_VALUE;
+
 std::string getMainExecutable(const char *argv0, void *MainExecAddr) {
   SmallVector<wchar_t, MAX_PATH> PathName;
   DWORD Size = ::GetModuleFileNameW(NULL, PathName.data(), PathName.capacity());
 
   // A zero return value indicates a failure other than insufficient space.
   if (Size == 0)
     return "";
 
HANDLE H =
                     NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL);
   if (H == INVALID_HANDLE_VALUE)
     return mapWindowsError(GetLastError());
   std::error_code EC = realPathFromHandle(H, RealPath);
   ::CloseHandle(H);
   return EC;
 }
 
+static std::error_code nativeFileToFd(Expected<HANDLE> H, int &ResultFD,
+                                      int OpenFlags) {
+  ResultFD = -1;
+  if (!H)
+    return errorToErrorCode(H.takeError());
+
+  ResultFD = ::_open_osfhandle(intptr_t(*H), OpenFlags);
+  if (ResultFD == -1) {
+    ::CloseHandle(*H);
+    return mapWindowsError(ERROR_INVALID_HANDLE);
+  }
+  return std::error_code();
+}
+
 std::error_code openFileForRead(const Twine &Name, int &ResultFD,
                                 SmallVectorImpl<char> *RealPath) {
-  ResultFD = -1;
+  Expected<HANDLE> NativeFile = openNativeFileForRead(Name, RealPath);
+  return nativeFileToFd(std::move(NativeFile), ResultFD, 0);
+}
+
+Expected<file_t> openNativeFileForRead(const Twine &Name,
+                                       SmallVectorImpl<char> *RealPath) {
   SmallVector<wchar_t, 128> PathUTF16;
 
   if (std::error_code EC = widenPath(Name, PathUTF16))
-    return EC;
+    return errorCodeToError(EC);
 
   HANDLE H =
       ::CreateFileW(PathUTF16.begin(), GENERIC_READ,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
                     NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
   if (H == INVALID_HANDLE_VALUE) {
     DWORD LastError = ::GetLastError();
     std::error_code EC = mapWindowsError(LastError);
     // Provide a better error message when trying to open directories.
     // This only runs if we failed to open the file, so there is probably
     // no performances issues.
     if (LastError != ERROR_ACCESS_DENIED)
-      return EC;
+      return errorCodeToError(EC);
     if (is_directory(Name))
-      return make_error_code(errc::is_a_directory);
-    return EC;
-  }
-
-  ResultFD = ::_open_osfhandle(intptr_t(H), 0);
-  if (ResultFD == -1) {
-    ::CloseHandle(H);
-    return mapWindowsError(ERROR_INVALID_HANDLE);
+      return errorCodeToError(make_error_code(errc::is_a_directory));
+    return errorCodeToError(EC);
   }
 
   // Fetch the real name of the file, if the user asked
   if (RealPath)
     realPathFromHandle(H, *RealPath);
 
-  return std::error_code();
+  return H;
 }
 
 std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
                             sys::fs::OpenFlags Flags, unsigned Mode) {
+  int OpenFlags = 0;
+  if (Flags & F_Append)
+    OpenFlags |= _O_APPEND;
+
+  if (Flags & F_Text)
+    OpenFlags |= _O_TEXT;
+
+  Expected<HANDLE> NativeFile = openNativeFileForWrite(Name, Flags, Mode);
+  return nativeFileToFd(std::move(NativeFile), ResultFD, OpenFlags);
+}
+
+Expected<file_t> openNativeFileForWrite(const Twine &Name, OpenFlags Flags,
+                                        unsigned Mode) {
   // Verify that we don't have both "append" and "excl".
   assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) &&
          "Cannot specify both 'excl' and 'append' file creation flags!");
 
-  ResultFD = -1;
   SmallVector<wchar_t, 128> PathUTF16;
 
   if (std::error_code EC = widenPath(Name, PathUTF16))
-    return EC;
+    return errorCodeToError(EC);
 
   DWORD CreationDisposition;
   if (Flags & F_Excl)
     CreationDisposition = CREATE_NEW;
   else if ((Flags & F_Append) || (Flags & F_NoTrunc))
     CreationDisposition = OPEN_ALWAYS;
   else
     CreationDisposition = CREATE_ALWAYS;
Expected<file_t> openNativeFileForWrite(const Twine &Name, OpenFlags Flags,
 
   if (H == INVALID_HANDLE_VALUE) {
     DWORD LastError = ::GetLastError();
     std::error_code EC = mapWindowsError(LastError);
     // Provide a better error message when trying to open directories.
     // This only runs if we failed to open the file, so there is probably
     // no performances issues.
     if (LastError != ERROR_ACCESS_DENIED)
-      return EC;
+      return errorCodeToError(EC);
     if (is_directory(Name))
-      return make_error_code(errc::is_a_directory);
-    return EC;
+      return errorCodeToError(make_error_code(errc::is_a_directory));
+    return errorCodeToError(EC);
   }
 
-  int OpenFlags = 0;
-  if (Flags & F_Append)
-    OpenFlags |= _O_APPEND;
-
-  if (Flags & F_Text)
-    OpenFlags |= _O_TEXT;
-
-  ResultFD = ::_open_osfhandle(intptr_t(H), OpenFlags);
-  if (ResultFD == -1) {
-    ::CloseHandle(H);
-    return mapWindowsError(ERROR_INVALID_HANDLE);
-  }
+  return H;
 
-  return std::error_code();
+void closeFile(file_t &F) {
+  ::CloseHandle(F);
+  F = kInvalidFile;
 }
 
 std::error_code remove_directories(const Twine &path, bool IgnoreErrors) {
   // Convert to utf-16.
   SmallVector<wchar_t, 128> Path16;
   std::error_code EC = widenPath(path, Path16);
   if (EC && !IgnoreErrors)
     return EC;