This is an archive of the discontinued LLVM Phabricator instance.

[LIBC] Implement remainder of posix 'sched.h' minus `SCHED_SPORADIC`
ClosedPublic

Authored by goldstein.w.n on Apr 11 2023, 10:31 PM.

Details

Summary

Includes macros:

linux/SCHED_OTHER // posix req
linux/SCHED_FIFO // posix req
linux/SCHED_RR // posix req
linux/SCHED_BATCH
linux/SCHED_ISO
linux/SCHED_IDLE
linux/SCHED_DEADLINE

Includes types:

struct sched_param { int sched_priority; }

Includes functions:

sched_setparam
sched_getparam
sched_setscheduler
sched_getscheduler
sched_get_priority_max
sched_get_priority_min
sched_rr_get_interval

Diff Detail

Event Timeline

goldstein.w.n created this revision.Apr 11 2023, 10:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 11 2023, 10:31 PM
goldstein.w.n requested review of this revision.Apr 11 2023, 10:31 PM

some initial comments, will do a more thorough review later today.

libc/spec/posix.td
49

this shouldn't be StructTimeSpecPtr

libc/src/sched/linux/sched_get_priority_max.cpp
21

does this syscall not have a name?

goldstein.w.n marked an inline comment as done.Apr 12 2023, 11:57 AM
goldstein.w.n added inline comments.
libc/src/sched/linux/sched_get_priority_max.cpp
21

Bah, will fix from debugging (was forgetting to return ret but went a bit crazy first).

michaelrj added inline comments.Apr 12 2023, 4:29 PM
libc/include/llvm-libc-types/struct_sched_param.h
22–36

if we don't expect to need SCHED_SPORADIC then we can leave this part out of the struct for the moment.

libc/test/src/sched/get_priority_test.cpp
36

given that there are relatively few of these it's probably better to just repeat the code of the test. It makes the file longer, but it also makes debugging easier. The ASSERT macros only report the line they're on and the failing value, so if one of these is failing it'll be hard to tell from the error which it is. If you want to you can use the {braces} to scope each of the tests, that way redefining the variables each time won't be an issue.

50

if you want, you could split the failures into a separate test.

libc/test/src/sched/param_and_scheduler_test.cpp
28–29

nit: formatting?

139–141

each of these should probably be in their own test.

