refactor: fix definition of time_t and off_t in Newlib#5132
Draft
dybucc wants to merge 2 commits into
Draft
Conversation
Contributor
Author
|
CI seems to be failing for reasons unrelated to the chagnes introduced in the patch. A rerun should |
time_t in Newlibtime_t and off_t in Newlib
At present, the newlib module uses a faulty definition of the `time_t` type that falls back to a 32-bti signed integer if the target does not match either one of the `horizon` operating system or the espressif ESP-IDF with the 32-bit compatibility shim for `time_t` values. In all current upstream repos forking newlib, the definition follows a default where `time_t` is defined as a 64-bit type. It only falls back to a 32-bit type if a non-default build-time option is issues to the `configure` script, or otherwise if the host machine is detected to support `c_long`s larger than 32 bits.
The newlib definition is shown to be always a `c_long` when not running under any one of Cygwin and a machine not following the LP64 memory model abstraction. This seems to hold across all targets currently supported in the `libc` crate. The exceptions that were being made for `espidf` and `vita` did not seem correct. The definitions in their corresponding forks fit exactly those of the upstream newlib repo maintained by the Cygwin developers. More details can be found in the accompanying PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adresses bit-width representation issues under the newlib target environment concerned with
both the
time_tandoff_ttypes.In newlib,
time_tis conditionally defined as being ac_longlongunder thehorizonandespidftarget operating systems. Otherwise, it always defaults to using a 32-bit signed integer.The
off_tis always defined as a 64-bti signed integer except inespidfand invita, wherethey are defined as
c_longandc_int, respectively.This is possibly incorrect. The newlib upstream repo maintained by Cygwin defines
time_tconditionally as being either one of a
c_longor a signed integer type with at least 64 bitsprecision. The definition will always default to the latter unless one issues
--enable-newlib-long-time_tto theconfigurebuild script, or if the host system is known tohave
c_longs larger than 32 bits. In much the same vein, the espressif upstream repo for theirfork of newlib uses the exact same definition, and so does the devkitARM shipped for development on
horizon. Support forespidfwith 32-bittime_tis likely provided for compatibility purposesfollowing the analogous compatibility shims espressif has set in place in their upstream repos [1]
[2].
The upstream repo also includes definitions for the
off_ttype that depend mostly on whether thetarget machine is running under the Cygwin environment and is not following the LP64 model. This
definition remains the same across all forks from espressif and the VitaSDK. The specifics of the
type's bit-width are left to the C implementation's discretion in most cases, as the type is only
defined as being a
c_longlongin the afore mentioned case (Cygwin + non-LP64.) Indeed, one couldinfer from this that the type is likely to be 64-bits wide as a consequence of running under the
LP64 model and having a fallback definition in terms of a
c_long. That, though, could not be thecase. The conditionally compiled code in charge of those
typedefs may evaluate that the system isnot running under Cygwin and still provide a
c_long-relative definition. According to the GNUdocs, the preprocessor will short-circuit expressions involving logical operators to
#ifin muchthe same way as in "runtime" C. Even though the likelihood of there being systems other than Windows
that follow LLP64 is not large, this does mean we are always exposing a 64-bit definition, when
off_tcould be 32 bits wide.This patch changes the definition to fit a 64-bit signed integer
time_tunless compiling for theafore mentioned environment with the existing
cfg(espidf_time32)enabled. Note this option is"documented" as being meant for end users of the
libccrate to enable, but no information on it isprovided anywhere in all of the
libccrate. It seems like the only users of thatcfgare theprojects 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 havecustom modules at the end of the
newlib/mod.rsmodule. Notably, no architecture-specificdefinitions were found for
aarch64.This patch also changes the definition of
off_tto be unconditionally be ac_long, for the abovementioned reasons.
Changes introduced in this patch have not yet been tested in
horizon(for thetime_tchange) norin
vita(for theoff_tchange.) For that reason, the PR is currently open as a draft, pendingtesting on
horizon. The PR has been opened anyway to the last three days' worth of work.The test consisted simply of checking the
size_of::<T>()forT = libc::time_tandT = libc::off_tthrough the QEMU emulator withesp-32as the target. These bitwidths werecompared with their values usptream, and they seem to fit correctly.
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 multipletarget systems, but the one under
include/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 their
time_tdefinition.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