From 66946b7f215503f5ef57fd5ab574a1011039ce5d Mon Sep 17 00:00:00 2001 From: Michael Jones 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 @@ -28,24 +23,18 @@ namespace shm_common { LIBC_INLINE_VAR constexpr cpp::string_view SHM_PREFIX = "/dev/shm/"; using SHMPath = cpp::array; -LIBC_INLINE cpp::optional translate_name(cpp::string_view name) { +LIBC_INLINE ErrorOr 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 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 // 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 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(SYS_unlink, path_result->data()); +#elif defined(SYS_unlinkat) + int ret = LIBC_NAMESPACE::syscall_impl(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 - 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 -#include -#include - 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 -#include -#include #include -#include 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 - 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 using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher; using LlvmLibcShmTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;