uclibc: fix build + CI, add unstable time64 support for newer uclibc toolchains #5046
uclibc: fix build + CI, add unstable time64 support for newer uclibc toolchains #5046skrap wants to merge 2 commits intorust-lang:mainfrom
Conversation
|
|
||
| time64="$1" | ||
|
|
||
| if [ "${time64:-0}" != "0" ]; then |
There was a problem hiding this comment.
I added this script to use a toolchain with or without 64-bit time_t depending on $1.
| "sys/fanotify.h", | ||
| // <sys/auxv.h> is not present on uclibc | ||
| (!uclibc, "sys/auxv.h"), | ||
| "sys/auxv.h", |
There was a problem hiding this comment.
uclibc has this header, at least in the toolchain versions in the CI.
| // Not defined in uclibc as of 1.0.34 | ||
| // Not defined in uclibc as of 1.0.45 | ||
| "gettid" if uclibc => true, | ||
| "getauxval" if uclibc => true, |
There was a problem hiding this comment.
libc doesn't define getauxval.
| pub nr: crate::__s32, | ||
| } | ||
|
|
||
| #[cfg_attr( |
There was a problem hiding this comment.
Several definitions here were moved around in the linux/l4re shared restructuring. These are now defined elsewhere; remove them here.
| pub const EM_MN10300: u16 = 89; | ||
| pub const EM_MN10200: u16 = 90; | ||
| pub const EM_PJ: u16 = 91; | ||
| #[cfg(not(target_env = "uclibc"))] |
There was a problem hiding this comment.
Many of these guarded constants are actually defined in uclibc. I removed guards where appropriate.
| pub const IP_ORIGDSTADDR: c_int = 20; | ||
| pub const IP_RECVORIGDSTADDR: c_int = IP_ORIGDSTADDR; | ||
| pub const IP_MINTTL: c_int = 21; | ||
| #[cfg(not(target_env = "uclibc"))] |
There was a problem hiding this comment.
Similar removal of unnecessary guards.
|
CI seems to error on a rustc issue when building; I think this is unrelated. Awaiting a fix there, but feel free to review in the meantime! |
| cfg_if! { | ||
| if #[cfg(not(target_env = "uclibc"))] { | ||
| pub const MFD_NOEXEC_SEAL: c_uint = 0x0008; | ||
| pub const MFD_EXEC: c_uint = 0x0010; | ||
| pub const MFD_HUGE_64KB: c_uint = 0x40000000; | ||
| pub const MFD_HUGE_512KB: c_uint = 0x4c000000; | ||
| pub const MFD_HUGE_1MB: c_uint = 0x50000000; | ||
| pub const MFD_HUGE_2MB: c_uint = 0x54000000; | ||
| pub const MFD_HUGE_8MB: c_uint = 0x5c000000; | ||
| pub const MFD_HUGE_16MB: c_uint = 0x60000000; | ||
| pub const MFD_HUGE_32MB: c_uint = 0x64000000; | ||
| pub const MFD_HUGE_256MB: c_uint = 0x70000000; | ||
| pub const MFD_HUGE_512MB: c_uint = 0x74000000; | ||
| pub const MFD_HUGE_1GB: c_uint = 0x78000000; | ||
| pub const MFD_HUGE_2GB: c_uint = 0x7c000000; | ||
| pub const MFD_HUGE_16GB: c_uint = 0x88000000; | ||
| pub const MFD_HUGE_MASK: c_uint = 63; | ||
| pub const MFD_HUGE_SHIFT: c_uint = 26; | ||
| } | ||
| } | ||
| pub const MFD_NOEXEC_SEAL: c_uint = 0x0008; | ||
| pub const MFD_EXEC: c_uint = 0x0010; | ||
| pub const MFD_HUGE_64KB: c_uint = 0x40000000; | ||
| pub const MFD_HUGE_512KB: c_uint = 0x4c000000; | ||
| pub const MFD_HUGE_1MB: c_uint = 0x50000000; | ||
| pub const MFD_HUGE_2MB: c_uint = 0x54000000; | ||
| pub const MFD_HUGE_8MB: c_uint = 0x5c000000; | ||
| pub const MFD_HUGE_16MB: c_uint = 0x60000000; | ||
| pub const MFD_HUGE_32MB: c_uint = 0x64000000; | ||
| pub const MFD_HUGE_256MB: c_uint = 0x70000000; | ||
| pub const MFD_HUGE_512MB: c_uint = 0x74000000; | ||
| pub const MFD_HUGE_1GB: c_uint = 0x78000000; | ||
| pub const MFD_HUGE_2GB: c_uint = 0x7c000000; | ||
| pub const MFD_HUGE_16GB: c_uint = 0x88000000; | ||
| pub const MFD_HUGE_MASK: c_uint = 63; | ||
| pub const MFD_HUGE_SHIFT: c_uint = 26; |
There was a problem hiding this comment.
@farao do the constants in this file that are changed here not exist on l4re? If so, they could just move to linux rather than linux_l4re_shared.
There was a problem hiding this comment.
yes that's true they do not exist on l4re so they should rather be moved to linux. I think I only put them in here to have all the MFD_* consts in one file.
There was a problem hiding this comment.
What about the constants like EM_OPENRISC or PTHREAD_MUTEX_STALLED above?
@skrap for the constants here, could you update them to not(target_os = "l4re") rather than removing the gate? I think that's easier than moving them all.
There was a problem hiding this comment.
Maybe also with a comment that this is done to keep similar constants in the same file
There was a problem hiding this comment.
Re EM_OPENRISC:
Some of these are a bit unexpected to have live in libc, to be honest. The EM_* constants are defined not by libc or by the kernel, but by the ELF format spec. They're fixed no matter what OS or std library we're running. So for this constant, I would prefer that libc present a unified interface to higher-level crates, rather than adopt the uclibc quirk of having EM_OR1K be a synonym for EM_OPENRISC.
Re PTHREAD_MUTEX_STALLED:
I'm not sure I understand the question here. This constant isn't conditional based on the target_env.
There was a problem hiding this comment.
could you update them to not(target_os = "l4re")
I think your request is reasonable, but I would like to ask that the requested l4re change go into a separate PR, as removing those constants for the l4re build could easily introduce breakage, and I don't know anything about that platform.
There was a problem hiding this comment.
To answer the question a few comments above: yes, all constants that were disabled for uclibc are not present on l4re. I would of course prefer if they stayed disabled for l4re in one way or the other
| // constants not available in uclibc 1.0.45 | ||
| // but defined outside the uclibc library, | ||
| // e.g. file format constants or kernel-defined | ||
| // constants. |
There was a problem hiding this comment.
Nit: can be rewrapped to 88 or 100 chars to match the rest (kind of wish rustfmt just did this)
| "ALG_SET_KEY_BY_KEY_SERIAL" | ||
| | "AT_MINSIGSTKSZ" | ||
| | "BUS_MCEERR_AO" | ||
| | "BUS_MCEERR_AR" | ||
| | "CAN_BUS_OFF_THRESHOLD" | ||
| | "CAN_CTRLMODE_TDC_AUTO" | ||
| | "CAN_CTRLMODE_TDC_MANUAL" | ||
| | "CAN_ERR_CNT" |
There was a problem hiding this comment.
It seems like many of these should be fine if included via linux/... .h paths; is there a reason that isn't recommended or doesn't work on uclibc?
If so, it would be good to mark them deprecated on the target and remove in a release or two.
There was a problem hiding this comment.
I can do that.
I was unsure of the philosophy here with respect to kernel-defined constants. They would either be present in linux/*.h headers or not depending on the kernel version, regardless of whatever libc library/version was being used on the system. So what triggers a particular constant being promoted into the libc crate?
There was a problem hiding this comment.
I included the constants via linux UAPI headers as much as possible, but in some cases doing so conflicted with the definitions from uclibc. In this case, I left a comment.
In order to increase readability, I moved the uclibc-specific handling of exceptions to its own code branch and out of the catch-all branch, mirroring how musl does things.
This comment has been minimized.
This comment has been minimized.
|
I addressed the review feedback. Thanks for having a look! I had to fence out |
|
@tgross35 I am unsure of the desired workflow here - I see a "request re-review" button in the github UI. Is this on your plate already, or do I push that button now that I have addressed your feedback? (Not trying to rush, just trying to learn the intended workflow.) |
There was a problem hiding this comment.
Think the todo list here should be about the last one, also some questions.
@tgross35 I am unsure of the desired workflow here - I see a "request re-review" button in the github UI. Is this on your plate already, or do I push that button now that I have addressed your feedback? (Not trying to rush, just trying to learn the intended workflow.)
You can request a review. Usually I just go based on labels but requesting a review should also flip to S-waiting-on-review IIRC (as does @rustbot review).
The cycle is a bit slow for libc for nontrivial PRs, typically I go through everything every ~2 weeks.
| curl --retry 5 -L "https://toolchains.bootlin.com/downloads/releases/toolchains/armv7-eabihf/tarballs/armv7-eabihf--uclibc--${version}.tar.xz" | \ | ||
| tar xjf - -C /toolchain --strip-components=1 |
There was a problem hiding this comment.
No need to change this of course but fyi, | at the end of the line allows wrapping. So the \ isn't needed
| cfg_if! { | ||
| if #[cfg(not(target_env = "uclibc"))] { | ||
| pub const MFD_NOEXEC_SEAL: c_uint = 0x0008; | ||
| pub const MFD_EXEC: c_uint = 0x0010; | ||
| pub const MFD_HUGE_64KB: c_uint = 0x40000000; | ||
| pub const MFD_HUGE_512KB: c_uint = 0x4c000000; | ||
| pub const MFD_HUGE_1MB: c_uint = 0x50000000; | ||
| pub const MFD_HUGE_2MB: c_uint = 0x54000000; | ||
| pub const MFD_HUGE_8MB: c_uint = 0x5c000000; | ||
| pub const MFD_HUGE_16MB: c_uint = 0x60000000; | ||
| pub const MFD_HUGE_32MB: c_uint = 0x64000000; | ||
| pub const MFD_HUGE_256MB: c_uint = 0x70000000; | ||
| pub const MFD_HUGE_512MB: c_uint = 0x74000000; | ||
| pub const MFD_HUGE_1GB: c_uint = 0x78000000; | ||
| pub const MFD_HUGE_2GB: c_uint = 0x7c000000; | ||
| pub const MFD_HUGE_16GB: c_uint = 0x88000000; | ||
| pub const MFD_HUGE_MASK: c_uint = 63; | ||
| pub const MFD_HUGE_SHIFT: c_uint = 26; | ||
| } | ||
| } | ||
| pub const MFD_NOEXEC_SEAL: c_uint = 0x0008; | ||
| pub const MFD_EXEC: c_uint = 0x0010; | ||
| pub const MFD_HUGE_64KB: c_uint = 0x40000000; | ||
| pub const MFD_HUGE_512KB: c_uint = 0x4c000000; | ||
| pub const MFD_HUGE_1MB: c_uint = 0x50000000; | ||
| pub const MFD_HUGE_2MB: c_uint = 0x54000000; | ||
| pub const MFD_HUGE_8MB: c_uint = 0x5c000000; | ||
| pub const MFD_HUGE_16MB: c_uint = 0x60000000; | ||
| pub const MFD_HUGE_32MB: c_uint = 0x64000000; | ||
| pub const MFD_HUGE_256MB: c_uint = 0x70000000; | ||
| pub const MFD_HUGE_512MB: c_uint = 0x74000000; | ||
| pub const MFD_HUGE_1GB: c_uint = 0x78000000; | ||
| pub const MFD_HUGE_2GB: c_uint = 0x7c000000; | ||
| pub const MFD_HUGE_16GB: c_uint = 0x88000000; | ||
| pub const MFD_HUGE_MASK: c_uint = 63; | ||
| pub const MFD_HUGE_SHIFT: c_uint = 26; |
There was a problem hiding this comment.
What about the constants like EM_OPENRISC or PTHREAD_MUTEX_STALLED above?
@skrap for the constants here, could you update them to not(target_os = "l4re") rather than removing the gate? I think that's easier than moving them all.
| cfg_if! { | ||
| if #[cfg(not(target_env = "uclibc"))] { | ||
| pub const MFD_NOEXEC_SEAL: c_uint = 0x0008; | ||
| pub const MFD_EXEC: c_uint = 0x0010; | ||
| pub const MFD_HUGE_64KB: c_uint = 0x40000000; | ||
| pub const MFD_HUGE_512KB: c_uint = 0x4c000000; | ||
| pub const MFD_HUGE_1MB: c_uint = 0x50000000; | ||
| pub const MFD_HUGE_2MB: c_uint = 0x54000000; | ||
| pub const MFD_HUGE_8MB: c_uint = 0x5c000000; | ||
| pub const MFD_HUGE_16MB: c_uint = 0x60000000; | ||
| pub const MFD_HUGE_32MB: c_uint = 0x64000000; | ||
| pub const MFD_HUGE_256MB: c_uint = 0x70000000; | ||
| pub const MFD_HUGE_512MB: c_uint = 0x74000000; | ||
| pub const MFD_HUGE_1GB: c_uint = 0x78000000; | ||
| pub const MFD_HUGE_2GB: c_uint = 0x7c000000; | ||
| pub const MFD_HUGE_16GB: c_uint = 0x88000000; | ||
| pub const MFD_HUGE_MASK: c_uint = 63; | ||
| pub const MFD_HUGE_SHIFT: c_uint = 26; | ||
| } | ||
| } | ||
| pub const MFD_NOEXEC_SEAL: c_uint = 0x0008; | ||
| pub const MFD_EXEC: c_uint = 0x0010; | ||
| pub const MFD_HUGE_64KB: c_uint = 0x40000000; | ||
| pub const MFD_HUGE_512KB: c_uint = 0x4c000000; | ||
| pub const MFD_HUGE_1MB: c_uint = 0x50000000; | ||
| pub const MFD_HUGE_2MB: c_uint = 0x54000000; | ||
| pub const MFD_HUGE_8MB: c_uint = 0x5c000000; | ||
| pub const MFD_HUGE_16MB: c_uint = 0x60000000; | ||
| pub const MFD_HUGE_32MB: c_uint = 0x64000000; | ||
| pub const MFD_HUGE_256MB: c_uint = 0x70000000; | ||
| pub const MFD_HUGE_512MB: c_uint = 0x74000000; | ||
| pub const MFD_HUGE_1GB: c_uint = 0x78000000; | ||
| pub const MFD_HUGE_2GB: c_uint = 0x7c000000; | ||
| pub const MFD_HUGE_16GB: c_uint = 0x88000000; | ||
| pub const MFD_HUGE_MASK: c_uint = 63; | ||
| pub const MFD_HUGE_SHIFT: c_uint = 26; |
There was a problem hiding this comment.
Maybe also with a comment that this is done to keep similar constants in the same file
| /* | ||
| Here are a list of kernel UAPI constants which appear in linux/ headers, | ||
| but cannot be imported due to conflicts with the uclibc headers. | ||
| The conflicting linux/ header is noted. | ||
| */ |
There was a problem hiding this comment.
What is the expected flow for these when you're writing C if there are conflicts?
There was a problem hiding this comment.
My view on it is that these constants are passed through the underlying target_env and to the kernel, rather than invoking specific behavior within the target_env c std lib. For constants where this is true, it means that they're safe and appropriate to use regardless of whether the c std lib re-exports them or not. The case where they could lead to unintentional bad behavior is when the c std lib needed to change behavior based on the constant. I am not individually researching and testing each of these, so it's possible that there's a bug here, but I would like to consider those issues as outside the scope of this PR. (This is just moving around these constants and fixing the build.)
To answer your question, if I were writing a piece of C code and needed one of these constants, I would probably find a way to untangle the header mess or just copy the #define into my implementation. Ugly, yes, but this is the state of system headers in C in my experience.
There are also of other examples in libc-test/build.rs of this type of issue - e.g. see test_linux_like_apis() which appears to try to work around this issue for combinations of specific constants and headers which don't work.
| else | ||
| version='bleeding-edge-2024.05-1' # last version with 32-bit time_t | ||
| fi |
There was a problem hiding this comment.
Does this more or less mean that you're stuck on an old uclibc version if you want to use time APIs? There aren't the duplicate symbols and header config like glibc has?
If so, I think we could flip the default at some point in the near future. Technically breaking but we can get away with it on T3 targets, and musl and glibc aren't quite as stuck.
There was a problem hiding this comment.
Ah, no - let me explain. uClibc is intended for use on embedded devices where space (and thus code size) is at a premium. A developer would typically build their own uclibc with only the features required for their use case. uclibc facilitates this customization by having a configuration step (via a menuconfig, like the kernel menuconfig system, if you've ever used that) where the developer chooses the features they like, including whether to use a 32-bit or 64-bit time_t, and then builds their own uclibc for their project.
So we cannot say for sure within the libc crate which size of time_t will be present in the underlying uclibc library.
The bootlin folks have chosen to enable the 64-bit time_t feature on their build of uclibc as of the toolchain noted above. Having a reference toolchain is really useful for ease of CI and developer bootstrapping purposes, but the uclibc configuration choices that the bootlin toolchain makes aren't necessarily the same (nor binary compatible) with any other developer's use of uclibc.
This is why I expect everyone using uclibc with rust ends up either using -Zbuild-std or building their own cross toolchain, in either case targeting their project-specific uclibc headers, and (as of this change) making use of the --cfg=libc_unstable_uclibc_time64 rustflag if needed.
|
Reminder, once the PR becomes ready for a review, use |
| cfg_if! { | ||
| // Set cfg(libc_unstable_uclibc_time64) in rustflags if your uclibc has 64-bit time | ||
| if #[cfg(linux_time_bits64)] { | ||
| pub type time_t = c_longlong; | ||
| pub type suseconds_t = c_longlong; | ||
| } else { | ||
| pub type time_t = c_long; | ||
| pub type suseconds_t = c_long; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do any structs need to be updated here? I know on other targets the padding in the timespec struct sometimes needs to be adjusted. I assume there are no function link names to update if uclibc had a pretty clean cut 32->64 transition.
There was a problem hiding this comment.
I am relying on the libc-test infrastructure a bit here to catch errors, but I think everything should be OK. I am happy to be the point person for any issues raised for 64-bit time_t in uclibc, if bugs are reported.
There was a problem hiding this comment.
@rustbot ready
Thanks for the review! I have replied inline to the issues raised.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Description
Goals of this change:
time_tconfiguration, which is enabled on the latest bootlin toolchains.Testing instructions
For testing, I have done the following:
libc-testpasses against the last bootlin toolchain to use 32-bittime_t(bleeding-edge-2024.02-1)libc-testpasses against the most recent bootlin toolchain (bleeding-edge-2025.08-1)RUSTFLAGS="--cfg=libc_unstable_uclibc_time64"as expectedSources
I am not changing APIs so I don't think this applies.
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI
@rustbot label +stable-nominated