[libc][NFC] clean internal fd handling (#143991)

The previous internal fcntl implementation modified errno directly, this
patch fixes that. This patch also moves open and close into OSUtil since
they are used in multiple places. There are more places that need
similar cleanup but only got comments in this patch to keep it
relatively reviewable.

Related to: https://github.com/llvm/llvm-project/issues/143937

libc/src/__support/File/linux/file.cpp

@@ -19,8 +19,8 @@
 #include "src/__support/macros/config.h"
 
 #include "hdr/fcntl_macros.h" // For mode_t and other flags to the open syscall
-#include <sys/stat.h>    // For S_IS*, S_IF*, and S_IR* flags.
-#include <sys/syscall.h> // For syscall numbers
+#include <sys/stat.h>         // For S_IS*, S_IF*, and S_IR* flags.
+#include <sys/syscall.h>      // For syscall numbers
 
 namespace LIBC_NAMESPACE_DECL {
 
@@ -128,10 +128,11 @@ ErrorOr<LinuxFile *> create_file_from_fd(int fd, const char *mode) {
     return Error(EINVAL);
   }
 
-  int fd_flags = internal::fcntl(fd, F_GETFL);
-  if (fd_flags == -1) {
+  auto result = internal::fcntl(fd, F_GETFL);
+  if (!result.has_value()) {
     return Error(EBADF);
   }
+  int fd_flags = result.value();
 
   using OpenMode = File::OpenMode;
   if (((fd_flags & O_ACCMODE) == O_RDONLY &&
@@ -145,8 +146,9 @@ ErrorOr<LinuxFile *> create_file_from_fd(int fd, const char *mode) {
   if ((modeflags & static_cast<ModeFlags>(OpenMode::APPEND)) &&
       !(fd_flags & O_APPEND)) {
     do_seek = true;
-    if (internal::fcntl(fd, F_SETFL,
-                        reinterpret_cast<void *>(fd_flags | O_APPEND)) == -1) {
+    if (!internal::fcntl(fd, F_SETFL,
+                         reinterpret_cast<void *>(fd_flags | O_APPEND))
+             .has_value()) {
       return Error(EBADF);
     }
   }

libc/src/__support/OSUtil/fcntl.h

@@ -8,12 +8,18 @@
 #ifndef LLVM_LIBC_SRC___SUPPORT_OSUTIL_FCNTL_H
 #define LLVM_LIBC_SRC___SUPPORT_OSUTIL_FCNTL_H
 
+#include "hdr/types/mode_t.h"
+#include "src/__support/error_or.h"
 #include "src/__support/macros/config.h"
 
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-int fcntl(int fd, int cmd, void *arg = nullptr);
+ErrorOr<int> fcntl(int fd, int cmd, void *arg = nullptr);
+
+ErrorOr<int> open(const char *path, int flags, mode_t mode_flags = 0);
+
+ErrorOr<int> close(int fd);
 
 } // namespace internal
 } // namespace LIBC_NAMESPACE_DECL

libc/src/__support/OSUtil/linux/CMakeLists.txt

@@ -16,7 +16,6 @@ add_object_library(
     .${LIBC_TARGET_ARCHITECTURE}.linux_${LIBC_TARGET_ARCHITECTURE}_util
     libc.src.__support.common
     libc.src.__support.CPP.string_view
-    libc.src.errno.errno
     libc.hdr.fcntl_macros
     libc.hdr.types.struct_flock
     libc.hdr.types.struct_flock64

libc/src/__support/OSUtil/linux/fcntl.cpp

@@ -8,23 +8,24 @@
 
 #include "src/__support/OSUtil/fcntl.h"
 
+#include "hdr/errno_macros.h"
 #include "hdr/fcntl_macros.h"
+#include "hdr/types/mode_t.h"
 #include "hdr/types/off_t.h"
 #include "hdr/types/struct_f_owner_ex.h"
 #include "hdr/types/struct_flock.h"
 #include "hdr/types/struct_flock64.h"
 #include "src/__support/OSUtil/syscall.h" // For internal syscall function.
 #include "src/__support/common.h"
-#include "src/__support/libc_errno.h"
+#include "src/__support/error_or.h"
 #include "src/__support/macros/config.h"
 
-#include <stdarg.h>
 #include <sys/syscall.h> // For syscall numbers.
 
 namespace LIBC_NAMESPACE_DECL {
 namespace internal {
 
-int fcntl(int fd, int cmd, void *arg) {
+ErrorOr<int> fcntl(int fd, int cmd, void *arg) {
 #if SYS_fcntl
   constexpr auto FCNTL_SYSCALL_ID = SYS_fcntl;
 #elif defined(SYS_fcntl64)
@@ -33,8 +34,7 @@ int fcntl(int fd, int cmd, void *arg) {
 #error "fcntl and fcntl64 syscalls not available."
 #endif
 
-  int new_cmd = cmd;
-  switch (new_cmd) {
+  switch (cmd) {
   case F_OFD_SETLKW: {
     struct flock *flk = reinterpret_cast<struct flock *>(arg);
     // convert the struct to a flock64
@@ -45,8 +45,11 @@ int fcntl(int fd, int cmd, void *arg) {
     flk64.l_len = flk->l_len;
     flk64.l_pid = flk->l_pid;
     // create a syscall
-    return LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, new_cmd,
-                                             &flk64);
+    int ret =
+        LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd, &flk64);
+    if (ret < 0)
+      return Error(-ret);
+    return ret;
   }
   case F_OFD_GETLK:
   case F_OFD_SETLK: {
@@ -59,60 +62,80 @@ int fcntl(int fd, int cmd, void *arg) {
     flk64.l_len = flk->l_len;
     flk64.l_pid = flk->l_pid;
     // create a syscall
-    int retVal = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd,
-                                                   new_cmd, &flk64);
+    int ret =
+        LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd, &flk64);
     // On failure, return
-    if (retVal == -1)
-      return -1;
+    if (ret < 0)
+      return Error(-1);
     // Check for overflow, i.e. the offsets are not the same when cast
     // to off_t from off64_t.
     if (static_cast<off_t>(flk64.l_len) != flk64.l_len ||
-        static_cast<off_t>(flk64.l_start) != flk64.l_start) {
-      libc_errno = EOVERFLOW;
-      return -1;
-    }
+        static_cast<off_t>(flk64.l_start) != flk64.l_start)
+      return Error(EOVERFLOW);
+
     // Now copy back into flk, in case flk64 got modified
     flk->l_type = flk64.l_type;
     flk->l_whence = flk64.l_whence;
     flk->l_start = static_cast<decltype(flk->l_start)>(flk64.l_start);
     flk->l_len = static_cast<decltype(flk->l_len)>(flk64.l_len);
     flk->l_pid = flk64.l_pid;
-    return retVal;
+    return ret;
   }
   case F_GETOWN: {
     struct f_owner_ex fex;
     int ret = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd,
                                                 F_GETOWN_EX, &fex);
-    if (ret >= 0)
-      return fex.type == F_OWNER_PGRP ? -fex.pid : fex.pid;
-    libc_errno = -ret;
-    return -1;
+    if (ret < 0)
+      return Error(-ret);
+    return fex.type == F_OWNER_PGRP ? -fex.pid : fex.pid;
   }
 #ifdef SYS_fcntl64
   case F_GETLK: {
     if constexpr (FCNTL_SYSCALL_ID == SYS_fcntl64)
-      new_cmd = F_GETLK64;
+      cmd = F_GETLK64;
     break;
   }
   case F_SETLK: {
     if constexpr (FCNTL_SYSCALL_ID == SYS_fcntl64)
-      new_cmd = F_SETLK64;
+      cmd = F_SETLK64;
     break;
   }
   case F_SETLKW: {
     if constexpr (FCNTL_SYSCALL_ID == SYS_fcntl64)
-      new_cmd = F_SETLKW64;
+      cmd = F_SETLKW64;
     break;
   }
 #endif
   }
-  int retVal = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, new_cmd,
-                                                 reinterpret_cast<void *>(arg));
-  if (retVal >= 0) {
-    return retVal;
-  }
-  libc_errno = -retVal;
-  return -1;
+
+  // default, but may use rewritten cmd from above.
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(FCNTL_SYSCALL_ID, fd, cmd,
+                                              reinterpret_cast<void *>(arg));
+  if (ret < 0)
+    return Error(-ret);
+  return ret;
+}
+
+ErrorOr<int> open(const char *path, int flags, mode_t mode_flags) {
+#ifdef SYS_open
+  int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_open, path, flags, mode_flags);
+#else
+  int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_openat, AT_FDCWD, path, flags,
+                                             mode_flags);
+#endif
+  if (fd < 0)
+    return Error(-fd);
+
+  return fd;
+}
+
+ErrorOr<int> close(int fd) {
+  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_close, fd);
+
+  if (ret < 0)
+    return Error(-ret);
+
+  return ret;
 }
 
 } // namespace internal

libc/src/fcntl/linux/CMakeLists.txt

@@ -19,6 +19,7 @@ add_entrypoint_object(
   DEPENDS
     libc.hdr.fcntl_macros
     libc.src.__support.OSUtil.osutil
+    libc.src.errno.errno
 )
 
 add_entrypoint_object(

libc/src/fcntl/linux/fcntl.cpp

@@ -10,6 +10,7 @@
 
 #include "src/__support/OSUtil/fcntl.h"
 #include "src/__support/common.h"
+#include "src/__support/libc_errno.h"
 #include "src/__support/macros/config.h"
 
 #include <stdarg.h>
@@ -22,7 +23,14 @@ LLVM_LIBC_FUNCTION(int, fcntl, (int fd, int cmd, ...)) {
   va_start(varargs, cmd);
   arg = va_arg(varargs, void *);
   va_end(varargs);
-  return LIBC_NAMESPACE::internal::fcntl(fd, cmd, arg);
+
+  auto result = LIBC_NAMESPACE::internal::fcntl(fd, cmd, arg);
+
+  if (!result.has_value()) {
+    libc_errno = result.error();
+    return -1;
+  }
+  return result.value();
 }
 
 } // namespace LIBC_NAMESPACE_DECL

libc/src/fcntl/linux/open.cpp

@@ -8,15 +8,13 @@
 
 #include "src/fcntl/open.h"
 
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "hdr/fcntl_macros.h"
+#include "hdr/types/mode_t.h"
+#include "src/__support/OSUtil/fcntl.h"
 #include "src/__support/common.h"
 #include "src/__support/libc_errno.h"
 #include "src/__support/macros/config.h"
-
-#include "hdr/fcntl_macros.h"
-#include "hdr/types/mode_t.h"
 #include <stdarg.h>
-#include <sys/syscall.h> // For syscall numbers.
 
 namespace LIBC_NAMESPACE_DECL {
 
@@ -31,17 +29,13 @@ LLVM_LIBC_FUNCTION(int, open, (const char *path, int flags, ...)) {
     va_end(varargs);
   }
 
-#ifdef SYS_open
-  int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_open, path, flags, mode_flags);
-#else
-  int fd = LIBC_NAMESPACE::syscall_impl<int>(SYS_openat, AT_FDCWD, path, flags,
-                                             mode_flags);
-#endif
-  if (fd > 0)
-    return fd;
+  auto result = internal::open(path, flags, mode_flags);
 
-  libc_errno = -fd;
-  return -1;
+  if (!result.has_value()) {
+    libc_errno = result.error();
+    return -1;
+  }
+  return result.value();
 }
 
 } // namespace LIBC_NAMESPACE_DECL

