newlib: fix definition of time_t and off_t - #5132
Conversation
|
CI seems to be failing for reasons unrelated to the changes introduced in the patch. A rerun should |
time_t in Newlibtime_t and off_t in Newlib
7152f77 to
e821035
Compare
This comment has been minimized.
This comment has been minimized.
|
CI actually passes. There seems to be an issue with a glob import that is not used, but this has not |
de697e2 to
0e027c7
Compare
This comment has been minimized.
This comment has been minimized.
0e027c7 to
6abed2e
Compare
This comment has been minimized.
This comment has been minimized.
|
I couldn't remember what the possible env+os combinations were for these platforms, here's a quick list. |
There was a problem hiding this comment.
I'm having some difficulty extracting a simplified goal from the top post. Is this the net delta on targets we currently support?
- On armv7-rtems,
time_tchanges from i32 to i64 - On armv7-rtems,
off_tchanges from i64 toc_long, which should be i64->i32 - On armv7-vita,
off_tchanges fromc_inttoc_long. I think there is no change here?
If this sounds accurate, ping the target maintainer (see https://doc.rust-lang.org/rustc/platform-support/armv7-rtems-eabihf.html) for confirmation. This is generally a good idea when tricky things like this change on less popular targets, there may be config or other nuance that doesn't show up in the source code.
Also please don't call this a refactor, that term usually means cleaning up code without changing behavior - if the user experience changes somehow then it's kind of misleading. (Applies to some of your other PRs as well).
Note this option is "documented" as being meant for end users of the libc crate to enable, but no information on it is provided anywhere in all of the libc crate.
It's mostly intended to be set by espidf build tooling, but it would be good to at least mention it. A PR would be welcome in the crate-level docs, after #5179 adds some structure.
It would also be great to know whether the libc crate considers horizon to be the OS for both the Nintendo Switch and the Nintendo 3DS targets, or only for the Nintendo 3DS target.
aarch64-nintendo-switch-freestanding does not appear to set a target_env, so I would assume the latter.
|
@rustbot author for clarification |
|
Reminder, once the PR becomes ready for a review, use |
time_t and off_t in Newlibtime_t and off_t
|
The changes here are required to fit the definitions upstream of The changes for file offset types are concerned with either
I'll open a PR soon with the documentation for ESP-IDF. @rustbot ready |
6abed2e to
c2b0970
Compare
This comment has been minimized.
This comment has been minimized.
|
Hey @thesummer! I was wondering if I could get confirmation on these changes on the RTEMS target you maintain? |
|
Hey @nikarh @pheki @zetanumbers! I was wondering if I could get confirmation on these changes on the Sony PS Vita target you maintain? |
There was a problem hiding this comment.
Thanks for asking!
Just need a change as the Vita uses 32-bit time_t.
Keeping it on the top-level comment for easy reference, here are the sources I could find (somewhat redundant with the finds in the root PR comment but in vitasdk's fork):
off_t
time_t
--enable-newlib-long-time_tis set in CMakeLists.txt- In configure:
- In configure.in:
_TIME_T_is settime_tis set
| if #[cfg(any( | ||
| target_os = "espidf", | ||
| target_os = "vita", | ||
| target_os = "rtems", | ||
| target_arch = "aarch64" | ||
| ))] { |
There was a problem hiding this comment.
For vita, this one seems correct, as by looking at the definitions it seems to be defined as a long. There should also be no change in behavior as both c_int and c_long are aliases to i32 in this platform.
Sources in the top level review comment.
| } else { | ||
| pub type time_t = i64; |
There was a problem hiding this comment.
This one looks wrong for the vita, which uses 32-bit time, as --enable-newlib-long-time_t is set when building newlib in vitasdk (currently the only supported SDK).
The correct definition would be c_long (effectively i32).
Sources in the top-level comment.
There was a problem hiding this comment.
Thanks. That's fixed now.
This comment has been minimized.
This comment has been minimized.
Done. @rustbot ready |
88f1801 to
84c4a56
Compare
This comment has been minimized.
This comment has been minimized.
84c4a56 to
8b8665b
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
There's a typo in the message for 0bb0085 ("newlib: change definition of file offset types"), ino_t was never i64. Not going to block though
8b8665b to
a9073cb
Compare
Replace `time_t` definition in `newlib` for certain targets. This option is configurable in upstream newlib [^1] and some targets seem to have it enabled in their build scripts. Effective changes include the following. OS | old | new | effective change -----------------------------------|-------------|--------|--------------------------- risc32/xtensa32-espidf time32 | `i32` | `long` | alias only, no size change risc32/xtensa32-espidf not(time32) | `long long` | `i64` | alias only, no size change arm32-horizon | `long long` | `i64` | alias only, no size change arm32-rtems | `i32` | `i64` | ABI change, 32->64 bits arm32-vita | `i32` | `long` | alias only, no size change [^1]: https://github.com/cygwin/cygwin/blob/41e6325ad64fa00f5e61e25c2020a8bc716c29a0/newlib/README#L351
Use mirrored definition of file offset types in `vita` target. Effective changes include the following. OS | ty | old | new | effective change -----------------------|---------|------------------|------------------|--------------------------- risc32/xtensa32-espidf | `dev_t` | `short` | `short` | none arm32-horizon | `dev_t` | `u32` | `u32` | none arm32-rtems | `dev_t` | `u32` | `u32` | none arm32-vita | `dev_t` | `short` | `short` | none -----------------------|---------|------------------|------------------|--------------------------- risc32/xtensa32-espidf | `ino_t` | `unsigned short` | `unsigned short` | none arm32-horizon | `ino_t` | `u32` | `u32` | none arm32-rtems | `ino_t` | `u32` | `u32` | none arm32-vita | `ino_t` | `unsigned short` | `unsigned short` | none -----------------------|---------|------------------|------------------|--------------------------- risc32/xtensa32-espidf | `off_t` | `long` | `long` | none arm32-horizon | `off_t` | `i64` | `i64` | none arm32-rtems | `off_t` | `i64` | `i64` | none arm32-vita | `off_t` | `int` | `long` | alias only, no size change
Remove AArch64 bindigns in newlib. Rust has no officially supported target using newlib in AArch64.
Force a compile error on targets with no file offset type definitions. This ensures any new target needs providing defintions for these types.
Move the `generic` module into the top-level `newlib` module. They were already reexported by all supported targets in their respective modules. These types were also conditionally defined anyway.
Remove PowerPC definitions. There's no Rust target using newlib on PowerPC.
a9073cb to
5cd00d8
Compare
|
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
This PR addresses bit width mismatches for both
time_tand file offset types. This applies to thelinux/newlibmodule.Upstream and forks for each supported target provide conditional definitions.
time_tcan be configured in the newlib build script. File offset types require manual intervention in a specific header file. The file containing the default definition will check for a macro that is defined in that specific "override" file. The default definition is a Clong.A number of supported targets were using either one of the above overrides. This was not being taken into account in the
libccrate. This patch fixes that by further expanding conditionally compiled type definitions.Sources
Cygwin's, espressif's and the Vita SDK's newlib forks showing conditional definition of the
time_ttype and theoff_ttype. Note Vita SDK's fork contains definitions relative to multiple target systems, but the one underinclude/syshas taken priority. The definition underinclude/vita/sys#includes the former to define itsoff_ttype.Sources on devkitARM could not be found online, but they can be found under the installed toolchain's directory in
devkitARM/by runningripgrepwith the following command.Newlib sources documenting the
configurebuild script non-default option for using ac_longfor theirtime_tdefinition.Cygwin upstream codesearch showing the definition of
_off_t, which defines__off_t, and eventuallyoff_t. Note how this is defined inmachine/_types.h(as ani64,) and later that is checked insys/_types.h(falling back to ac_long.)espressif's
esp-32fork of newlib,vita's and RTEMS' showing how they don't have a definition in terms ani64undermachine/_types.h.Checklist
libc-test/semverhave been updated*LASTor*MAXare included (see #3131)cd libc-test && cargo test --target mytarget); especially relevant for platforms that may not be checked in CI