libc/test/src/sched/rr_get_interval_test.cpp
37 ↗(On Diff #512922)

where is getuid included from?

goldstein.w.n marked 6 inline comments as done.Apr 12 2023, 6:13 PM
goldstein.w.n added inline comments.
libc/test/src/sched/rr_get_interval_test.cpp
37 ↗(On Diff #512922)

Hmm, so it seems the test is not actually building/running. I double checked the cmakelist and it seems right. Any idea whats going on?

sivachandra added inline comments.Apr 13 2023, 1:04 AM
libc/include/llvm-libc-macros/linux/sched-macros.h
15

Can you add a comment explaining how these numbers were picked? If they were picked to be in sync with the Linux kernel, mention the kernel header which defines them.

libc/include/llvm-libc-macros/sched-macros.h
14 ↗(On Diff #513032)

Same here.

libc/test/src/sched/rr_get_interval_test.cpp
1 ↗(On Diff #513032)

Extra n.

1 ↗(On Diff #513032)

Also, why is the file not named sched_rr_get_interval_test.cpp.

goldstein.w.n marked 4 inline comments as done.Apr 13 2023, 11:08 AM
goldstein.w.n added inline comments.
libc/test/src/sched/rr_get_interval_test.cpp
1 ↗(On Diff #513032)

Renamed, was just following the norm of the direction which seems to be to omit sched_ prefix.

goldstein.w.n marked an inline comment as done.

Fix nits

Few more nits but I will let @michaelrj drive this review.

libc/include/llvm-libc-macros/linux/sched-macros.h
13

Instead of Copy ..., say something like, The definitions here match those in linux/sched.h.

libc/include/llvm-libc-macros/sched-macros.h
13 ↗(On Diff #513312)

Ditto.

16 ↗(On Diff #513312)

I missed this last time: These definitions are also based on linux/sched.h so they should stay in linux/sched-macros.h?

goldstein.w.n added inline comments.Apr 13 2023, 1:28 PM
libc/include/llvm-libc-macros/sched-macros.h
16 ↗(On Diff #513312)

I missed this last time: These definitions are also based on linux/sched.h so they should stay in linux/sched-macros.h?

Yeah that's probably best. The reason I put them in generic is they are required for POSIX, not Linux extensions.

goldstein.w.n marked 3 inline comments as done.Apr 13 2023, 2:02 PM

Cleanup sched.h macros

goldstein.w.n edited the summary of this revision. (Show Details)Apr 13 2023, 2:03 PM
goldstein.w.n edited the summary of this revision. (Show Details)
goldstein.w.n edited the summary of this revision. (Show Details)

I think with these last few changes it should be good to go. I want to test it before it lands so I'll approve once that's done.

libc/test/src/sched/get_priority_test.cpp
17

Even if sched isn't in the filename, it should be in the test title since this is what shows up when you run the test. Same for the other tests.

libc/test/src/sched/sched_rr_get_interval_test.cpp
39

for clarity, this should be ::getuid since it's coming from the system's libc.

goldstein.w.n marked 2 inline comments as done.Apr 13 2023, 8:13 PM

clarify test / getuid()

Update td after moving macros

sivachandra added inline comments.Apr 13 2023, 9:38 PM
libc/include/llvm-libc-macros/sched-macros.h
13 ↗(On Diff #513312)

Remove these commented out lines - it will become a precedent for new patches otherwise.

libc/test/src/sched/sched_rr_get_interval_test.cpp
39

LLVM's libc has getuid so why don't we just use __llvm_libc::getuid here?

goldstein.w.n marked an inline comment as done.Apr 13 2023, 11:08 PM

Remove comments, use llvm-libc getuid

michaelrj accepted this revision.Apr 18 2023, 4:17 PM

LGTM, tests pass on my end. I've left a few nits but feel free to land once they're fixed

libc/spec/posix.td
640

this should still have CpuSetT in the Types list since it's used by sched_getcpucount

646

not relevant for this patch, but in a future patch it would be good to add the argument ArgSpect<VoidType> here. In C a function with no arguments is interpreted as having unspecified parameters, whereas a function with an argument of void has no arguments.

libc/test/src/sched/param_and_scheduler_test.cpp
38

nit: These test enough functionality that they probably shouldn't be called smoke tests. I'd recommend just calling this SchedOtherTest and same for the rest below.

This revision is now accepted and ready to land.Apr 18 2023, 4:17 PM
goldstein.w.n marked an inline comment as done.

SmokeTest -> Test

libc/spec/posix.td
640

sched_getcpucount is gnu extension though. We have CpuSetT in gnu.td.

646

+1, is there an issue we add it now (not supported), or just leave it as todo?

As an side, what are these files for? I've had bugs in them but libc compiled fine /tests ran.

michaelrj added inline comments.Apr 20 2023, 11:38 AM
libc/spec/posix.td
640

ah, you're right. Nevermind then.

646

You can fix it now or leave a TODO and fix it in a followup patch, either works.

These files are used to generate the header files in fullbuild mode. We have a TableGen based utility called HeaderGen that takes in the api.td file for the platform you're building for. Then the api.td file includes these files in the spec folder. These files tell HeaderGen what the function prototypes for each function are, and which header file they go in. Then the entrypoints.txt and headers.txt files determine which functions and headers should be built for this specific platform. If you want to see the header files then build libc-headers and look in build/projects/libc/include, though make sure you've set LLVM_LIBC_FULL_BUILD to ON in your cmake. If you want to test that these work, then run the libc-api-test which uses the PrototypeTestGen (in libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp) to generate the prototypes for each function, then test that they match the implementation.

Make yield void

goldstein.w.n marked 4 inline comments as done.Apr 20 2023, 12:42 PM
goldstein.w.n added inline comments.
libc/spec/posix.td
646

not relevant for this patch, but in a future patch it would be good to add the argument ArgSpect<VoidType> here. In C a function with no arguments is interpreted as having unspecified parameters, whereas a function with an argument of void has no arguments.

Done for yield. Will do in the future.

646

You can fix it now or leave a TODO and fix it in a followup patch, either works.

These files are used to generate the header files in fullbuild mode. We have a TableGen based utility called HeaderGen that takes in the api.td file for the platform you're building for. Then the api.td file includes these files in the spec folder. These files tell HeaderGen what the function prototypes for each function are, and which header file they go in. Then the entrypoints.txt and headers.txt files determine which functions and headers should be built for this specific platform. If you want to see the header files then build libc-headers and look in build/projects/libc/include, though make sure you've set LLVM_LIBC_FULL_BUILD to ON in your cmake. If you want to test that these work, then run the libc-api-test which uses the PrototypeTestGen (in libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp) to generate the prototypes for each function, then test that they match the implementation.

Ahh makes sense. Thanks :)

This revision was automatically updated to reflect the committed changes.
goldstein.w.n marked an inline comment as done.