[libc] Cleanup mman functions and tests #159657
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove functions depending on each other, cleanup internal errno in shm_common, remove various unnecessary includes.
|
@llvm/pr-subscribers-libc Author: Michael Jones (michaelrj-google) ChangesRemove functions depending on each other, cleanup internal errno in Full diff: https://github.com/llvm/llvm-project/pull/159657.diff 9 Files Affected:
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;
|
vonosmas
approved these changes
Sep 18, 2025
lntue
approved these changes
Sep 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove functions depending on each other, cleanup internal errno in
shm_common, remove various unnecessary includes.