newlib: fix definition of time_t and off_t

[dybucc]

Description

This PR adresses bit-width representation issues under the newlib target environment concerned with both the time_t and off_t types.

In newlib, time_t is conditionally defined as being a c_longlong under the horizon and espidf target operating systems. Otherwise, it always defaults to using a 32-bit signed integer. off_t is always defined as a 64-bit signed integer except in espidf and in vita, where they are defined as c_long and c_int, respectively.

This is possibly incorrect. The newlib upstream repo maintained by Cygwin defines time_t conditionally as being either one of a c_long or a signed integer type with at least 64 bits precision. The definition will always default to the latter unless one issues --enable-newlib-long-time_t to the configure build script, or if the host system is known to have c_longs larger than 32 bits. In much the same vein, the espressif upstream repo for their fork of newlib uses the exact same definition, and so does the devkitARM shipped for development on horizon. Support for espidf with 32-bit time_t is likely provided for compatibility purposes following the analogous compatibility shims espressif has set in place in their upstream repos [1] [2].

Upstream also defines off_t in an ever so slightly different way to the declaration in the libc crate. The definition in the "official" repo maintained by the Cygwin developers will always provide a 64-bit value. The forks for vita, espressif's esp-32, devkitA64 (and by extension horizon in the Nintendo Switch but not in the Nintendo 3DS,) and RTEMS, don't define this type in the machine/_types.h header file. This header file is the one where the first definition for the type appears, and is the one that takes priority if it exists. A macro checks that in sys/_types.h, and will otherwise define the type in terms of a c_long. This only happens in the afore mentioned platforms, where the definition of off_t is removed from machine/_types.h. Currently, the exposed type is not completely wrong, but is quite defintely wrong in vita.

This patch changes the time_t definition to fit a 64-bit signed integer unless compiling for the afore mentioned environment (the one for time_t) with the existing cfg(espidf_time32) enabled. 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 seems like the only users of that cfg are the projects in the esp-rs org. Some of the other targets using newlib have been verified to also follow this definition for time_t (sources below.) Among those checked are the ones for which we have custom modules at the end of the newlib/mod.rs module. Notably, no architecture-specific definitions were found for aarch64.

