refactor: change definition of time_t in TEEOS#5130
Draft
dybucc wants to merge 1 commit into
Draft
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 |
de93600 to
3a011ec
Compare
This comment has been minimized.
This comment has been minimized.
3a011ec to
e6a8671
Compare
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
CI actually passes. There seems to be an issue with a glob import that is not used, but this has not |
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.
e6a8671 to
56108bc
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. At present, we expose a singletime_ttype whose bit-width relies on semantics specific to each target system, asc_longis likely to be 64-bit wide under platforms following LP64 and 32-bit wide 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 the OhOS 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 OhOS SDK shows that all type definitions accross architectures follow musl's. For reference, I ran
ripgrepacross the SDKs for Darwin, Linux and Windows 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*MAXare included (see #3131)cd libc-test && cargo test --target mytarget); especially relevant for platforms that may not be checked in CI