types.h: time_t as int64_t

[GUIDINGLI]

Summary

types.h: time_t as int64_t

The following code will generate errors:
auto now = time(nullptr);
auto last_active_time = GetEventService(self->ctx_)->getActiveTime(); if (last_active_time + 60 * 1000 / 1000 <= now) {

src/ams/../controller/controller_timer.h: In lambda function: src/ams/../controller/controller_timer.h:117:57: warning: comparison of integer expressions of different signedness: 'long long int' and 'long long unsigned int' [-Wsign-compare]
117 | if (last_active_time + 60 * 1000 / 1000 <= now) {

ref:
https://www.gnu.org/software/libc/manual/html_node/Time-Types.html

On POSIX-conformant systems, time_t is an integer type.

Advantage:
For the most POSIX applications they assume the time_t as signed and do compare with 0.
The code will become more compatible after this modification.

Disadvantage:
year 2038 problem

Someone think time_t will be overflow when set int32_t, should think his code should open TIME64 or handle the overflow himself.

Impact

32 bit time_t is now signed
This will cause Y2038 problem, if your system has this problem, you should open TIME64 or handle the overflow.
‘ put things right once and for all ’

Testing

CI

[acassis]

I think it was discussed some time ago, but I don't remember about the consensus: if it should be unsigned or signed. I think signed makes sense

[xiaoxiang781216]

I think it was discussed some time ago, but I don't remember about the consensus: if it should be unsigned or signed. I think signed makes sense

ALL other POSIX OS use signed integer, and we already find the compatibility issue, so it's better to follow their convention to improve the compatibility.

[yamt]

types.h: time_t as int64_t

The following code will generate warnings: auto now = time(nullptr); auto last_active_time = GetEventService(self->ctx_)->getActiveTime(); if (last_active_time + 60 * 1000 / 1000 <= now) {

src/ams/../controller/controller_timer.h: In lambda function: src/ams/../controller/controller_timer.h:117:57: warning: comparison of integer expressions of different signedness: 'long long int' and 'long long unsigned int' [-Wsign-compare] 117 | if (last_active_time + 60 * 1000 / 1000 <= now) {

does getActiveTime return something which is not time_t?

[yamt]

I think it was discussed some time ago, but I don't remember about the consensus: if it should be unsigned or signed. I think signed makes sense

ALL other POSIX OS use signed integer, and we already find the compatibility issue, so it's better to follow their convention to improve the compatibility.

otoh it breaks compatibility with nuttx itself.
if we make it unsigned, we should deprecate 32-bit time_t, IMO.

[yamt]

@GUIDINGLI
can you include pros/cons and references to prior discussions in the commit message?
i think a single incomplete example of compiler warning is not enough for this kind of changes.

[xiaoxiang781216]

I think it was discussed some time ago, but I don't remember about the consensus: if it should be unsigned or signed. I think signed makes sense

ALL other POSIX OS use signed integer, and we already find the compatibility issue, so it's better to follow their convention to improve the compatibility.

otoh it breaks compatibility with nuttx itself.

Which kernel code does depend on the unsigned time_t? Anyway, we need ensure the code owned by NuttX working for time_t which could be signed/unsigned or 32/64bit. But many userspace library comes from *nix, they may never try or encounter the unsigned time_t before since all POSIX OS except NuttX define time_t to signed.

if we make it unsigned, we should deprecate 32-bit time_t, IMO.

Actually, we never ship the product which define time_t to 32bit. It's fragile and hard to handle 2038 issue with 32bit integer.

[yamt]

I think it was discussed some time ago, but I don't remember about the consensus: if it should be unsigned or signed. I think signed makes sense

ALL other POSIX OS use signed integer, and we already find the compatibility issue, so it's better to follow their convention to improve the compatibility.

otoh it breaks compatibility with nuttx itself.

Which kernel code does depend on the unsigned time_t?

i meant, our own 32-bit time_t is unsigned even if we merge this PR.

if we make it unsigned, we should deprecate 32-bit time_t, IMO.

Actually, we never ship the product which define time_t to 32bit. It's fragile and hard to handle 2038 issue with 32bit integer.

i meant we == nuttx.
i guess you meant we == xiaomi.

i agree it doesn't make much sense to ship a product with 32-bit time_t anymore.

[GUIDINGLI]

@GUIDINGLI @acassis @xiaoxiang781216

Update time_t to int32_t & int64_t either in TIME32 or TIME64

[GUIDINGLI]

@acassis @xiaoxiang781216 @yamt @jkivilin
If you don't have any more questions, then this PR will be merged after CI pass

[xiaoxiang781216]

@acassis @xiaoxiang781216 @yamt @jkivilin If you don't have any more questions, then this PR will be merged after CI pass

I always prefer to align the implemented define behavior with other popular POSIX OS to improve the compatibility.

[yamt]

this makes 32-bit time_t almost unusable. if we make this change, we should make a bold statement to users.

[yamt]

@acassis @xiaoxiang781216 @yamt @jkivilin If you don't have any more questions, then this PR will be merged after CI pass

i feel a few days is too short for this kind of change. please wait for at least a week. also, please make a bold announcement with the explanation of implications (especially negative ones) on the mailing list.

[yamt]

i still think it's better for nuttx to keep time_t unsigned until we actually drop 32-bit time_t support.
but i can understand people prefer signed time_t.

[slorquet]

Disadvantage:
None.

Are you kidding?

This is putting a HUGE number of unknown projects at risk. You dont know which unknown projects are using unsigned 32 bits timestamps out there. This will be a major and dangerous regression for them.

This change requires these project to be EXTRA careful about how they handle time comparisons.

AT LEAST, please, add an explicit #warning "32 bit time_t is now signed" that triggers when CONFIG_SYSTEM_TIME64 is not defined. Pretty please.

[acassis]

@xiaoxiang781216 please wait before merging this PR. @yamt and @slorquet are against this change.

I think compatibility with other OS is good, but the Y2038 BUG is a big concern here!

[slorquet]

Also, can someone be technically explicit about what this means?

Impact
CI

Testing
CI

[acassis]

@xiaoxiang781216 is there some way to show the warning at end of the compilation? Maybe #warning will be ignored because people doesn't see it during the build process

[slorquet]

year 2038 problem comes from the fact that the max value is decreased from 4294967295 (Sunday, February 7, 2106 6:28:15 AM) to 2147483647 (Tuesday, January 19, 2038 3:14:07 AM)

Good remark Alan.

One, or several, ways to reliably warn the user is enough for me to accept this change.

I oppose doing this in a way that users will not notice.

edit: warning could be amended with a suggestion to upgrade code to 64-bit timestamps.

[cederom]

• Please wait until discussion is resolved (here and on the mailing list).
• This is a big breaking change, should not be enforced, nor self-merged.
• "CI" as Impact and Testing description of the PR is unacceptable for this kind of change.
• Introduction of Y2038 problem seems the biggest impact of the 32-bit time after that change. Could you please grep and identify impacted projects in nuttx and nuttx-apps repo?

[GUIDINGLI]

Disadvantage:
None.

Are you kidding?

Please be mindful of your words.

This is putting a HUGE number of unknown projects at risk. You dont know which unknown projects are using unsigned 32 bits timestamps out there. This will be a major and dangerous regression for them.

As I said in the mail list:
You should find a better way to ‘ put things right once and for all ’, and not use the unsigned int to postpone the occurrence.

This change requires these project to be EXTRA careful about how they handle time comparisons.

Lots of bug are report on this, after the SUB they suppose result will be < 0, but compare failed caused by the time_t to unsigned.

AT LEAST, please, add an explicit #warning "32 bit time_t is now signed" that triggers when CONFIG_SYSTEM_TIME64 is not defined. Pretty please.

Accept this, added

@slorquet

[xiaoxiang781216]

This is putting a HUGE number of unknown projects at risk. You dont know which unknown projects are using unsigned 32 bits timestamps out there. This will be a major and dangerous regression for them.

This change requires these project to be EXTRA careful about how they handle time comparisons.

@slorquet unsigned time_t need be EXTRA careful than signed time_t since almost project only test with signed time_t due OS mapping time_t to signed type.

[slorquet]

@xiaoxiang781216 Yep, I am aware of it, but as said on the mailing list I think the signed cast when comparing variables that can roll over is a must.

In all cases if everyone agree that signedness of time_t should be a kconfig option that defaults to the legacy state, I'm completely ok with it.

Making it an option gives stability to projects while allowing the use of the new feature.
I would be extra happy if the same could be done with the 64 bit version but that is secondary.

[cederom]

• To move forward and keep things compatible and elegant I proposed Kconfig selectable type for time_t so its not hardcoded and so developers have a choice.
• We currently discuss on the mailing list whether to split that type defaults for 32/64-bit platforms and smaller ones, or to have common single list of types to configure via Kconfig before build for time_t, and it turns out int64_t seems best default, while easy switch to int32_t or uint32_t is still available for the developer if necessary as required.
• One list of choice should keep this setting crystal clear, in one place, with conscious manual selection (no hidden inconsistencies, no auto-detection, no variants, etc), configuration level selectable, no code modification, etc.
• As this selection is crucial it can be printed explicitly on screen summary as part of configure process.
• @codebje presented in-depth analysis on various architectures and confirmed using int64_t as virtual type variable should have small impact and is worth solving the 32-bit storage problem whether signed (Y2038 problem) or unsigned (build errors and on ported GNU software). Biggest impact will be on Cortex-M0, 16-bit, and 8-bit platforms (this can be tuned with configuration selectable type).

  https://godbolt.org/z/8bKj6an4z
  I don't think, personally, that any of this is reason enough to worry about it. Using an int64_t for time_t is IMO a perfectly reasonable default for 32-bit and up targets. and possibly for all targets. Offering a kconfig option to support 32-bit (signed or unsigned) time_t offers an easy work-around should anyone find that the couple of percent increase in code size and execution time causes their project to fail - or if anyone is in the unfortunate situation where they depend on code that's made assumptions about size or signedness.

• @slorquet proposes casting to signed on each time arithmetics as solution to signed/unsigned problem but leaving defaults as-is.
• @yamt had some objections to configuration selectable time_t types but we are waiting for precise alternative solution proposition. I guess @yamt along with @slorquet want to keep uint32_t as default not the int64_t do I get this right?
• @patacongo what solution sounds best for you? :-)

[xiaoxiang781216]

@GUIDINGLI please update as the discussion in email list.

[cederom]

There is no final decision yet but it would help the decision with the proof-of-concept:

1. Kconfig configurable time_t type selection.
2. Possible choices: int64_t, int32_t, uint32_t.
3. int64_t is the default (according to POSIX.1-2024 / IEEE Std 1003.1-2024 / The Open Group Standard Base Specifications, Issue 8 [1][2][3]).

[1] https://pubs.opengroup.org/onlinepubs/9799919799/
[2] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/time.h.html
[3] https://pubs.opengroup.org/onlinepubs/9799919799/basedefs/sys_types.h.html

[patacongo]

3. int64_t is the default (according to POSIX.1-2024 / IEEE Std 1003.1-2024 / The Open Group Standard Base Specifications, Issue 8 [1][2][3]).

It is not "the default", it is the exact with of time_t as specified by POSIX 2024. It is not a choice. It is not an option. It is the POSIX required size.

[cederom]

@cederom: > 3. int64_t is the default (according to POSIX.1-2024 / IEEE Std 1003.1-2024 / The Open Group Standard Base Specifications, Issue 8 [1][2][3]).

@patacongo: It is not "the default", it is the exact with of time_t as specified by POSIX 2024. It is not a choice. It is not an option. It is the POSIX required size.

I see, so we end up with simply int64_t time_t according to POSIX.1-2024, no Kconfig selectable options :D

[patacongo]

I see, so we end up with simply int64_t time_t according to POSIX.1-2024, no Kconfig selectable options :D

Petty hard to escape that interpretation. I would have preferred a little more efficient use of memory. Like setting aside some bits for better time resolution now that we have more bits than needed.