[libc][sched] Implement proxy headers for cpu_set_t, struct sched_param and sched_macros.
#126303
Conversation
|
@llvm/pr-subscribers-libc Author: Krishna Pandey (krishna2803) ChangesFixes #126287 Full diff: https://github.com/llvm/llvm-project/pull/126303.diff 17 Files Affected:
diff --git a/libc/hdr/types/cpu_set_t.h b/libc/hdr/types/cpu_set_t.h
new file mode 100644
index 000000000000000..26aed7592fa2c58
--- /dev/null
+++ b/libc/hdr/types/cpu_set_t.h
@@ -0,0 +1,22 @@
+//===-- Proxy for cpu_set_t -----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_HDR_TYPES_CPU_SET_T_H
+#define LLVM_LIBC_HDR_TYPES_CPU_SET_T_H
+
+#ifdef LIBC_FULL_BUILD
+
+#include "include/llvm-libc-types/cpu_set_t.h"
+
+#else // Overlay mode
+
+#include <sched.h>
+
+#endif // LLVM_LIBC_FULL_BUILD
+
+#endif // LLVM_LIBC_HDR_TYPES_CPU_SET_T_H
diff --git a/libc/src/sched/linux/sched_getaffinity.cpp b/libc/src/sched/linux/sched_getaffinity.cpp
index 7b1fd8c5aa2aff5..41c7731142aebdd 100644
--- a/libc/src/sched/linux/sched_getaffinity.cpp
+++ b/libc/src/sched/linux/sched_getaffinity.cpp
@@ -13,7 +13,7 @@
#include "src/__support/macros/config.h"
#include "src/errno/libc_errno.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
#include <stdint.h>
#include <sys/syscall.h> // For syscall numbers.
diff --git a/libc/src/sched/linux/sched_getcpucount.cpp b/libc/src/sched/linux/sched_getcpucount.cpp
index dbda4b2c789ad26..849123d9b960e3a 100644
--- a/libc/src/sched/linux/sched_getcpucount.cpp
+++ b/libc/src/sched/linux/sched_getcpucount.cpp
@@ -11,7 +11,7 @@
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
#include <stddef.h>
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/linux/sched_setaffinity.cpp b/libc/src/sched/linux/sched_setaffinity.cpp
index cad48c26bf93835..a131bda2bb5eb94 100644
--- a/libc/src/sched/linux/sched_setaffinity.cpp
+++ b/libc/src/sched/linux/sched_setaffinity.cpp
@@ -13,7 +13,7 @@
#include "src/__support/macros/config.h"
#include "src/errno/libc_errno.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
#include <sys/syscall.h> // For syscall numbers.
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_getaffinity.h b/libc/src/sched/sched_getaffinity.h
index 52ec5bca6b22a21..de8a14d51daa99c 100644
--- a/libc/src/sched/sched_getaffinity.h
+++ b/libc/src/sched/sched_getaffinity.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_GETAFFINITY_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_getcpucount.h b/libc/src/sched/sched_getcpucount.h
index 8f35301ca6c0d43..f04099f8db0c45f 100644
--- a/libc/src/sched/sched_getcpucount.h
+++ b/libc/src/sched/sched_getcpucount.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_GETCPUCOUNT_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
#include <stddef.h>
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_getparam.h b/libc/src/sched/sched_getparam.h
index e1b23656d585958..03bfc008be95b85 100644
--- a/libc/src/sched/sched_getparam.h
+++ b/libc/src/sched/sched_getparam.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_GETPARAM_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_getscheduler.h b/libc/src/sched/sched_getscheduler.h
index d29e902180ea8ab..970b96097d33d1d 100644
--- a/libc/src/sched/sched_getscheduler.h
+++ b/libc/src/sched/sched_getscheduler.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_GETSCHEDULER_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_rr_get_interval.h b/libc/src/sched/sched_rr_get_interval.h
index ff093298a3bb341..b8bf17497cb3292 100644
--- a/libc/src/sched/sched_rr_get_interval.h
+++ b/libc/src/sched/sched_rr_get_interval.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_RR_GET_INTERVAL_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_setaffinity.h b/libc/src/sched/sched_setaffinity.h
index cb2303dd813e23e..368aac0cc5bf98b 100644
--- a/libc/src/sched/sched_setaffinity.h
+++ b/libc/src/sched/sched_setaffinity.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_SETAFFINITY_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_setparam.h b/libc/src/sched/sched_setparam.h
index e4691a7912000fc..cf5b2840c4551f7 100644
--- a/libc/src/sched/sched_setparam.h
+++ b/libc/src/sched/sched_setparam.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_SETPARAM_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/src/sched/sched_setscheduler.h b/libc/src/sched/sched_setscheduler.h
index e745002c0d961cf..5c99aa2d10a004a 100644
--- a/libc/src/sched/sched_setscheduler.h
+++ b/libc/src/sched/sched_setscheduler.h
@@ -10,7 +10,7 @@
#define LLVM_LIBC_SRC_SCHED_SCHED_SETSCHEDULER_H
#include "src/__support/macros/config.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
namespace LIBC_NAMESPACE_DECL {
diff --git a/libc/test/src/sched/affinity_test.cpp b/libc/test/src/sched/affinity_test.cpp
index b5085203e5ce0a4..5f7d6571b481e51 100644
--- a/libc/test/src/sched/affinity_test.cpp
+++ b/libc/test/src/sched/affinity_test.cpp
@@ -12,7 +12,7 @@
#include "src/sched/sched_setaffinity.h"
#include "test/UnitTest/ErrnoSetterMatcher.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
#include <sys/syscall.h>
TEST(LlvmLibcSchedAffinityTest, SmokeTest) {
diff --git a/libc/test/src/sched/cpu_count_test.cpp b/libc/test/src/sched/cpu_count_test.cpp
index 5250368a261622e..b0c7cfe8614a05c 100644
--- a/libc/test/src/sched/cpu_count_test.cpp
+++ b/libc/test/src/sched/cpu_count_test.cpp
@@ -12,7 +12,7 @@
#include "src/sched/sched_getcpucount.h"
#include "test/UnitTest/ErrnoSetterMatcher.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
#include <sys/syscall.h>
TEST(LlvmLibcSchedCpuCountTest, SmokeTest) {
diff --git a/libc/test/src/sched/get_priority_test.cpp b/libc/test/src/sched/get_priority_test.cpp
index 59205c51e4a164f..fe437cda4d7b6c1 100644
--- a/libc/test/src/sched/get_priority_test.cpp
+++ b/libc/test/src/sched/get_priority_test.cpp
@@ -11,7 +11,7 @@
#include "src/sched/sched_get_priority_min.h"
#include "test/UnitTest/Test.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
TEST(LlvmLibcSchedGetPriorityTest, HandleBadPolicyTest) {
diff --git a/libc/test/src/sched/param_and_scheduler_test.cpp b/libc/test/src/sched/param_and_scheduler_test.cpp
index 747c7e3409e413f..c5d6217e75b3210 100644
--- a/libc/test/src/sched/param_and_scheduler_test.cpp
+++ b/libc/test/src/sched/param_and_scheduler_test.cpp
@@ -16,7 +16,7 @@
#include "src/unistd/getuid.h"
#include "test/UnitTest/Test.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
// We Test:
// SCHED_OTHER, SCHED_FIFO, SCHED_RR
diff --git a/libc/test/src/sched/sched_rr_get_interval_test.cpp b/libc/test/src/sched/sched_rr_get_interval_test.cpp
index c22a2c76d743cda..2dadf8bd7df3a62 100644
--- a/libc/test/src/sched/sched_rr_get_interval_test.cpp
+++ b/libc/test/src/sched/sched_rr_get_interval_test.cpp
@@ -14,7 +14,7 @@
#include "src/unistd/getuid.h"
#include "test/UnitTest/Test.h"
-#include <sched.h>
+#include "hdr/types/cpu_set_t.h"
TEST(LlvmLibcSchedRRGetIntervalTest, SmokeTest) {
LIBC_NAMESPACE::libc_errno = 0;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
|
||
| #include <sched.h> | ||
| #include "hdr/types/cpu_set_t.h" | ||
| #include <stddef.h> |
There was a problem hiding this comment.
What does this file use from stddef.h?
Please explicitly include libc/hdr/types/size_t.h for size_t.
libc/src/sched/sched_getparam.h
Outdated
|
|
||
| #include "src/__support/macros/config.h" | ||
| #include <sched.h> | ||
| #include "hdr/types/cpu_set_t.h" |
There was a problem hiding this comment.
Add libc/hdr/types/pid_t.h and ...oh look, we need a proxy header for struct sched_param, too! Want to put down this PR and get that resolved in a distinct PR, too?
This function signature isn't even using cpu_set_t, so it's spurious to include it!
There was a problem hiding this comment.
we need a proxy header for struct sched_param, too!
aah.. this is weird, i have a feeling that this could be the case for some other stuff too! but i think we should cpuset includes done first and then maybe we can work on struct sched_param (and the other types if there are any).
There was a problem hiding this comment.
aah.. this is weird, i have a feeling that this could be the case for some other stuff too! but i think we should cpuset includes done first and then maybe we can work on struct sched_param (and the other types if there are any).
and... this caused me a lot of trouble with the tests :(
i guess i will just do the fix in this PR only as it is messing with the tests
There was a problem hiding this comment.
/home/krishna/OpenSource/llvm-project/build/./bin/clang++ --target=x86_64-unknown-linux-gnu -DLIBC_NAMESPACE=__llvm_libc_21_0_0_git -D_DEBUG -I/home/krishna/OpenSource/llvm-project/libc -isystem /home/krishna/OpenSource/llvm-project/build/runtimes/runtimes-bins/libc/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -g -fpie -ffixed-point -Wextra-semi -Wno-c99-extensions -Wno-gnu-imaginary-constant -Wno-pedantic -std=gnu++17 -MD -MT libc/test/src/sched/CMakeFiles/libc.test.src.sched.get_priority_test.__build__.dir/get_priority_test.cpp.o -MF libc/test/src/sched/CMakeFiles/libc.test.src.sched.get_priority_test.__build__.dir/get_priority_test.cpp.o.d -o libc/test/src/sched/CMakeFiles/libc.test.src.sched.get_priority_test.__build__.dir/get_priority_test.cpp.o -c /home/krishna/OpenSource/llvm-project/libc/test/src/sched/get_priority_test.cpp
/home/krishna/OpenSource/llvm-project/libc/test/src/sched/get_priority_test.cpp:66:18: error: use of undeclared identifier 'SCHED_OTHER'
66 | int policy = SCHED_OTHER;
| ^
/home/krishna/OpenSource/llvm-project/libc/test/src/sched/get_priority_test.cpp:80:18: error: use of undeclared identifier 'SCHED_FIFO'
80 | int policy = SCHED_FIFO;
| ^
/home/krishna/OpenSource/llvm-project/libc/test/src/sched/get_priority_test.cpp:94:18: error: use of undeclared identifier 'SCHED_RR'
94 | int policy = SCHED_RR;
| ^
3 errors generated.
and.. i guess we need to create proxy headers for sched-macros.h as well..
There was a problem hiding this comment.
yep! heh, sorry, sometimes when there's more than 1 bug, we don't have visibility beyond the first. It looked like cpuset includes were the only issue, but fixing it uncovers other issues that need to be addressed first. Thanks for digging into them.
libc/src/sched/sched_getscheduler.h
Outdated
|
|
||
| #include "src/__support/macros/config.h" | ||
| #include <sched.h> | ||
| #include "hdr/types/cpu_set_t.h" |
There was a problem hiding this comment.
Thanks for the patch! I'm stopping my review here, because this issue pertains to the rest of the PR below I assume.
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
krishna2803
changed the title
cpu_set_t, struct sched_param and sched_macros.
krishna2803
force-pushed
the
fix-cpuset-includes
branch
from
121296d to
2028cff
Compare
Signed-off-by: krishna2803 <[email protected]>
krishna2803
force-pushed
the
fix-cpuset-includes
branch
from
2028cff to
ece948f
Compare
nickdesaulniers
left a comment
nickdesaulniers
left a comment
There was a problem hiding this comment.
Cool, now the cmake needs to get cleaned up.
| // If a processor with more than 1024 CPUs is to be supported in future, | ||
| // we need to adjust the size of this array. | ||
| unsigned long __mask[128 / sizeof(unsigned long)]; | ||
| unsigned long __bits[128 / sizeof(unsigned long)]; |
There was a problem hiding this comment.
What's up with this change?
There was a problem hiding this comment.
so the glibc implementation of cpu_set_t uses __bits instead of __mask and when we're in overlay mode, this would include sched.h from the system's libc and that might cause bugs if glibc is being used in the system
ref: https://elixir.bootlin.com/glibc/glibc-2.41/source/posix/bits/cpu-set.h#L38-L43
There was a problem hiding this comment.
when we're in overlay mode, this would include sched.h from the system's libc
Right, and libc/include/llvm-libc-types/cpu_set_t.h should only be included in fullbuild mode, not overlay mode.
| int result = 0; | ||
| for (size_t i = 0; i < cpuset_size / sizeof(long); ++i) { | ||
| result += __builtin_popcountl(mask->__mask[i]); | ||
| result += __builtin_popcountl(mask->__bits[i]); |
There was a problem hiding this comment.
Why this change?
Also, while you're here, mind removing the {} in the for?
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
libc/src/sched/sched_setparam.h
Outdated
|
|
||
| #include "src/__support/macros/config.h" | ||
| #include <sched.h> | ||
| #include "hdr/types/cpu_set_t.h" |
| #include "test/UnitTest/ErrnoSetterMatcher.h" | ||
|
|
||
| #include <sched.h> | ||
| #include "hdr/sched_macros.h" |
There was a problem hiding this comment.
What is this test using from this header? I couldn't spot it, quickly.
There was a problem hiding this comment.
ah right, it's the macro: CPU_COUNT
There was a problem hiding this comment.
hmm now that i think of it, we might not need to do LIBC_NAMESPACE::CPU_COUNT(&mask) now and CPU_COUNT(&mask) should suffice as we don't include the system's sched.h now
libc/src/sched/linux/CMakeLists.txt
Outdated
| HDRS | ||
| ../sched_getaffinity.h | ||
| DEPENDS | ||
| libc.include.sched |
There was a problem hiding this comment.
| libc.include.sched |
libc/src/sched/linux/CMakeLists.txt
Outdated
| HDRS | ||
| ../sched_getscheduler.h | ||
| DEPENDS | ||
| libc.include.sched |
There was a problem hiding this comment.
| libc.include.sched |
libc/src/sched/linux/CMakeLists.txt
Outdated
| DEPENDS | ||
| libc.include.sched | ||
| libc.hdr.types.pid_t | ||
| libc.include.sys_syscall |
There was a problem hiding this comment.
| libc.include.sys_syscall |
libc/src/sched/linux/CMakeLists.txt
Outdated
| ../sched_rr_get_interval.h | ||
| DEPENDS | ||
| libc.include.sys_syscall | ||
| libc.include.sched |
There was a problem hiding this comment.
| libc.include.sched |
libc/src/sched/linux/CMakeLists.txt
Outdated
| HDRS | ||
| ../sched_rr_get_interval.h | ||
| DEPENDS | ||
| libc.include.sys_syscall |
There was a problem hiding this comment.
| libc.include.sys_syscall |
libc/test/src/sched/CMakeLists.txt
Outdated
| SRCS | ||
| affinity_test.cpp | ||
| DEPENDS | ||
| libc.include.sched |
There was a problem hiding this comment.
Remove libc.include.sched from the cmake for each file you removed #include <sched.h> from.
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
|
The presubmit failures are curious: So it looks like scudo is including <sched.h>, which is including the host's sched.h (not ours), which includes time.h (ours), which leads to the redefinition since ours collides with the hosts... I wonder if something in the list of includes in compiler-rt/lib/scudo/standalone/linux.cpp is something we don't provide, so the build is falling back to using the host headers (for fullbuild, which is a hermeticity issue in llvm-libc's build!), which transitively include the host's struct_timespec.h somehow... Probably going to have to run that compiler invocation with |
|
Just tried to check out your branch and reproduce locally, and could not. That's fun. |
Things like these get me very confused and in the end i waste a lot of time, but i guess that's the part of the process haha. |
|
and this was all fine till ece948f3 |
| // If a processor with more than 1024 CPUs is to be supported in future, | ||
| // we need to adjust the size of this array. | ||
| unsigned long __mask[128 / sizeof(unsigned long)]; | ||
| unsigned long __bits[128 / sizeof(unsigned long)]; |
There was a problem hiding this comment.
when we're in overlay mode, this would include sched.h from the system's libc
Right, and libc/include/llvm-libc-types/cpu_set_t.h should only be included in fullbuild mode, not overlay mode.
libc/hdr/CMakeLists.txt
Outdated
| HDRS | ||
| sched_macros.h | ||
| FULL_BUILD_DEPENDS | ||
| libc.include.llvm-libc-macros.sched_macros |
There was a problem hiding this comment.
Can you test adding a line here:
| libc.include.llvm-libc-macros.sched_macros | |
| libc.include.sched |
It's ultimately not correct, but @michaelrj-google and I have a hypothesis that anchoring a dependency in libc/config/{OS}/{ARCH}/entrypoints.txt has a race for fullbuild when using scudo.
Please just add a commit on top with that one line change; if it doesn't pass presubmit, we can drop it+force push.
If it does, we can merge this PR and file a bug/todo about looking into that race.
There was a problem hiding this comment.
I got the following errors when trying this PR locally:
$ cmake -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_RUNTIMES="libc;compiler-rt" -DLLVM_LIBC_FULL_BUILD=ON -DLLVM_LIBC_INCLUDE_SCUDO=ON -DCOMPILER_RT_BUILD_SCUDO_STANDALONE_WITH_LLVM_LIBC=ON -DCOMPILER_RT_BUILD_GWP_ASAN=OFF -DCOMPILER_RT_SCUDO_STANDALONE_BUILD_SHARED=OFF -G Ninja -S ../runtimes
...
$ ninja libc
[826/826] Linking CXX static library libc/lib/libc.a
$ ninja check-libc
[5499/9162] Building CXX object libc/tes..._test.__build__.dir/cpu_count_test.cpp.o
FAILED: libc/test/src/sched/CMakeFiles/libc.test.src.sched.cpu_count_test.__build__.dir/cpu_count_test.cpp.o
/usr/bin/clang++ -DLIBC_NAMESPACE=__llvm_libc_21_0_0_git -I/usr/local/google/home/lntue/experiment/llvm-project/libc -isystem /usr/local/google/home/lntue/experiment/llvm-project/build/libc/include -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O3 -DNDEBUG -std=gnu++17 -fpie -DLIBC_FULL_BUILD -ffreestanding -fno-exceptions -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -Wimplicit-fallthrough -Wwrite-strings -Wno-c99-extensions -Wno-gnu-imaginary-constant -Wno-pedantic -Wstrict-prototypes -Wextra-semi -Wnewline-eof -Wnonportable-system-include-path -Wthread-safety -MD -MT libc/test/src/sched/CMakeFiles/libc.test.src.sched.cpu_count_test.__build__.dir/cpu_count_test.cpp.o -MF libc/test/src/sched/CMakeFiles/libc.test.src.sched.cpu_count_test.__build__.dir/cpu_count_test.cpp.o.d -o libc/test/src/sched/CMakeFiles/libc.test.src.sched.cpu_count_test.__build__.dir/cpu_count_test.cpp.o -c /usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/sched/cpu_count_test.cpp
/usr/local/google/home/lntue/experiment/llvm-project/libc/test/src/sched/cpu_count_test.cpp:32:18: error: use of undeclared identifier '__sched_getcpucount'; did you mean '__llvm_libc_21_0_0_git::__sched_getcpucount'?
int num_cpus = CPU_COUNT(&mask);
^
/usr/local/google/home/lntue/experiment/llvm-project/libc/include/llvm-libc-macros/linux/sched-macros.h:27:24: note: expanded from macro 'CPU_COUNT'
#define CPU_COUNT(set) CPU_COUNT_S(sizeof(cpu_set_t), set)
^
/usr/local/google/home/lntue/experiment/llvm-project/libc/include/llvm-libc-macros/linux/sched-macros.h:26:35: note: expanded from macro 'CPU_COUNT_S'
#define CPU_COUNT_S(setsize, set) __sched_getcpucount(setsize, set)
^
/usr/local/google/home/lntue/experiment/llvm-project/libc/src/sched/sched_getcpucount.h:20:5: note: '__llvm_libc_21_0_0_git::__sched_getcpucount' declared here
int __sched_getcpucount(size_t cpuset_size, const cpu_set_t *mask);
^
1 error generated.
[5628/9162] Building CXX object libc/tes...st.__unit__.__build__.dir/op_tests.cpp.o
ninja: build stopped: subcommand failed.
There was a problem hiding this comment.
oh, so changing from LIBC_NAMESPACE::CPU_COUNT to just CPU_COUNT is causing problems in full build mode.. i'll just keep it as it was
libc/hdr/CMakeLists.txt
Outdated
There was a problem hiding this comment.
you still need the dependency on libc.include.llvm-libc-macros.sched_macros in full build mode.
…mode Signed-off-by: krishna2803 <[email protected]>
krishna2803
force-pushed
the
fix-cpuset-includes
branch
from
d0f0281 to
b34baef
Compare
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
This reverts commit 7b48a0c. Signed-off-by: krishna2803 <[email protected]>
libc/hdr/sched_macros.h
Outdated
|
|
||
| #ifdef LIBC_FULL_BUILD | ||
|
|
||
| #ifndef _SCHED_H |
There was a problem hiding this comment.
This should not be in here. You are using this to suppress the system/glibc sched.h definitions. We should investigate and make sure the header inclusions are separated in full build mode instead.
There was a problem hiding this comment.
If for some reason, the system's usr/include/sched.h and libc/hdr/sched_macros.h are included in the same translation unit, we need to do one of the following options:
- Fulfilling our
sched.hrelated headers to be complete enough so thatusr/include/sched.his not needed anymore in full build mode - Update our implementations/tests so that
usr/include/sched.his not needed anymore in full build mode
Let me know if you need help tracing back to how all of those inclusion conflicts are happening.
There was a problem hiding this comment.
guess i'll just have to work on this using the github workflow failures only lol. I tried:
cmake -S ../runtimes -GNinja -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_RUNTIMES=libc -DLLVM_USE_LINKER=lld LLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_RUNTIMES="libc;compiler-rt" -DCOMPILER_RT_SCUDO_STANDALONE_BUILD_SHARED=OFF -DCOMPILER_RT_BUILD_GWP_ASAN=OFF -DLLVM_LIBC_INCLUDE_SCUDO=ON -DCOMPILER_RT_BUILD_SCUDO_STANDALONE_WITH_LLVM_LIBC=ON -DLLVM_LIBC_FULL_BUILD=ON
followed by:
ninja check-libc
and i'm not able to reproduce these errors (on X86) so these are specific to aarch64 and i have no other way of testing for that architecture.
There was a problem hiding this comment.
The source compiler-rt/lib/scudo/standalone/linux.cpp that is causing the errors uses sched.h:
in the function getNumberOfCPUs:
llvm-project/compiler-rt/lib/scudo/standalone/linux.cpp
Lines 164 to 171 in 3f67c42
which is declared in common.h
llvm-project/compiler-rt/lib/scudo/standalone/common.h
Lines 170 to 171 in 3f67c42
we already have definitions for cpu_set_t and sched_getaffinity but not for CPU_COUNT. I planned on first implementing the proxy headers for sched.h related definitions and then work on #124642 for the implementation of those macros but it seems there's a cyclic dependency within these issues. Since sched.h is not too big of a header, I think we should go with the 1st approach:
- Fulfilling our sched.h related headers to be complete enough so that usr/include/sched.h is not needed anymore in full build mode
@lntue How should I proceed? Should I complete #124642 prior to this, or implement CPU_COUNT within this PR only, which should complete the requirements for Scudo for now?
There was a problem hiding this comment.
Oh, I missed it. we do have a definition for CPU_COUNT here:
llvm-project/libc/include/llvm-libc-macros/linux/sched-macros.h
Lines 26 to 27 in 3f67c42
that would mean we do have a complete enough sched.h implementation so the bug shouldn't be because of some missing implementation (at least for now) which then means that the bug is due to the order of includes only?
Signed-off-by: krishna2803 <[email protected]>
This reverts commit de1eead. Signed-off-by: krishna2803 <[email protected]>
This reverts commit 55849ca. Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
Signed-off-by: krishna2803 <[email protected]>
|
commit 3f67c429 suggests we can safely remove the include for |
krishna2803
force-pushed
the
fix-cpuset-includes
branch
from
c2babfc to
3f67c42
Compare
4 participants
Fixes #126287