Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2025

Conversation

michaelrj-google
Copy link
Contributor

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: #143937

@llvmbot llvmbot added libc bazel "Peripheral" support tier build system: utils/bazel labels Jun 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 12, 2025

@llvm/pr-subscribers-libc

Author: Michael Jones (michaelrj-google)

Changes

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: #143937


Patch is 21.58 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143991.diff

14 Files Affected:

  • (modified) libc/docs/configure.rst (+1-1)
  • (modified) libc/src/__support/File/linux/file.cpp (+8-6)
  • (modified) libc/src/__support/OSUtil/fcntl.h (+7-1)
  • (modified) libc/src/__support/OSUtil/linux/CMakeLists.txt (-1)
  • (modified) libc/src/__support/OSUtil/linux/fcntl.cpp (+53-30)
  • (modified) libc/src/fcntl/linux/CMakeLists.txt (+1)
  • (modified) libc/src/fcntl/linux/fcntl.cpp (+9-1)
  • (modified) libc/src/fcntl/linux/open.cpp (+9-15)
  • (modified) libc/src/sys/auxv/linux/getauxval.cpp (+26-11)
  • (modified) libc/src/sys/mman/linux/shm_common.h (+5)
  • (modified) libc/src/sys/mman/linux/shm_open.cpp (+12-4)
  • (modified) libc/src/sys/mman/linux/shm_unlink.cpp (+6-3)
  • (modified) libc/src/unistd/linux/close.cpp (+6-6)
  • (modified) utils/bazel/llvm-project-overlay/libc/BUILD.bazel (+55-3)
diff --git a/libc/docs/configure.rst b/libc/docs/configure.rst
index 8d53390ae19bf..109412225634f 100644
--- a/libc/docs/configure.rst
+++ b/libc/docs/configure.rst
@@ -29,7 +29,7 @@ to learn about the defaults for your platform and target.
     - ``LIBC_CONF_ENABLE_STRONG_STACK_PROTECTOR``: Enable -fstack-protector-strong to defend against stack smashing attack.
     - ``LIBC_CONF_KEEP_FRAME_POINTER``: Keep frame pointer in functions for better debugging experience.
 * **"errno" options**
-    - ``LIBC_CONF_ERRNO_MODE``: The implementation used for errno, acceptable values are LIBC_ERRNO_MODE_DEFAULT, LIBC_ERRNO_MODE_UNDEFINED, LIBC_ERRNO_MODE_THREAD_LOCAL, LIBC_ERRNO_MODE_SHARED, LIBC_ERRNO_MODE_EXTERNAL, and LIBC_ERRNO_MODE_SYSTEM.
+    - ``LIBC_CONF_ERRNO_MODE``: The implementation used for errno, acceptable values are LIBC_ERRNO_MODE_DEFAULT, LIBC_ERRNO_MODE_UNDEFINED, LIBC_ERRNO_MODE_THREAD_LOCAL, LIBC_ERRNO_MODE_SHARED, LIBC_ERRNO_MODE_EXTERNAL, LIBC_ERRNO_MODE_SYSTEM, and LIBC_ERRNO_MODE_SYSTEM_INLINE.
 * **"general" options**
     - ``LIBC_ADD_NULL_CHECKS``: Add nullptr checks in the library's implementations to some functions for which passing nullptr is undefined behavior.
 * **"math" options**
diff --git a/libc/src/__support/File/linux/file.cpp b/libc/src/__support/File/linux/file.cpp
index 761e352f74ead..4594dadf1ccdf 100644
--- a/libc/src/__support/File/linux/file.cpp
+++ b/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);
     }
   }
