refactor: change definition of time_t in TEEOS#5130
Conversation
time_t in TEEOStime_t in TEEOS
|
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.
|
CI actually passes. There seems to be an issue with a glob import that is not used, but this has not |
e6a8671 to
56108bc
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. |
There was a problem hiding this comment.
At least for OhOS it was the case that support was added after musl's switch to 64-bit only, meaning the platform never had a chance of supporting 32-bit (a couple of these platforms are in this boat https://github.com/rust-lang/libc/blame/e34c8b6da178f87bfbb4613fa9b7a8318548eff3/build.rs#L118-L119). So I'd expect the same thing here, either i64 or c_longlong is fine.
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.
56108bc to
843cd29
Compare
|
An |
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