-
Notifications
You must be signed in to change notification settings - Fork 14.8k
libc: Introduce calls to sysconf to get page size. #163462
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
base: main
Are you sure you want to change the base?
libc: Introduce calls to sysconf to get page size. #163462
Conversation
Created using spr 1.3.6-beta.1
@llvm/pr-subscribers-libc Author: Peter Collingbourne (pcc) Changessysconf(_SC_PAGESIZE) is implemented now. Full diff: https://github.com/llvm/llvm-project/pull/163462.diff 4 Files Affected:
diff --git a/libc/test/src/sys/mman/linux/mincore_test.cpp b/libc/test/src/sys/mman/linux/mincore_test.cpp
index fb86252992def..7d470fc4b08b1 100644
--- a/libc/test/src/sys/mman/linux/mincore_test.cpp
+++ b/libc/test/src/sys/mman/linux/mincore_test.cpp
@@ -16,12 +16,13 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
+#include <unistd.h>
+
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
using LlvmLibcMincoreTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
-// TODO: Replace with sysconf call once the function is properly implemented.
-constexpr size_t PAGE_SIZE = 4096;
+const size_t PAGE_SIZE = sysconf(_SC_PAGESIZE);
TEST_F(LlvmLibcMincoreTest, UnMappedMemory) {
unsigned char vec;
diff --git a/libc/test/src/sys/mman/linux/mlock_test.cpp b/libc/test/src/sys/mman/linux/mlock_test.cpp
index f4a072ec18a31..834c513fdda0b 100644
--- a/libc/test/src/sys/mman/linux/mlock_test.cpp
+++ b/libc/test/src/sys/mman/linux/mlock_test.cpp
@@ -27,9 +27,9 @@
#include "test/UnitTest/Test.h"
#include <sys/syscall.h>
+#include <unistd.h>
-// TODO: Replace with sysconf call once the function is properly implemented.
-constexpr size_t PAGE_SIZE = 4096;
+const size_t PAGE_SIZE = sysconf(_SC_PAGESIZE);
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
using LlvmLibcMlockTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
diff --git a/libc/test/src/sys/mman/linux/msync_test.cpp b/libc/test/src/sys/mman/linux/msync_test.cpp
index 764a67d02e3b1..e99328992800b 100644
--- a/libc/test/src/sys/mman/linux/msync_test.cpp
+++ b/libc/test/src/sys/mman/linux/msync_test.cpp
@@ -15,8 +15,9 @@
#include "test/UnitTest/ErrnoSetterMatcher.h"
#include "test/UnitTest/Test.h"
-// TODO: Replace with sysconf call once the function is properly implemented.
-constexpr size_t PAGE_SIZE = 4096;
+#include <unistd.h>
+
+const size_t PAGE_SIZE = sysconf(_SC_PAGESIZE);
using namespace LIBC_NAMESPACE::testing::ErrnoSetterMatcher;
using LlvmLibcMsyncTest = LIBC_NAMESPACE::testing::ErrnoCheckingTest;
diff --git a/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp b/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp
index 094bcb2c71427..f10c8d4211631 100644
--- a/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp
+++ b/libc/test/src/sys/mman/linux/remap_file_pages_test.cpp
@@ -17,9 +17,9 @@
#include <sys/mman.h>
#include <sys/stat.h> // For S_IRWXU
+#include <unistd.h>
-// TODO: Replace with sysconf call once the function is properly implemented.
-constexpr size_t PAGE_SIZE = 4096;
+const size_t PAGE_SIZE = sysconf(_SC_PAGESIZE);
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Fails;
using LIBC_NAMESPACE::testing::ErrnoSetterMatcher::Succeeds;
|
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.
Thanks for the fix, you need to make sure to call the LLVM-libc specific sysconf
instead of getting the one from the system. You'll also need to update the cmake for the tests so that it gets linked properly. Please also update the bazel files.
#include "test/UnitTest/ErrnoSetterMatcher.h" | ||
#include "test/UnitTest/Test.h" | ||
|
||
#include <unistd.h> |
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.
this should be the LLVM-libc sysconf.
#include <unistd.h> | |
#include "src/unistd/sysconf.h" |
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.
Done
|
||
// TODO: Replace with sysconf call once the function is properly implemented. | ||
constexpr size_t PAGE_SIZE = 4096; | ||
const size_t PAGE_SIZE = sysconf(_SC_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.
you need to call the sysconf in the LLVM-libc namespace.
const size_t PAGE_SIZE = sysconf(_SC_PAGESIZE); | |
const size_t PAGE_SIZE = LIBC_NAMESPACE::sysconf(_SC_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.
It's fine for tests, but just be aware that this changes the behavior to require a global constructor.
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.
Right, I also figured that would be fine for tests
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.
Done
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.
Thanks for the fix, you need to make sure to call the LLVM-libc specific
sysconf
instead of getting the one from the system.
Will do, I'm guessing that's causing the test failures because the _SC_
values are different?
|
||
// TODO: Replace with sysconf call once the function is properly implemented. | ||
constexpr size_t PAGE_SIZE = 4096; | ||
const size_t PAGE_SIZE = sysconf(_SC_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.
Right, I also figured that would be fine for tests
Created using spr 1.3.6-beta.1
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.
You'll also need to update the cmake for the tests so that it gets linked properly.
Done
Please also update the bazel files.
Sorry, I'm not set up to build LLVM with Bazel and I don't think that contributors are required to update the alternative build system files.
#include "test/UnitTest/ErrnoSetterMatcher.h" | ||
#include "test/UnitTest/Test.h" | ||
|
||
#include <unistd.h> |
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.
Done
|
||
// TODO: Replace with sysconf call once the function is properly implemented. | ||
constexpr size_t PAGE_SIZE = 4096; | ||
const size_t PAGE_SIZE = sysconf(_SC_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.
Done
sysconf(_SC_PAGESIZE) is implemented now.