diff --git a/libc/src/__support/OSUtil/fcntl.h b/libc/src/__support/OSUtil/fcntl.h
index 46f7d28132396..3983d78f7f89c 100644
--- a/libc/src/__support/OSUtil/fcntl.h
+++ b/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
diff --git a/libc/src/__support/OSUtil/linux/CMakeLists.txt b/libc/src/__support/OSUtil/linux/CMakeLists.txt
index b9704d42cd33b..4681d8c2bb73c 100644
--- a/libc/src/__support/OSUtil/linux/CMakeLists.txt
+++ b/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
diff --git a/libc/src/__support/OSUtil/linux/fcntl.cpp b/libc/src/__support/OSUtil/linux/fcntl.cpp
index 99e16ad58c918..bb76eee90efd2 100644
--- a/libc/src/__support/OSUtil/linux/fcntl.cpp
+++ b/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
diff --git a/libc/src/fcntl/linux/CMakeLists.txt b/libc/src/fcntl/linux/CMakeLists.txt
index 580db16cd4132..c31eb3f438c10 100644
--- a/libc/src/fcntl/linux/CMakeLists.txt
+++ b/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(
diff --git a/libc/src/fcntl/linux/fcntl.cpp b/libc/src/fcntl/linux/fcntl.cpp
index a0c8459ced342..fd9c48eb562f7 100644
--- a/libc/src/fcntl/linux/fcntl.cpp
+++ b/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
diff --git a/libc/src/fcntl/linux/open.cpp b/libc/src/fcntl/linux/open.cpp
index a21a03788deaa..3a56d10554198 100644
--- a/libc/src/fcntl/linux/open.cpp
+++ b/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
diff --git a/libc/src/sys/auxv/linux/getauxval.cpp b/libc/src/sys/auxv/linux/getauxval.cpp
index f3ae7c5c4e07a..b50c5845bcc2b 100644
--- a/libc/src/sys/auxv/linux/getauxval.cpp
+++ b/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 @@ class AuxvMMapGuard {
   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 @@ class AuxvMMapGuard {
 
 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);
 
diff --git a/libc/src/sys/mman/linux/shm_common.h b/libc/src/sys/mman/linux/shm_common.h
index 69911012ff7e9..29d1401821e49 100644
--- a/libc/src/sys/mman/linux/shm_common.h
+++ b/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>
 
diff --git a/libc/src/sys/mman/linux/shm_open.cpp b/libc/src/sys/mman/linux/shm_open.cpp
index 11de482272d00..3099062eace98 100644
--- a/libc/src/sys/mman/linux/shm_open.cpp
+++ b/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;
 }
 
diff --git a/libc/src/sys/mman/linux/shm_unlink.cpp b/libc/src/sys/mman/linux/shm_unlink.cpp
index 6a76301512201..4c61c7cd16bad 100644
--- a/libc/src/sys/mman/linux/shm_unlink.cpp
+++ b/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;
 }
 
diff --git a/libc/src/unistd/linux/close.cpp b/libc/src/unistd/linux/close.cpp
index b5842f2b64d20..6ef3a3c6d63f0 100644
--- a/libc/src/unistd/linux/close.cpp
+++ b/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
diff --git a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
index 0cedad2859247..a69720fba2a0c 100644
--- a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
+++ b/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",
+        ":__su...
[truncated]

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: llvm#143937
@michaelrj-google
Copy link
Contributor Author

rebased to clear the doc change, planning to merge once presubmits finish.

@michaelrj-google michaelrj-google merged commit 51689c9 into llvm:main Jun 13, 2025
15 of 16 checks passed
@michaelrj-google michaelrj-google deleted the libcOSUtilFileCleanup branch June 13, 2025 17:31
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jun 13, 2025

LLVM Buildbot has detected a new failure on builder libc-x86_64-debian running on libc-x86_64-debian while building libc,utils at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/43/builds/23372

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
[       OK ] LlvmLibcSysIoctlTest.InvalidCommandAndFIONREAD (39 us)
Ran 1 tests.  PASS: 1  FAIL: 0
[2141/2141] Running unit test libc.test.utils.FPUtil.x86_long_double_test
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcX86LongDoubleTest.is_nan
[       OK ] LlvmLibcX86LongDoubleTest.is_nan (8 ms)
Ran 1 tests.  PASS: 1  FAIL: 0
@@@BUILD_STEP libc-fuzzer@@@
Running: ninja libc-fuzzer
[1/4] Linking CXX executable libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz
FAILED: libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz 
: && /usr/bin/clang++ -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fsanitize=fuzzer -O3 -DNDEBUG  libc/fuzzing/stdio/CMakeFiles/libc.fuzzing.stdio.printf_parser_fuzz.dir/printf_parser_fuzz.cpp.o -o libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz  libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./exit.cpp.o  libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./fcntl.cpp.o && :
/usr/bin/ld: libc/fuzzing/stdio/CMakeFiles/libc.fuzzing.stdio.printf_parser_fuzz.dir/printf_parser_fuzz.cpp.o: in function `__llvm_libc_20_0_0_git::printf_core::Parser<__llvm_libc_20_0_0_git::internal::MockArgList>::get_next_section()':
printf_parser_fuzz.cpp:(.text._ZN22__llvm_libc_20_0_0_git11printf_core6ParserINS_8internal11MockArgListEE16get_next_sectionEv[_ZN22__llvm_libc_20_0_0_git11printf_core6ParserINS_8internal11MockArgListEE16get_next_sectionEv]+0x10b8): undefined reference to `__llvm_libc_20_0_0_git::libc_errno'
/usr/bin/ld: printf_parser_fuzz.cpp:(.text._ZN22__llvm_libc_20_0_0_git11printf_core6ParserINS_8internal11MockArgListEE16get_next_sectionEv[_ZN22__llvm_libc_20_0_0_git11printf_core6ParserINS_8internal11MockArgListEE16get_next_sectionEv]+0x10bd): undefined reference to `__llvm_libc_20_0_0_git::Errno::operator int()'
/usr/bin/ld: libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz: hidden symbol `_ZN22__llvm_libc_20_0_0_git10libc_errnoE' isn't defined
/usr/bin/ld: final link failed: bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2/4] Linking CXX executable libc/fuzzing/stdlib/libc.fuzzing.stdlib.atof_differential_fuzz
[3/4] Linking CXX executable libc/fuzzing/math/libc.fuzzing.math.math_differential_fuzz
[4/4] Linking CXX executable libc/fuzzing/math/libc.fuzzing.math.nextafter_differential_fuzz
ninja: build stopped: subcommand failed.
['ninja', 'libc-fuzzer'] exited with return code 1.
The build step threw an exception...
Traceback (most recent call last):
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 176, in step
    yield
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 167, in main
    run_command(['ninja', 'libc-fuzzer'])
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 191, in run_command
    util.report_run_cmd(cmd, cwd=directory)
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian/llvm-zorg/zorg/buildbot/builders/annotated/util.py", line 49, in report_run_cmd
    subprocess.check_call(cmd, shell=shell, *args, **kwargs)
  File "/usr/lib/python3.11/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['ninja', 'libc-fuzzer']' returned non-zero exit status 1.