A simple test has been ran in all platforms except vita and aarch64 (which here means the Nintendo Switch with devkitPro's devkitA64) for both time_t and off_t. The former used a two-element array of time_t values and ensured that a write through an FFI routine (like time) only overwrote one of the values and not both. The latter required a bit more intricate testing, as no C standard library routine takes a mutable pointer to an off_t. All of the affected platforms required emulation and to some extent, supported embedding a virtual file system to the compiled binary. This was used to embed files with different sizes and use stream seeking operations with values for off_t both larger and smaller than 32 bits.

Testing was not possible in vita because the emulator did not support the type of homebrew package that the Rust toolchain for vita builds. In horizon for the Nintendo Switch under aarch64 with devkitA64, testing was neither possible for the same reason as in vita.

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. It just so happens that the newlib module has submodules for all three of them with a note that mentions testing only with devkitARM. Using cfg's for different architecures may not be the right choice, as there could be other systems whose ISA matched aarch64 with changes to their newlib fork that differ from those in devkitA64 (or any other devkitPro SDK, for that matter.)

Sources

• Cygwin's, espressif's and the Vita SDK's newlib forks showing conditional definition of the time_t type and the off_t type. Note Vita SDK's fork contains definitions relative to multiple target systems, but the one under include/sys has taken priority. The definition under include/vita/sys #includes the former to define its off_t type.

  https://github.com/cygwin/cygwin/blob/6926523688bab730ae1b56f24ad6fb203b7350af/newlib/libc/include/sys/_types.h#L185
  https://github.com/espressif/newlib-esp32/blob/esp-4.5.0/newlib/libc/include/sys/_types.h#L185
  https://github.com/search?q=repo%3Avitasdk%2Fnewlib+%2F%5Cb_time_t_%5Cb%2F&type=code
  https://github.com/cygwin/cygwin/blob/6926523688bab730ae1b56f24ad6fb203b7350af/newlib/libc/include/sys/_types.h#L100
  https://github.com/espressif/newlib-esp32/blob/67537d91e601c40e2b52b9b865518abb7eecc85b/newlib/libc/include/sys/_types.h#L100
  https://github.com/vitasdk/newlib/blob/fbb8375870d799758e0b4358d2f1708781aa26a6/newlib/libc/include/sys/_types.h#L100

  Sources on devkitARM could not be found online, but they can be found under the installed toolchain's directory in devkitARM/ by running ripgrep with the following command.

  rg -C 5 -S -uu -g "**/*.h" -e "\b_time_t_\b"

• Newlib sources documenting the configure build script non-default option for using a c_long for their time_t definition.

  https://github.com/cygwin/cygwin/blob/6926523688bab730ae1b56f24ad6fb203b7350af/newlib/README#L351

• Cygwin upstream codesearch showing the definition of _off_t, which defines __off_t, and eventually off_t. Note how this is defined in machine/_types.h (as an i64,) and later that is checked in sys/_types.h (falling back to a c_long.)

  https://github.com/search?q=repo%3Acygwin%2Fcygwin%20symbol%3A%2F%5Cb_*off_t%5Cb%2F&type=code

• espressif's esp-32 fork of newlib, vita's and RTEMS' showing how they don't have a definition in terms an i64 under machine/_types.h.

  https://github.com/search?q=repo%3Aespressif%2Fnewlib-esp32+path%3Anewlib%2Flibc%2Finclude+%2F%5Cb_*off_t%5Cb%2F&type=code
  https://github.com/search?q=repo%3Avitasdk%2Fnewlib+path%3Anewlib%2Flibc%2Finclude+%2F%5Cb_*off_t%5Cb%2F&type=code
  https://gitlab.rtems.org/search?search=path%3Anewlib%2Flibc%2Finclude+extension%3Ah+%22_*off_t%22&nav_source=navbar&project_id=416&group_id=1086&search_code=true&repository_ref=main

Checklist

• Relevant tests in libc-test/semver have been updated
• No placeholder or unstable values like *LAST or *MAX are included (see #3131)
• Tested locally (cd libc-test && cargo test --target mytarget); especially relevant for platforms that may not be checked in CI

[dybucc]

CI seems to be failing for reasons unrelated to the changes introduced in the patch. A rerun should
do it.

[dybucc]

CI actually passes. There seems to be an issue with a glob import that is not used, but this has not
been changed in the patch (it's not even part of it, for that matter.) For some reason, rebasing
onto main with dependabot updates has ended up with a warning across all of my open PRs due to
that one (now apparently unused) import.

[tgross35]

I couldn't remember what the possible env+os combinations were for these platforms, here's a quick list.

> $cfg | find 'target_env="newlib"' | get tgt | each {|tgt| $cfg | find $tgt -n | find -nr '(target_env|target_os)' } | flatten
  #               tgt                        cfg
─────────────────────────────────────────────────────────
  0   armv6k-nintendo-3ds            target_env="newlib"
  1   armv6k-nintendo-3ds            target_os="horizon"
  2   armv7-rtems-eabihf             target_env="newlib"
  3   armv7-rtems-eabihf             target_os="rtems"
  4   armv7-sony-vita-newlibeabihf   target_env="newlib"
  5   armv7-sony-vita-newlibeabihf   target_os="vita"
  6   riscv32imac-esp-espidf         target_env="newlib"
  7   riscv32imac-esp-espidf         target_os="espidf"
  8   riscv32imafc-esp-espidf        target_env="newlib"
  9   riscv32imafc-esp-espidf        target_os="espidf"
 10   riscv32imc-esp-espidf          target_env="newlib"
 11   riscv32imc-esp-espidf          target_os="espidf"
 12   xtensa-esp32-espidf            target_env="newlib"
 13   xtensa-esp32-espidf            target_os="espidf"
 14   xtensa-esp32s2-espidf          target_env="newlib"
 15   xtensa-esp32s2-espidf          target_os="espidf"
 16   xtensa-esp32s3-espidf          target_env="newlib"
 17   xtensa-esp32s3-espidf          target_os="espidf"

[tgross35]

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_t changes from i32 to i64
• On armv7-rtems, off_t changes from i64 to c_long, which should be i64->i32
• On armv7-vita, off_t changes from c_int to c_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.

View changes since this review

[tgross35]

@rustbot author for clarification

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[dybucc]

The changes here are required to fit the definitions upstream of time_t, which was generally mismatched.

The changes for file offset types are concerned with either

1. having a mirrored definition, rather an an equivalent one, and
2. with the same bit width mismatches as with time_t.

I'll open a PR soon with the documentation for ESP-IDF.

@rustbot ready

[dybucc]

Hey @thesummer!

I was wondering if I could get confirmation on these changes on the RTEMS target you maintain?

[dybucc]

Hey @nikarh @pheki @zetanumbers!

I was wondering if I could get confirmation on these changes on the Sony PS Vita target you maintain?

[pheki]

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

• _off_t is defined
• __off_t is defined
• off_t is defined

time_t

• --enable-newlib-long-time_t is set in CMakeLists.txt
• In configure:
  • newlib_long_time_t is set
  • _WANT_USE_LONG_TIME_T is set
• In configure.in:
  • newlib_long_time_t is set
  • _WANT_USE_LONG_TIME_T is set
• _TIME_T_ is set
• time_t is set

View changes since this review

[pheki]

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.

[pheki]

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.

[tgross35]

The changes here are required to fit the definitions upstream of time_t, which was generally mismatched.

The changes for file offset types are concerned with either

1. having a mirrored definition, rather an an equivalent one, and
2. with the same bit width mismatches as with `time_t`.

Indeed libc doesn't match every source, but with every change I need to figure out a succinct description of what (1) is changing to look more similar to source without affecting users, vs. (2) is a behavior change of some form. IOW I'm interested in the intent more than the problem list, to make sure I'm on the same page as PR authors.

To help with that, are the summary bullet points at #5132 (review) accurate?

[rustbot]

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.