[libc-commits] [libc] [libc] Cleanup mman functions and tests (PR #159657)
Michael Jones via libc-commits
libc-commits at lists.llvm.org
Thu Sep 18 14:45:11 PDT 2025
https://github.com/michaelrj-google created https://github.com/llvm/llvm-project/pull/159657
Remove functions depending on each other, cleanup internal errno in
shm_common, remove various unnecessary includes.
>From 66946b7f215503f5ef57fd5ab574a1011039ce5d Mon Sep 17 00:00:00 2001
From: Michael Jones <michaelrj at google.com>
Date: Thu, 18 Sep 2025 21:37:51 +0000
Subject: [PATCH] [libc] Cleanup mman functions and tests
Remove functions depending on each other, cleanup internal errno in
shm_common, remove various unnecessary includes.
---
libc/src/sys/mman/linux/CMakeLists.txt | 9 +++---
libc/src/sys/mman/linux/shm_common.h | 29 ++++++-----------
libc/src/sys/mman/linux/shm_open.cpp | 22 +++++++------
libc/src/sys/mman/linux/shm_unlink.cpp | 32 +++++++++++++++----
libc/test/src/sys/mman/linux/madvise_test.cpp | 2 --
libc/test/src/sys/mman/linux/mincore_test.cpp | 5 ---
libc/test/src/sys/mman/linux/mlock_test.cpp | 7 ++--
libc/test/src/sys/mman/linux/mremap_test.cpp | 2 --
libc/test/src/sys/mman/linux/shm_test.cpp | 2 --
9 files changed, 55 insertions(+), 55 deletions(-)
diff --git a/libc/src/sys/mman/linux/CMakeLists.txt b/libc/src/sys/mman/linux/CMakeLists.txt
index 89a0ad1527a06..7181bb98a187f 100644
--- a/libc/src/sys/mman/linux/CMakeLists.txt
+++ b/libc/src/sys/mman/linux/CMakeLists.txt
@@ -184,11 +184,10 @@ add_header_library(
HDRS
shm_common.h
DEPENDS
+ libc.hdr.errno_macros
libc.src.__support.CPP.array
libc.src.__support.CPP.string_view
- libc.src.__support.CPP.optional
- libc.src.__support.common
- libc.src.errno.errno
+ libc.src.__support.error_or
libc.src.string.memory_utils.inline_memcpy
)
@@ -199,8 +198,8 @@ add_entrypoint_object(
HDRS
../shm_open.h
DEPENDS
- libc.src.fcntl.open
libc.hdr.types.mode_t
+ libc.src.errno.errno
.shm_common
)
@@ -211,6 +210,6 @@ add_entrypoint_object(
HDRS
../shm_unlink.h
DEPENDS
- libc.src.unistd.unlink
+ libc.src.errno.errno
.shm_common
)
diff --git a/libc/src/sys/mman/linux/shm_common.h b/libc/src/sys/mman/linux/shm_common.h
index 29d1401821e49..9ba8fd1ea100c 100644
--- a/libc/src/sys/mman/linux/shm_common.h
+++ b/libc/src/sys/mman/linux/shm_common.h
@@ -6,18 +6,13 @@
//
//===----------------------------------------------------------------------===//
+#include "hdr/errno_macros.h"
#include "src/__support/CPP/array.h"
-#include "src/__support/CPP/optional.h"
#include "src/__support/CPP/string_view.h"
-#include "src/__support/libc_errno.h"
+#include "src/__support/error_or.h"
#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>
@@ -28,24 +23,18 @@ namespace shm_common {
LIBC_INLINE_VAR constexpr cpp::string_view SHM_PREFIX = "/dev/shm/";
using SHMPath = cpp::array<char, NAME_MAX + SHM_PREFIX.size() + 1>;
-LIBC_INLINE cpp::optional<SHMPath> translate_name(cpp::string_view name) {
+LIBC_INLINE ErrorOr<SHMPath> translate_name(cpp::string_view name) {
// trim leading slashes
size_t offset = name.find_first_not_of('/');
- if (offset == cpp::string_view::npos) {
- libc_errno = EINVAL;
- return cpp::nullopt;
- }
+ if (offset == cpp::string_view::npos)
+ return Error(EINVAL);
name = name.substr(offset);
// check the name
- if (name.size() > NAME_MAX) {
- libc_errno = ENAMETOOLONG;
- return cpp::nullopt;
- }
- if (name == "." || name == ".." || name.contains('/')) {
- libc_errno = EINVAL;
- return cpp::nullopt;
- }
+ if (name.size() > NAME_MAX)
+ return Error(ENAMETOOLONG);
+ if (name == "." || name == ".." || name.contains('/'))
+ return Error(EINVAL);
// prepend the prefix
SHMPath buffer;
diff --git a/libc/src/sys/mman/linux/shm_open.cpp b/libc/src/sys/mman/linux/shm_open.cpp
index 3099062eace98..46231ba1279a8 100644
--- a/libc/src/sys/mman/linux/shm_open.cpp
+++ b/libc/src/sys/mman/linux/shm_open.cpp
@@ -7,9 +7,11 @@
//===----------------------------------------------------------------------===//
#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/libc_errno.h"
#include "src/__support/macros/config.h"
#include "src/sys/mman/linux/shm_common.h"
@@ -18,17 +20,19 @@ 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)) {
- if (cpp::optional<shm_common::SHMPath> buffer =
- shm_common::translate_name(name)) {
- auto result = internal::open(buffer->data(), oflags | DEFAULT_OFLAGS, mode);
+ auto path_result = shm_common::translate_name(name);
+ if (!path_result.has_value()) {
+ libc_errno = path_result.error();
+ return -1;
+ }
- if (!result.has_value()) {
- libc_errno = result.error();
- return -1;
- }
- return result.value();
+ auto open_result =
+ internal::open(path_result->data(), oflags | DEFAULT_OFLAGS, mode);
+ if (!open_result.has_value()) {
+ libc_errno = open_result.error();
+ return -1;
}
- return -1;
+ return open_result.value();
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/sys/mman/linux/shm_unlink.cpp b/libc/src/sys/mman/linux/shm_unlink.cpp
index 4c61c7cd16bad..7671b1918b83c 100644
--- a/libc/src/sys/mman/linux/shm_unlink.cpp
+++ b/libc/src/sys/mman/linux/shm_unlink.cpp
@@ -7,20 +7,38 @@
//===----------------------------------------------------------------------===//
#include "src/sys/mman/shm_unlink.h"
+
+#include "hdr/fcntl_macros.h"
+#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
+#include "src/__support/libc_errno.h" // For internal errno.
#include "src/__support/macros/config.h"
#include "src/sys/mman/linux/shm_common.h"
-#include "src/unistd/unlink.h"
+#include <sys/syscall.h> // For SYS_unlink, SYS_unlinkat
namespace LIBC_NAMESPACE_DECL {
-// TODO: stop calling the public unlink function. It should be calling an
-// internal shared utility.
+// TODO: move the unlink syscall to a shared utility.
LLVM_LIBC_FUNCTION(int, shm_unlink, (const char *name)) {
- if (cpp::optional<shm_common::SHMPath> buffer =
- shm_common::translate_name(name))
- return LIBC_NAMESPACE::unlink(buffer->data());
- return -1;
+ auto path_result = shm_common::translate_name(name);
+ if (!path_result.has_value()) {
+ libc_errno = path_result.error();
+ return -1;
+ }
+#ifdef SYS_unlink
+ int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_unlink, path_result->data());
+#elif defined(SYS_unlinkat)
+ int ret = LIBC_NAMESPACE::syscall_impl<int>(SYS_unlinkat, AT_FDCWD,
+ path_result->data(), 0);
+#else
+#error "unlink and unlinkat syscalls not available."
+#endif
+
+ if (ret < 0) {
+ libc_errno = -ret;
+ return -1;
+ }
+ return ret;
}
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/test/src/sys/mman/linux/madvise_test.cpp b/libc/test/src/sys/mman/linux/madvise_test.cpp
index 6671050a28038..b7c3f0571571c 100644
--- a/libc/test/src/sys/mman/linux/madvise_test.cpp
+++ b/libc/test/src/sys/mman/linux/madvise_test.cpp
@@ -13,8 +13,6 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
-#include <sys/mman.h>
-
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
using LlvmLibcMadviseTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
diff --git a/libc/test/src/sys/mman/linux/mincore_test.cpp b/libc/test/src/sys/mman/linux/mincore_test.cpp
index ade620b838a38..3a15291564922 100644
--- a/libc/test/src/sys/mman/linux/mincore_test.cpp
+++ b/libc/test/src/sys/mman/linux/mincore_test.cpp
@@ -6,7 +6,6 @@
//
//===----------------------------------------------------------------------===//
-#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
#include "src/sys/mman/madvise.h"
#include "src/sys/mman/mincore.h"
#include "src/sys/mman/mlock.h"
@@ -18,10 +17,6 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
-#include <sys/mman.h>
-#include <sys/syscall.h>
-#include <unistd.h>
-
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
using LlvmLibcMincoreTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
diff --git a/libc/test/src/sys/mman/linux/mlock_test.cpp b/libc/test/src/sys/mman/linux/mlock_test.cpp
index 6b81411ca604a..cd374222680f8 100644
--- a/libc/test/src/sys/mman/linux/mlock_test.cpp
+++ b/libc/test/src/sys/mman/linux/mlock_test.cpp
@@ -6,6 +6,10 @@
//
//===----------------------------------------------------------------------===//
+// TODO: Simplify these tests and split them up. mlock, mlock2, mlockall,
+// munlock, and munlockall should have separate test files which only need to
+// check our code paths (succeeds and errors).
+
#include "src/__support/OSUtil/syscall.h" // For internal syscall function.
#include "src/__support/libc_errno.h"
#include "src/sys/mman/madvise.h"
@@ -24,10 +28,7 @@
#include "test/UnitTest/Test.h"
#include <linux/capability.h>
-#include <sys/mman.h>
-#include <sys/resource.h>
#include <sys/syscall.h>
-#include <unistd.h>
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
using LlvmLibcMlockTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
diff --git a/libc/test/src/sys/mman/linux/mremap_test.cpp b/libc/test/src/sys/mman/linux/mremap_test.cpp
index 5ff774d57614a..620292a2d0109 100644
--- a/libc/test/src/sys/mman/linux/mremap_test.cpp
+++ b/libc/test/src/sys/mman/linux/mremap_test.cpp
@@ -13,8 +13,6 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
-#include <sys/mman.h>
-
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
using LlvmLibcMremapTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
diff --git a/libc/test/src/sys/mman/linux/shm_test.cpp b/libc/test/src/sys/mman/linux/shm_test.cpp
index ae555fa2f1aff..48bdf84c7270d 100644
--- a/libc/test/src/sys/mman/linux/shm_test.cpp
+++ b/libc/test/src/sys/mman/linux/shm_test.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#include "hdr/fcntl_macros.h"
-#include "src/__support/OSUtil/syscall.h"
#include "src/fcntl/fcntl.h"
#include "src/sys/mman/mmap.h"
#include "src/sys/mman/munmap.h"
@@ -18,7 +17,6 @@
#include "test/UnitTest/ErrnoCheckingTest.h"
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
-#include <sys/syscall.h>
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
using LlvmLibcShmTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
More information about the libc-commits
mailing list