@@@STEP_FAILURE@@@

@llvm-ci
Copy link
Collaborator

llvm-ci commented Jun 13, 2025

LLVM Buildbot has detected a new failure on builder libc-x86_64-debian-dbg-asan running on libc-x86_64-debian while building libc,utils at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/147/builds/22918

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
[ RUN      ] LlvmLibcRoundToIntegerTest.InfinityAndNaN
[       OK ] LlvmLibcRoundToIntegerTest.InfinityAndNaN (32 us)
[ RUN      ] LlvmLibcRoundToIntegerTest.RoundNumbers
[       OK ] LlvmLibcRoundToIntegerTest.RoundNumbers (107 us)
[ RUN      ] LlvmLibcRoundToIntegerTest.SubnormalRange
[       OK ] LlvmLibcRoundToIntegerTest.SubnormalRange (11936 ms)
Ran 3 tests.  PASS: 3  FAIL: 0
@@@BUILD_STEP libc-fuzzer@@@
Running: ninja libc-fuzzer
[1/4] Linking CXX executable libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz
FAILED: libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz 
: && /usr/bin/clang++ -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fno-omit-frame-pointer -fsanitize=address -fdiagnostics-color -fsanitize=fuzzer -g  libc/fuzzing/stdio/CMakeFiles/libc.fuzzing.stdio.printf_parser_fuzz.dir/printf_parser_fuzz.cpp.o -o libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz  libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./exit.cpp.o  libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./fcntl.cpp.o && :
/usr/bin/ld: libc/fuzzing/stdio/CMakeFiles/libc.fuzzing.stdio.printf_parser_fuzz.dir/printf_parser_fuzz.cpp.o: in function `__llvm_libc_20_0_0_git::printf_core::Parser<__llvm_libc_20_0_0_git::internal::MockArgList>::get_next_section()':
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg-asan/llvm-project/libc/src/stdio/printf_core/parser.h:270: undefined reference to `__llvm_libc_20_0_0_git::libc_errno'
/usr/bin/ld: /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg-asan/llvm-project/libc/src/stdio/printf_core/parser.h:270: undefined reference to `__llvm_libc_20_0_0_git::Errno::operator int()'
/usr/bin/ld: libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz: hidden symbol `_ZN22__llvm_libc_20_0_0_git10libc_errnoE' isn't defined
/usr/bin/ld: final link failed: bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2/4] Linking CXX executable libc/fuzzing/stdlib/libc.fuzzing.stdlib.atof_differential_fuzz
[3/4] Linking CXX executable libc/fuzzing/math/libc.fuzzing.math.nextafter_differential_fuzz
[4/4] Linking CXX executable libc/fuzzing/math/libc.fuzzing.math.math_differential_fuzz
ninja: build stopped: subcommand failed.
['ninja', 'libc-fuzzer'] exited with return code 1.
The build step threw an exception...
Traceback (most recent call last):
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg-asan/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 176, in step
    yield
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg-asan/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 167, in main
    run_command(['ninja', 'libc-fuzzer'])
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg-asan/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 191, in run_command
    util.report_run_cmd(cmd, cwd=directory)
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg-asan/llvm-zorg/zorg/buildbot/builders/annotated/util.py", line 49, in report_run_cmd
    subprocess.check_call(cmd, shell=shell, *args, **kwargs)
  File "/usr/lib/python3.11/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['ninja', 'libc-fuzzer']' returned non-zero exit status 1.
