refactor: change definition of time_t in TEEOS#5130
Open
dybucc wants to merge 1 commit into
Open
Conversation
time_t in TEEOStime_t in TEEOS
Contributor
Author
|
CI seems to have failed for reasons unrelated to the changes introduced in this PR. Rerunning it |
The type with which it was defined did not reflect the right definition, not by musl standard nor by the actual definitions found in the latest OhOS SDK. Indeed, `c_long` in Linux systems is likely to be defined as a 64-bit integer, but the definition in all of the above, which are mentioned to be used by TEEOS in the rustc target information, define `time_t` specifically as an `_Int64`. Sources are included in the accompanying PR.
de93600 to
3a011ec
Compare
Collaborator
|
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. |
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 aims to both discuss and (possibly) change the bit-width of the
time_ttype in TEEOS. Atpresent, we expose a single
time_ttype whose bit-width relies on semantics specific to eachtarget system, as
c_longis likely to be 64-bit wide under platforms following LP64 and 32-bitwide under platforms following LLP64, but there's a few things that lead me to believe this may
cause conflicts in TEEOS.
The first issue I would like to address is that if we're exposing the
time_tdefinition in theOhosOS SDK that TEEOS uses, our current definition is not completely correct. SDKs for all three of
Darwin, Linux and Windows use the same definition as musl; Namely,
typedef _Int64 time_t. Inlibc, though, we expose it as ac_long.Just to be sure, though, I then looked at the TEEOS upstream kernel repo, and they define
time_tin terms of either one of a 32-bit unsigned integer or a 64-bit unsigned integer, depending on the
machine's word size. That implies this target OS likely supports running on both such types of
machines, which means that our current definition is sort of OK on the bitwidth side but not on the
signed integer side.
The proposed change in this PR switches the type to be an explicit 64-bit signed integer, though
this has not taken into account the definition in the TEEOS kernel. That's why I would like to spark
some discussion on this, or otherwise call it a day and learn something new.
Sources
A regex search through the downloadable OhosOS SDK shows that all type definitions accross
architectures follow musl's. For reference, I ran
ripgrepacross the SDKs for Darwin, Linux andWindows with the following command:
I also ran it by more generally scoping the search to header files, but relevant results were the
same.
TEEOS kernel sources defining
time_tas an unsigned integer.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