libc/src/sys/auxv/linux/getauxval.cpp

@@ -8,6 +8,8 @@
 
 #include "src/sys/auxv/getauxval.h"
 #include "config/app.h"
+#include "hdr/fcntl_macros.h"
+#include "src/__support/OSUtil/fcntl.h"
 #include "src/__support/common.h"
 #include "src/__support/libc_errno.h"
 #include "src/__support/macros/config.h"
@@ -17,14 +19,18 @@
 #include "src/__support/threads/callonce.h"
 #include "src/__support/threads/linux/futex_word.h"
 
+// -----------------------------------------------------------------------------
+// TODO: This file should not include other public libc functions. Calling other
+// public libc functions is an antipattern within LLVM-libc. This needs to be
+// cleaned up. DO NOT COPY THIS.
+// -----------------------------------------------------------------------------
+
 // for mallocing the global auxv
 #include "src/sys/mman/mmap.h"
 #include "src/sys/mman/munmap.h"
 
 // for reading /proc/self/auxv
-#include "src/fcntl/open.h"
 #include "src/sys/prctl/prctl.h"
-#include "src/unistd/close.h"
 #include "src/unistd/read.h"
 
 // getauxval will work either with or without __cxa_atexit support.
@@ -60,17 +66,18 @@ public:
   constexpr static size_t AUXV_MMAP_SIZE = sizeof(AuxEntry) * MAX_AUXV_ENTRIES;
 
   AuxvMMapGuard()