@@@STEP_FAILURE@@@

@llvm-ci
Copy link
Collaborator

llvm-ci commented Jun 13, 2025

LLVM Buildbot has detected a new failure on builder libc-x86_64-debian-dbg running on libc-x86_64-debian while building libc,utils at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/93/builds/23443

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: 'python ../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py ...' (failure)
...
[       OK ] LlvmLibcStrtoimaxTest.AutomaticBaseSelection (4 us)
Ran 7 tests.  PASS: 7  FAIL: 0
[2141/2141] Running unit test libc.test.utils.FPUtil.x86_long_double_test
[==========] Running 1 test from 1 test suite.
[ RUN      ] LlvmLibcX86LongDoubleTest.is_nan
[       OK ] LlvmLibcX86LongDoubleTest.is_nan (79 ms)
Ran 1 tests.  PASS: 1  FAIL: 0
@@@BUILD_STEP libc-fuzzer@@@
Running: ninja libc-fuzzer
[1/4] Linking CXX executable libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz
FAILED: libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz 
: && /usr/bin/clang++ -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -fsanitize=fuzzer -g  libc/fuzzing/stdio/CMakeFiles/libc.fuzzing.stdio.printf_parser_fuzz.dir/printf_parser_fuzz.cpp.o -o libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz  libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./exit.cpp.o  libc/src/__support/OSUtil/linux/CMakeFiles/libc.src.__support.OSUtil.linux.linux_util.dir/./fcntl.cpp.o && :
/usr/bin/ld: libc/fuzzing/stdio/CMakeFiles/libc.fuzzing.stdio.printf_parser_fuzz.dir/printf_parser_fuzz.cpp.o: in function `__llvm_libc_20_0_0_git::printf_core::Parser<__llvm_libc_20_0_0_git::internal::MockArgList>::get_next_section()':
/home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg/llvm-project/libc/src/stdio/printf_core/parser.h:270: undefined reference to `__llvm_libc_20_0_0_git::libc_errno'
/usr/bin/ld: /home/llvm-libc-buildbot/buildbot-worker/libc-x86_64-debian/libc-x86_64-debian-dbg/llvm-project/libc/src/stdio/printf_core/parser.h:270: undefined reference to `__llvm_libc_20_0_0_git::Errno::operator int()'
/usr/bin/ld: libc/fuzzing/stdio/libc.fuzzing.stdio.printf_parser_fuzz: hidden symbol `_ZN22__llvm_libc_20_0_0_git10libc_errnoE' isn't defined
/usr/bin/ld: final link failed: bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)
[2/4] Linking CXX executable libc/fuzzing/stdlib/libc.fuzzing.stdlib.atof_differential_fuzz
[3/4] Linking CXX executable libc/fuzzing/math/libc.fuzzing.math.nextafter_differential_fuzz
[4/4] Linking CXX executable libc/fuzzing/math/libc.fuzzing.math.math_differential_fuzz
ninja: build stopped: subcommand failed.
['ninja', 'libc-fuzzer'] exited with return code 1.
The build step threw an exception...
Traceback (most recent call last):
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 176, in step
    yield
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 167, in main
    run_command(['ninja', 'libc-fuzzer'])
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg/build/../llvm-zorg/zorg/buildbot/builders/annotated/libc-linux.py", line 191, in run_command
    util.report_run_cmd(cmd, cwd=directory)
  File "/home/llvm-libc-buildbot/home/sivachandra/libc-x86_64-debian/libc-x86_64-debian-dbg/llvm-zorg/zorg/buildbot/builders/annotated/util.py", line 49, in report_run_cmd
    subprocess.check_call(cmd, shell=shell, *args, **kwargs)
  File "/usr/lib/python3.11/subprocess.py", line 413, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['ninja', 'libc-fuzzer']' returned non-zero exit status 1.
@@@STEP_FAILURE@@@

tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
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: llvm#143937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants