[libc] Introduce a typed syscall wrapper and use it in mmap#197459
Merged
Conversation
Linux reserves a range of values (everything above -4096u, aka MAX_ERRNO) as an error value, so the check can be performed without knowing the details of the specific syscall. libc functions where these values would be a valid result (e.g. PTRACE_PEEKDATA) are implemented differently at the kernel level (e.g. returning the result through a pointer argument). The only exception are a handful of syscalls (getpid, getuid, ...) which can never fail, and where this could be an actual user/group ID (particularly on 32-bit systems). Specifically, for mmap, this lets us remove the is_valid_mmap helper and SYS_mmap2 ifdefs in various places. More generally, this can simplify many syscall wrappers as often the only thing they are doing is converting the return value into an ErrorOr.
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-libc Author: Pavel Labath (labath) ChangesLinux reserves a range of values (everything above -4096u, aka MAX_ERRNO) as an error value, so the check can be performed without knowing the details of the specific syscall. libc functions where these values would be a valid result (e.g. PTRACE_PEEKDATA) are implemented differently at the kernel level (e.g. returning the result through a pointer argument). The only exception are a handful of syscalls (getpid, getuid, ...) which can never fail, and where this could be an actual user/group ID (particularly on 32-bit systems). Specifically, for mmap, this lets us remove the is_valid_mmap helper and SYS_mmap2 ifdefs in various places. More generally, this can simplify many syscall wrappers as often the only thing they are doing is converting the return value into an ErrorOr. Full diff: https://github.com/llvm/llvm-project/pull/197459.diff 9 Files Affected:
diff --git a/libc/docs/dev/syscall_wrapper_refactor.rst b/libc/docs/dev/syscall_wrapper_refactor.rst
index 40535b04d90eb..329d0f32ec0d0 100644
--- a/libc/docs/dev/syscall_wrapper_refactor.rst
+++ b/libc/docs/dev/syscall_wrapper_refactor.rst
@@ -36,7 +36,7 @@ Example Wrapper (``src/__support/OSUtil/linux/syscall_wrappers/read.h``):
.. code-block:: c++
#include "hdr/types/ssize_t.h"
- #include "src/__support/OSUtil/linux/syscall.h" // For syscall_impl
+ #include "src/__support/OSUtil/linux/syscall.h" // For __syscall
#include "src/__support/common.h"
#include "src/__support/error_or.h"
#include "src/__support/macros/config.h"
@@ -46,11 +46,7 @@ Example Wrapper (``src/__support/OSUtil/linux/syscall_wrappers/read.h``):
namespace linux_syscalls {
LIBC_INLINE ErrorOr<ssize_t> read(int fd, void *buf, size_t count) {
- ssize_t ret = syscall_impl<ssize_t>(SYS_read, fd, buf, count);
- if (ret < 0) {
- return Error(-static_cast<int>(ret));
- }
- return ret;
+ return __syscall<ssize_t>(SYS_read, fd, buf, count);
}
} // namespace linux_syscalls
diff --git a/libc/src/__support/OSUtil/linux/CMakeLists.txt b/libc/src/__support/OSUtil/linux/CMakeLists.txt
index dc1865967a348..5d42bc67912fb 100644
--- a/libc/src/__support/OSUtil/linux/CMakeLists.txt
+++ b/libc/src/__support/OSUtil/linux/CMakeLists.txt
@@ -17,6 +17,7 @@ add_object_library(
.${LIBC_TARGET_ARCHITECTURE}.linux_${LIBC_TARGET_ARCHITECTURE}_util
libc.src.__support.common
libc.src.__support.CPP.string_view
+ libc.src.__support.error_or
libc.hdr.fcntl_macros
libc.hdr.types.struct_flock
libc.hdr.types.struct_flock64
@@ -32,6 +33,8 @@ add_header_library(
DEPENDS
libc.hdr.fcntl_macros
libc.hdr.sys_auxv_macros
+ libc.hdr.sys_mman_macros
+ libc.src.__support.OSUtil.linux.syscall_wrappers.mmap
libc.src.__support.OSUtil.osutil
libc.src.__support.common
libc.src.__support.CPP.optional
diff --git a/libc/src/__support/OSUtil/linux/auxv.h b/libc/src/__support/OSUtil/linux/auxv.h
index 1dc9bf0930b7c..a14f0ad731b05 100644
--- a/libc/src/__support/OSUtil/linux/auxv.h
+++ b/libc/src/__support/OSUtil/linux/auxv.h
@@ -11,11 +11,12 @@
#include "hdr/fcntl_macros.h" // For open flags
#include "hdr/sys_auxv_macros.h" // For AT_ macros
+#include "hdr/sys_mman_macros.h" // For mmap flags
+#include "src/__support/OSUtil/linux/syscall_wrappers/mmap.h"
#include "src/__support/OSUtil/syscall.h"
#include "src/__support/common.h"
#include "src/__support/threads/callonce.h"
-#include <linux/mman.h> // For mmap flags
#include <linux/param.h> // For EXEC_PAGESIZE
#include <linux/prctl.h> // For prctl
#include <sys/syscall.h> // For syscall numbers
@@ -82,20 +83,15 @@ LIBC_INLINE void Vector::initialize_unsafe(const Entry *auxv) {
[[gnu::cold]]
LIBC_INLINE void Vector::fallback_initialize_unsync() {
constexpr size_t AUXV_MMAP_SIZE = FALLBACK_AUXV_ENTRIES * sizeof(Entry);
-#ifdef SYS_mmap2
- constexpr int MMAP_SYSNO = SYS_mmap2;
-#else
- constexpr int MMAP_SYSNO = SYS_mmap;
-#endif
- long mmap_ret = syscall_impl<long>(MMAP_SYSNO, nullptr, AUXV_MMAP_SIZE,
- PROT_READ | PROT_WRITE,
- MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ ErrorOr<void *> mmap_ret =
+ linux_syscalls::mmap(nullptr, AUXV_MMAP_SIZE, PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
// We do not proceed if mmap fails.
- if (!linux_utils::is_valid_mmap(mmap_ret))
+ if (!mmap_ret.has_value())
return;
// Initialize the auxv array with AT_NULL entries.
- Entry *vector = reinterpret_cast<Entry *>(mmap_ret);
+ Entry *vector = static_cast<Entry *>(mmap_ret.value());
for (size_t i = 0; i < FALLBACK_AUXV_ENTRIES; ++i) {
vector[i].type = AT_NULL;
vector[i].val = AT_NULL;
diff --git a/libc/src/__support/OSUtil/linux/syscall.h b/libc/src/__support/OSUtil/linux/syscall.h
index 0e25618e81942..73aa3c250738c 100644
--- a/libc/src/__support/OSUtil/linux/syscall.h
+++ b/libc/src/__support/OSUtil/linux/syscall.h
@@ -11,6 +11,7 @@
#include "src/__support/CPP/bit.h"
#include "src/__support/common.h"
+#include "src/__support/error_or.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/optimization.h"
#include "src/__support/macros/properties/architectures.h"
@@ -29,24 +30,31 @@
namespace LIBC_NAMESPACE_DECL {
+// This function performs no error checking. For most syscalls, it's better to
+// use __syscall below.
template <typename R, typename... Ts>
LIBC_INLINE R syscall_impl(long __number, Ts... ts) {
static_assert(sizeof...(Ts) <= 6, "Too many arguments for syscall");
return cpp::bit_or_static_cast<R>(syscall_impl(__number, (long)ts...));
}
-// Linux-specific function for checking
-namespace linux_utils {
+namespace linux_syscalls {
LIBC_INLINE_VAR constexpr unsigned long MAX_ERRNO = 4095;
-// Ideally, this should be defined using PAGE_OFFSET
-// However, that is a configurable parameter. We mimic kernel's behavior
-// by checking against MAX_ERRNO.
-template <typename PointerLike>
-LIBC_INLINE constexpr bool is_valid_mmap(PointerLike ptr) {
- long addr = cpp::bit_cast<long>(ptr);
- return LIBC_LIKELY(addr > 0 || addr < -static_cast<long>(MAX_ERRNO));
+
+// Helper function to perform a system call, check the result and cast the
+// result to the expected type. This function is safe to use on most syscalls,
+// with the exception of a handful of syscalls (getpid, getuid, ...) that never
+// fail.
+template <typename R, typename... Ts>
+LIBC_INLINE ErrorOr<R> __syscall(long __number, Ts... ts) {
+ static_assert(sizeof...(Ts) <= 6, "Too many arguments");
+ unsigned long ret =
+ static_cast<unsigned long>(syscall_impl(__number, (long)ts...));
+ if (ret >= -MAX_ERRNO)
+ return Error(static_cast<int>(-ret));
+ return cpp::bit_or_static_cast<R>(ret);
}
-} // namespace linux_utils
+} // namespace linux_syscalls
} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/__support/OSUtil/linux/syscall_wrappers/mmap.h b/libc/src/__support/OSUtil/linux/syscall_wrappers/mmap.h
index 983973e70e205..aa1f2a90ec532 100644
--- a/libc/src/__support/OSUtil/linux/syscall_wrappers/mmap.h
+++ b/libc/src/__support/OSUtil/linux/syscall_wrappers/mmap.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC___SUPPORT_OSUTIL_SYSCALL_WRAPPERS_MMAP_H
#include "hdr/types/off_t.h"
-#include "src/__support/OSUtil/linux/syscall.h" // syscall_impl, linux_utils::is_valid_mmap
+#include "src/__support/OSUtil/linux/syscall.h" // For __syscall
#include "src/__support/common.h"
#include "src/__support/error_or.h"
#include "src/__support/macros/config.h"
@@ -40,17 +40,8 @@ LIBC_INLINE ErrorOr<void *> mmap(void *addr, size_t size, int prot, int flags,
// Explicit cast to silence "implicit conversion loses integer precision"
// warnings when compiling for 32-bit systems.
- long ret =
- syscall_impl<long>(syscall_number, reinterpret_cast<long>(addr), size,
- prot, flags, fd, static_cast<long>(offset));
-
- // A negative return value from the syscall can also be an error-free
- // value returned by the syscall. However, since a valid return address
- // cannot be within the last page, a return value corresponding to a
- // location in the last page is an error value.
- if (!linux_utils::is_valid_mmap(ret))
- return Error(-static_cast<int>(ret));
- return reinterpret_cast<void *>(ret);
+ return __syscall<void *>(syscall_number, reinterpret_cast<long>(addr), size,
+ prot, flags, fd, static_cast<long>(offset));
}
} // namespace linux_syscalls
diff --git a/libc/src/__support/threads/linux/CMakeLists.txt b/libc/src/__support/threads/linux/CMakeLists.txt
index b9273b42bb5f1..8ce19634c41b1 100644
--- a/libc/src/__support/threads/linux/CMakeLists.txt
+++ b/libc/src/__support/threads/linux/CMakeLists.txt
@@ -34,12 +34,14 @@ add_object_library(
libc.include.sys_syscall
libc.hdr.fcntl_macros
libc.hdr.errno_macros
+ libc.hdr.sys_mman_macros
libc.src.errno.errno
libc.src.__support.CPP.atomic
libc.src.__support.CPP.stringstream
libc.src.__support.CPP.string_view
libc.src.__support.common
libc.src.__support.error_or
+ libc.src.__support.OSUtil.linux.syscall_wrappers.mmap
libc.src.__support.threads.thread_common
COMPILE_OPTIONS
${libc_opt_high_flag}
diff --git a/libc/src/__support/threads/linux/thread.cpp b/libc/src/__support/threads/linux/thread.cpp
index bdbb8aefeef0c..2c9744ba66338 100644
--- a/libc/src/__support/threads/linux/thread.cpp
+++ b/libc/src/__support/threads/linux/thread.cpp
@@ -11,6 +11,7 @@
#include "src/__support/CPP/atomic.h"
#include "src/__support/CPP/string_view.h"
#include "src/__support/CPP/stringstream.h"
+#include "src/__support/OSUtil/linux/syscall_wrappers/mmap.h"
#include "src/__support/OSUtil/syscall.h" // For syscall functions.
#include "src/__support/common.h"
#include "src/__support/error_or.h"
@@ -25,22 +26,14 @@
#include "hdr/errno_macros.h"
#include "hdr/fcntl_macros.h"
#include "hdr/stdint_proxy.h"
+#include "hdr/sys_mman_macros.h" // For PROT_* and MAP_* definitions.
#include <linux/param.h> // For EXEC_PAGESIZE.
#include <linux/prctl.h> // For PR_SET_NAME
#include <linux/sched.h> // For CLONE_* flags.
-#include <sys/mman.h> // For PROT_* and MAP_* definitions.
#include <sys/syscall.h> // For syscall numbers.
namespace LIBC_NAMESPACE_DECL {
-#ifdef SYS_mmap2
-static constexpr long MMAP_SYSCALL_NUMBER = SYS_mmap2;
-#elif defined(SYS_mmap)
-static constexpr long MMAP_SYSCALL_NUMBER = SYS_mmap;
-#else
-#error "mmap or mmap2 syscalls not available."
-#endif
-
static constexpr size_t NAME_SIZE_MAX = 16; // Includes the null terminator
static constexpr uint32_t CLEAR_TID_VALUE = 0xABCD1234;
static constexpr unsigned CLONE_SYSCALL_FLAGS =
@@ -93,29 +86,27 @@ LIBC_INLINE ErrorOr<void *> alloc_stack(size_t stacksize, size_t guardsize) {
// TODO: Maybe add MAP_STACK? Currently unimplemented on linux but helps
// future-proof.
- long mmap_result = LIBC_NAMESPACE::syscall_impl<long>(
- MMAP_SYSCALL_NUMBER,
- 0, // No special address
- size, prot,
- MAP_ANONYMOUS | MAP_PRIVATE, // Process private.
- -1, // Not backed by any file
- 0 // No offset
- );
- if (!linux_utils::is_valid_mmap(mmap_result))
- return Error{int(-mmap_result)};
+ ErrorOr<void *> mmap_result =
+ linux_syscalls::mmap(nullptr, size, prot,
+ MAP_ANONYMOUS | MAP_PRIVATE, // Process private.
+ -1, // Not backed by any file
+ 0 // No offset
+ );
+ if (!mmap_result.has_value())
+ return mmap_result;
+
+ char *stack = static_cast<char *>(mmap_result.value()) + guardsize;
if (guardsize) {
// Give read/write permissions to actual stack.
// TODO: We are assuming stack growsdown here.
long result = LIBC_NAMESPACE::syscall_impl<long>(
- SYS_mprotect, mmap_result + guardsize, stacksize,
- PROT_READ | PROT_WRITE);
+ SYS_mprotect, stack, stacksize, PROT_READ | PROT_WRITE);
if (result != 0)
return Error{int(-result)};
}
- mmap_result += guardsize;
- return reinterpret_cast<void *>(mmap_result);
+ return stack;
}
// This must always be inlined as we may be freeing the calling threads stack in
diff --git a/libc/startup/linux/x86_64/CMakeLists.txt b/libc/startup/linux/x86_64/CMakeLists.txt
index 44259944078d6..522bbc85ad663 100644
--- a/libc/startup/linux/x86_64/CMakeLists.txt
+++ b/libc/startup/linux/x86_64/CMakeLists.txt
@@ -4,9 +4,11 @@ add_startup_object(
tls.cpp
DEPENDS
libc.config.app_h
+ libc.hdr.sys_mman_macros
libc.include.sys_mman
libc.include.sys_syscall
libc.src.__support.OSUtil.osutil
+ libc.src.__support.OSUtil.linux.syscall_wrappers.mmap
libc.src.string.memory_utils.inline_memcpy
COMPILE_OPTIONS
-fno-stack-protector
diff --git a/libc/startup/linux/x86_64/tls.cpp b/libc/startup/linux/x86_64/tls.cpp
index 497f744e89e28..6d2f318e6f699 100644
--- a/libc/startup/linux/x86_64/tls.cpp
+++ b/libc/startup/linux/x86_64/tls.cpp
@@ -6,25 +6,17 @@
//
//===----------------------------------------------------------------------===//
-#include "src/__support/OSUtil/syscall.h"
+#include "hdr/sys_mman_macros.h"
+#include "src/__support/OSUtil/linux/syscall_wrappers/mmap.h"
#include "src/__support/macros/config.h"
#include "src/string/memory_utils/inline_memcpy.h"
#include "startup/linux/do_start.h"
#include <asm/prctl.h>
-#include <sys/mman.h>
#include <sys/syscall.h>
namespace LIBC_NAMESPACE_DECL {
-#ifdef SYS_mmap2
-static constexpr long MMAP_SYSCALL_NUMBER = SYS_mmap2;
-#elif SYS_mmap
-static constexpr long MMAP_SYSCALL_NUMBER = SYS_mmap;
-#else
-#error "mmap and mmap2 syscalls not available."
-#endif
-
// TODO: Also generalize this routine and handle dynamic loading properly.
void init_tls(TLSDescriptor &tls_descriptor) {
if (app.tls.size == 0) {
@@ -45,17 +37,12 @@ void init_tls(TLSDescriptor &tls_descriptor) {
// offset 0x28 (40) and is of size uintptr_t.
uintptr_t tls_size_with_addr = tls_size + sizeof(uintptr_t) + 40;
- // We cannot call the mmap function here as the functions set errno on
- // failure. Since errno is implemented via a thread local variable, we cannot
- // use errno before TLS is setup.
- long mmap_retval = syscall_impl<long>(
- MMAP_SYSCALL_NUMBER, nullptr, tls_size_with_addr, PROT_READ | PROT_WRITE,
- MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
- // We cannot check the return value with MAP_FAILED as that is the return
- // of the mmap function and not the mmap syscall.
- if (!linux_utils::is_valid_mmap(mmap_retval))
+ ErrorOr<void *> mmap_ret =
+ linux_syscalls::mmap(nullptr, tls_size_with_addr, PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+ if (!mmap_ret.has_value())
syscall_impl<long>(SYS_exit, 1);
- uintptr_t *tls_addr = reinterpret_cast<uintptr_t *>(mmap_retval);
+ uintptr_t *tls_addr = static_cast<uintptr_t *>(mmap_ret.value());
// x86_64 TLS faces down from the thread pointer with the first entry
// pointing to the address of the first real TLS byte.
|
kaladron
approved these changes
May 13, 2026
kaladron
left a comment
kaladron
left a comment
Member
There was a problem hiding this comment.
Minor nits. I love the clarity that this brings and the boilerplate reduction.
| // with the exception of a handful of syscalls (getpid, getuid, ...) that never | ||
| // fail. | ||
| template <typename R, typename... Ts> | ||
| LIBC_INLINE ErrorOr<R> __syscall(long __number, Ts... ts) { |
Member
There was a problem hiding this comment.
I don't love the __ prefix. We're already namespaced. I think it makes sense to call it something obvious with tricks that look C-ish.
Contributor
Author
There was a problem hiding this comment.
Any suggestions on the name? I wanted to call it a plain syscall, but that's a macro (defined in glibc headers, I think).
Member
There was a problem hiding this comment.
syscall_checked or something similar could work, since it's handling some of the logic itself.
Contributor
Author
There was a problem hiding this comment.
Going with syscall_checked
| namespace LIBC_NAMESPACE_DECL { | ||
|
|
||
| // This function performs no error checking. For most syscalls, it's better to | ||
| // use __syscall below. |
Member
There was a problem hiding this comment.
Once the migration is complete, does this function go away? If yes, it might be less confusing to duplicate the logic below so it's obvious to just delete this when it has no callers.
Contributor
Author
There was a problem hiding this comment.
It is actually duplicated. The other function is calling the non-template asm syscall_impl functions defined in architecture specific headers.
michaelrj-google
approved these changes
May 13, 2026
michaelrj-google
left a comment
michaelrj-google
left a comment
Member
There was a problem hiding this comment.
Overall LGTM, feel free to merge once we're done bikeshedding over the name.
SchrodingerZhu
approved these changes
May 14, 2026
michaelrj-google
approved these changes
May 14, 2026
michaelrj-google
left a comment
michaelrj-google
left a comment
Member
There was a problem hiding this comment.
syscall_checked SGTM, thanks for working on this!
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
pedroMVicente
pushed a commit
to pedroMVicente/llvm-project
that referenced
this pull request
May 19, 2026
) Linux reserves a range of values (everything above -4096u, aka MAX_ERRNO) as an error value, so the check can be performed without knowing the details of the specific syscall. libc functions where these values would be a valid result (e.g. PTRACE_PEEKDATA) are implemented differently at the kernel level (e.g. returning the result through a pointer argument). The only exception are a handful of syscalls (getpid, getuid, ...) which can never fail, and where this could be an actual user/group ID (particularly on 32-bit systems). Specifically, for mmap, this lets us remove the is_valid_mmap helper and SYS_mmap2 ifdefs in various places. More generally, this can simplify many syscall wrappers as often the only thing they are doing is converting the return value into an ErrorOr.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Linux reserves a range of values (everything above -4096u, aka MAX_ERRNO) as an error value, so the check can be performed without knowing the details of the specific syscall. libc functions where these values would be a valid result (e.g. PTRACE_PEEKDATA) are implemented differently at the kernel level (e.g. returning the result through a pointer argument). The only exception are a handful of syscalls (getpid, getuid, ...) which can never fail, and where this could be an actual user/group ID (particularly on 32-bit systems).
Specifically, for mmap, this lets us remove the is_valid_mmap helper and SYS_mmap2 ifdefs in various places.
More generally, this can simplify many syscall wrappers as often the only thing they are doing is converting the return value into an ErrorOr.