-      : ptr(mmap(nullptr, AUXV_MMAP_SIZE, PROT_READ | PROT_WRITE,
-                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) {}
+      : ptr(LIBC_NAMESPACE::mmap(nullptr, AUXV_MMAP_SIZE,
+                                 PROT_READ | PROT_WRITE,
+                                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) {}
   ~AuxvMMapGuard() {
     if (ptr != MAP_FAILED)
-      munmap(ptr, AUXV_MMAP_SIZE);
+      LIBC_NAMESPACE::munmap(ptr, AUXV_MMAP_SIZE);
   }
   void submit_to_global() {
     // atexit may fail, we do not set it to global in that case.
     int ret = __cxa_atexit(
         [](void *) {
-          munmap(auxv, AUXV_MMAP_SIZE);
+          LIBC_NAMESPACE::munmap(auxv, AUXV_MMAP_SIZE);
           auxv = nullptr;
         },
         nullptr, nullptr);
@@ -90,10 +97,16 @@ private:
 
 class AuxvFdGuard {
 public:
-  AuxvFdGuard() : fd(open("/proc/self/auxv", O_RDONLY | O_CLOEXEC)) {}
+  AuxvFdGuard() {
+    auto result = internal::open("/proc/self/auxv", O_RDONLY | O_CLOEXEC);
+    if (!result.has_value())
+      fd = -1;
+
+    fd = result.value();
+  }
   ~AuxvFdGuard() {
     if (fd != -1)
-      close(fd);
+      internal::close(fd);
   }
   bool valid() const { return fd != -1; }
   int get() const { return fd; }
@@ -135,7 +148,8 @@ static void initialize_auxv_once(void) {
   bool error_detected = false;
   // Read until we use up all the available space or we finish reading the file.
   while (available_size != 0) {
-    ssize_t bytes_read = read(fd_guard.get(), buf, available_size);
+    ssize_t bytes_read =
+        LIBC_NAMESPACE::read(fd_guard.get(), buf, available_size);
     if (bytes_read <= 0) {
       if (libc_errno == EINTR)
         continue;
@@ -158,7 +172,7 @@ static AuxEntry read_entry(int fd) {
   size_t size = sizeof(AuxEntry);
   char *ptr = reinterpret_cast<char *>(&buf);
   while (size > 0) {
-    ssize_t ret = read(fd, ptr, size);
+    ssize_t ret = LIBC_NAMESPACE::read(fd, ptr, size);
     if (ret < 0) {
       if (libc_errno == EINTR)
         continue;
@@ -195,7 +209,8 @@ LLVM_LIBC_FUNCTION(unsigned long, getauxval, (unsigned long id)) {
     return search_auxv(app.auxv_ptr, id);
 
   static FutexWordType once_flag;
-  callonce(reinterpret_cast<CallOnceFlag *>(&once_flag), initialize_auxv_once);
+  LIBC_NAMESPACE::callonce(reinterpret_cast<CallOnceFlag *>(&once_flag),
+                           initialize_auxv_once);
   if (auxv != nullptr)
     return search_auxv(auxv, id);
 

libc/src/sys/mman/linux/shm_common.h

@@ -13,6 +13,11 @@
 #include "src/__support/macros/config.h"
 #include "src/string/memory_utils/inline_memcpy.h"
 
+// TODO: clean this up.
+//  1. Change from optional to ErrorOr, and return the errno instead of setting
+//    it here.
+//  2. Replace inline memcpy with __builtin_memcpy
+
 // TODO: Get PATH_MAX via https://github.com/llvm/llvm-project/issues/85121
 #include <linux/limits.h>
 

libc/src/sys/mman/linux/shm_open.cpp

@@ -7,9 +7,10 @@
 //===----------------------------------------------------------------------===//
 
 #include "src/sys/mman/shm_open.h"
+#include "hdr/fcntl_macros.h"
 #include "hdr/types/mode_t.h"
+#include "src/__support/OSUtil/fcntl.h"
 #include "src/__support/macros/config.h"
-#include "src/fcntl/open.h"
 #include "src/sys/mman/linux/shm_common.h"
 
 namespace LIBC_NAMESPACE_DECL {
@@ -17,9 +18,16 @@ namespace LIBC_NAMESPACE_DECL {
 static constexpr int DEFAULT_OFLAGS = O_NOFOLLOW | O_CLOEXEC | O_NONBLOCK;
 
 LLVM_LIBC_FUNCTION(int, shm_open, (const char *name, int oflags, mode_t mode)) {
-  using namespace shm_common;
-  if (cpp::optional<SHMPath> buffer = translate_name(name))
-    return open(buffer->data(), oflags | DEFAULT_OFLAGS, mode);
+  if (cpp::optional<shm_common::SHMPath> buffer =
+          shm_common::translate_name(name)) {
+    auto result = internal::open(buffer->data(), oflags | DEFAULT_OFLAGS, mode);
+
+    if (!result.has_value()) {
+      libc_errno = result.error();
+      return -1;
+    }
+    return result.value();
+  }
   return -1;
 }
 

libc/src/sys/mman/linux/shm_unlink.cpp

@@ -13,10 +13,13 @@
 
 namespace LIBC_NAMESPACE_DECL {
 
+// TODO: stop calling the public unlink function. It should be calling an
+// internal shared utility.
+
 LLVM_LIBC_FUNCTION(int, shm_unlink, (const char *name)) {
-  using namespace shm_common;
-  if (cpp::optional<SHMPath> buffer = translate_name(name))
-    return unlink(buffer->data());
+  if (cpp::optional<shm_common::SHMPath> buffer =
+          shm_common::translate_name(name))
+    return LIBC_NAMESPACE::unlink(buffer->data());
   return -1;
 }
 

libc/src/unistd/linux/close.cpp

@@ -8,9 +8,8 @@
 
 #include "src/unistd/close.h"
 
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/OSUtil/fcntl.h"
 #include "src/__support/common.h"
-
 #include "src/__support/libc_errno.h"
 #include "src/__support/macros/config.h"
 #include <sys/syscall.h> // For syscall numbers.
@@ -18,12 +17,13 @@
 namespace LIBC_NAMESPACE_DECL {
 
 LLVM_LIBC_FUNCTION(int, close, (int fd)) {
-  int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_close, fd);
-  if (ret < 0) {
-    libc_errno = -ret;
+  auto result = internal::close(fd);
+
+  if (!result.has_value()) {
+    libc_errno = result.error();
     return -1;
   }
-  return ret;
+  return result.value();
 }
 
 } // namespace LIBC_NAMESPACE_DECL

utils/bazel/llvm-project-overlay/libc/BUILD.bazel

@@ -344,6 +344,21 @@ libc_support_library(
     hdrs = ["hdr/types/struct_epoll_event.h"],
 )
 
+libc_support_library(
+    name = "types_struct_f_owner_ex",
+    hdrs = ["hdr/types/struct_f_owner_ex.h"],
+)
+
+libc_support_library(
+    name = "types_struct_flock",
+    hdrs = ["hdr/types/struct_flock.h"],
+)
+
+libc_support_library(
+    name = "types_struct_flock64",
+    hdrs = ["hdr/types/struct_flock64.h"],
+)
+
 libc_support_library(
     name = "types_struct_timespec",
     hdrs = ["hdr/types/struct_timespec.h"],
@@ -1380,6 +1395,28 @@ libc_support_library(
     ],
 )
 
+libc_support_library(
+    name = "__support_osutil_fcntl",
+    srcs = ["src/__support/OSUtil/linux/fcntl.cpp"],
+    hdrs = ["src/__support/OSUtil/fcntl.h"],
+    target_compatible_with = select({
+        "@platforms//os:linux": [],
+        "//conditions:default": ["@platforms//:incompatible"],
+    }),
+    deps = [
+        ":__support_common",
+        ":__support_error_or",
+        ":__support_osutil_syscall",
+        ":hdr_errno_macros",
+        ":hdr_fcntl_macros",
+        ":types_mode_t",
+        ":types_off_t",
+        ":types_struct_f_owner_ex",
+        ":types_struct_flock",
+        ":types_struct_flock64",
+    ],
+)
+
 libc_support_library(
     name = "__support_osutil_exit",
     srcs = ["src/__support/OSUtil/linux/exit.cpp"],
@@ -1601,8 +1638,8 @@ libc_support_library(
 libc_header_library(
     name = "libcxx_shared_headers",
     hdrs = [
-        "shared/libc_common.h",
         "shared/fp_bits.h",
+        "shared/libc_common.h",
         "shared/str_to_float.h",
         "shared/str_to_integer.h",
     ],
@@ -4475,13 +4512,28 @@ libc_function(
     }),
     deps = [
         ":__support_common",
-        ":__support_osutil_syscall",
+        ":__support_osutil_fcntl",
         ":errno",
         ":hdr_fcntl_macros",
         ":types_mode_t",
     ],
 )
 
+libc_function(
+    name = "fcntl",
+    srcs = ["src/fcntl/linux/fcntl.cpp"],
+    hdrs = ["src/fcntl/fcntl.h"],
+    target_compatible_with = select({
+        "@platforms//os:linux": [],
+        "//conditions:default": ["@platforms//:incompatible"],
+    }),
+    deps = [
+        ":__support_common",
+        ":__support_osutil_fcntl",
+        ":errno",
+    ],
+)
+
 libc_function(
     name = "openat",
     srcs = ["src/fcntl/linux/openat.cpp"],
@@ -4542,7 +4594,7 @@ libc_function(
     hdrs = ["src/unistd/close.h"],
     deps = [
         ":__support_common",
-        ":__support_osutil_syscall",
+        ":__support_osutil_fcntl",
         ":errno",
     ],
 )