-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[libc] fix sysconf test for rv32 #162685
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
[libc] fix sysconf test for rv32 #162685
Conversation
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-libc Author: Schrodinger ZHU Yifan (SchrodingerZhu) ChangesOn 32-bit architectures, MMAP may return userspace virtual address with high bit set. This patch copy the err detection logic from existing mmap function implementation. Full diff: https://github.com/llvm/llvm-project/pull/162685.diff 1 Files Affected:
diff --git a/libc/src/__support/OSUtil/linux/auxv.h b/libc/src/__support/OSUtil/linux/auxv.h
index 894868ae5824d..cca0f358e2366 100644
--- a/libc/src/__support/OSUtil/linux/auxv.h
+++ b/libc/src/__support/OSUtil/linux/auxv.h
@@ -16,6 +16,7 @@
#include <linux/auxvec.h> // For AT_ macros
#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
@@ -90,7 +91,7 @@ LIBC_INLINE void Vector::fallback_initialize_unsync() {
PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
// We do not proceed if mmap fails.
- if (mmap_ret <= 0)
+ if (mmap_ret <= 0 && mmap_ret > -EXEC_PAGESIZE)
return;
// Initialize the auxv array with AT_NULL entries.
|
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); | ||
// We do not proceed if mmap fails. | ||
if (mmap_ret <= 0) | ||
if (mmap_ret <= 0 && mmap_ret > -EXEC_PAGESIZE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should restrict this extra condition to RV32 only, and add comments to explain the reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (ret < 0 && ret > -EXEC_PAGESIZE) { |
So the original mmap.cpp always have this check logic.
However, I read the comment again and the argument looks suspicious. EXEC_PAGESIZE
is not always the page size in general.
Inside linux kernel, the logic is defined in https://elixir.bootlin.com/linux/v6.17.1/source/tools/include/linux/err.h#L33.
I think the checking logic should be provided as a common function and used in both cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a common check function and link to the linux kernel would be great.
template <typename PointerLike> | ||
LIBC_INLINE constexpr bool is_valid_mmap(PointerLike ptr) { | ||
long addr = cpp::bit_cast<long>(ptr); | ||
return __builtin_expect(addr > 0 || addr < -cpp::bit_cast<long>(MAX_ERRNO), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use LIBC_LIKELY
from https://github.com/llvm/llvm-project/blob/main/libc/src/__support/macros/optimization.h#L27
and for casting signed / unsigned integers, static_cast
should be good enough.
Closes #162671
On 32-bit architectures, MMAP may return userspace virtual address with high bit set. This patch copy the err detection logic from existing mmap function implementation.