Extending time_t to u64 (c_longlong)

[ivmarkov]

As per information from @igrr , ESP-IDF 5.0 will soon extend the time_t type from 32 to 64 bits.

The esp-idf-* crates can handle this transparently, as they have full access to the ESP-IDF environment.

The situation is not so rosy with our Rust STD support, which relies on manually-defined types in the Rust libc crate:
(In particular, the time_t type for newlib is defined here).

I was only able to come up with three ways to model this:

• Option 1: Introduce new ESP-IDF targets for ESP-IDF 5. I.e.:

  • riscv32imc-esp-espidf5 in addition to riscv32imc-esp-espidf
  • xtensa-esp-espidf5 in addition to xtensa-esp-espidf
  • xtensa-esps2-espidf5 in addition to xtensa-esps2-espidf
  • xtensa-esps3-espidf5 in addition to xtensa-esps3-espidf
  • Now, to minimize the upstream changes in Rust STD which are branching by #[cfg(os_target = "esp-idf")], we still need - in the *-espidf5 targets - to report as os_target esp-idf and then use another target property to differentiate between ESP-IDF5 and earlier. I'm thinking - as terrible as it sounds - to (ab)use the currently unused vendor property and have it defined to espressif for ESP-IDF 4, and espressif-espidf5 in the targets that are suffixed -espidf5.
  • Note that we can have ^^^ all of the above the other way around too: current targets become compatible with ESP-IDF 5, and for backwards compatibility with earlier ESP-IDF targets we can introduce *-espidf4 targets which would have espressif-espidf4 as vendor property

• Option 2: Rely on the user setting a Rustc flag in .cargo/config.toml

  • I.e. something like

  [build]
  rustflags = ["--cfg", "esp-idf-long-time-t"]
  

  • The fix in the rust libc crate can then do an additional branching by #[cfg(all(target_os = "esp-idf", "esp-idf-long-time-t"))]
  • Problem 1 with that approach is I'm not sure libc maintainers will accept that - but doesn't hurt if we try
  • Problem 2 is that if users are switching from ESP-IDF 4.X to ESP-IDF 5 branch or vice versa, they have to comment/uncomment this flag. Note however that this is not much different from asking them to switch to another target (i.e. from xtensa-esp-espidf to xtensa-esp-espidf5

Note also that for both Option 1 and Option 2 users might try to compile with e.g. 5.X ESP-IDF and 4.x target (or - if Option 2 is used - with 5.X ESP-IDF and the rustflags = ["--cfg", "esp-idf-long-time-t"] commented out). However, this has an easy workaround in that esp-idf-sys (which they need to compile anyway too) can detect that it is being compiled with 5.X ESP-IDF yet what libc returns is a 32 bit time_t and abort the compilation with a suggestion "Please add rustflags = ["--cfg", "esp-idf-long-time-t"] to your .config/cargo.toml file. And of course the other way around.

• Option 3: Maintain backwards-compatibility mode for compiling ESP-IDF 5 where time_t is 32 bit. We would use this mode in Rust. Once ESP-IDF5 becomes ubiquitous, we will drop support for ESP-IDF 4.X in Rust and we'll just switch to the 64 bit time_t.

My order of preference:

• Option 3 (obviously)
• Option 2
• Option 1

@MabezDev @jessebraham ^^^

[ivmarkov]

Just occurred to my mind that we can have Option 2 and Option 3 together too.

[MabezDev]

I like option 2 & 3 together, provided the libc maintainers will allow us that cfg flag. I think most people (excluding us) generally pick a idf version and stick with it. It seems like upgrading to V5 from V4 wouldn't need to change the cargo config, as the cfg would essentially become a no-op, so it would be safe to leave it in there? I suppose that depends on how the feature is implemented in libc though.

esp-idf V4 is here to stay for a couple of years so dropping it immediately is probably unfeasible.

[jessebraham]

Sorry, missed this when it was first posted.

I think we're probably all in agreement here, options 2 & 3 seem like the best path forward to me as well. Don't really have much else to add that hasn't already been said.

[ivmarkov]

I like option 2 & 3 together, provided the libc maintainers will allow us that cfg flag. I think most people (excluding us) generally pick a idf version and stick with it. It seems like upgrading to V5 from V4 wouldn't need to change the cargo config, as the cfg would essentially become a no-op, so it would be safe to leave it in there? I suppose that depends on how the feature is implemented in libc though.

Not really. Assuming only Option 2 is implemented:
If you keep the flag there when downgrading from V5 to V4 (or the other way around - as in you don't have the flag but upgrade from V4 to V5 - does not really matter if you go left or right), then libc will be compiled with c_ulonglong enabled, which would be wrong for V4. What is saving us is that after compiling libc (and std), esp-idf-sys compilation comes next. Now this guy (esp-idf-sys build scripts) can also look at the flag, and if it is incorrect for the ESP-IDF version that it compiles, it can hard-fail with an error saying "please remove/comment that flag in .config/cargo.toml.

Basically the flag is a workaround for the fact that libc cannot really "talk" to esp-idf-sys and ESP-IDF, nor does it know anything about ESP-IDF's actual C headers. It is really just a bunch of hard-coded mappings, maintained by hand and not generated with bindgen (as we do in esp-idf-sys). There are good reasons for that of course.

Now, with Option 3 it is a bit easier:

• If the flag is present, and the used ESP-IDF is V4, the build should still fail with a hard error, as time_t is then mapped in the libc crate to c_ulonglong which is not a setup supported by V4
• If the flag is present, and the used ESP-IDF is V5, the build will obviously succeed
• If the flag is not present, compiling against ESP-IDF V4 will obviously work; compiling ESP-IDF V5 will also work, but it has to be compiled in "backwards compatibility mode" by esp-idf-sys then (as in time_t mapped to 32 bit c_ulong).

[ivmarkov]

@igrr ^^^

[MabezDev]

Problem 1 with that approach is I'm not sure libc maintainers will accept that - but doesn't hurt if we try

FYI This was recently accepted, so I would imagine we can extend it for the espidf usecase.

[ivmarkov]

Problem 1 with that approach is I'm not sure libc maintainers will accept that - but doesn't hurt if we try

FYI This was recently accepted, so I would imagine we can extend it for the espidf usecase.

Yep, I saw that 'if' already.
The thing is, it is if-ing by target_os which I think is well accepted, because target_os is always present and part of the compilation target info.

We would instead be if-ing on an arbitrary cfg flag, which is not part of the information the compilation target carries with it:

• Basically cfg(all(target_os = "espidf", espidf_long_time_t))
• ... or - in case we want to model the flag as a string value rather than a bool - cfg(all(target_os = "espidf", espidf_time_t = "long")).

My hope is that because the examination of that flag will be constrained only for the case when the OS is "espidf", it might actually be accepted.

[ivmarkov]

By the way, when is this change coming to ESP-IDF V5?

[ivmarkov]

@igrr ^^^. Was this change released for ESP-IDF 5 or not?

[igrr]

@ivmarkov IDF v5.0 hasn't been released yet, but the master branch is now using the new toolchain with sizeof(time_t) == 8. (Starting from commit espressif/esp-idf@bf43076)

[ivmarkov]

@igrr Thanks, that's all I needed to know. This means, we have to upstream in Rust libc the check for esp_idf_long_time_t ASAP.

[MabezDev]

From the meeting, we made the preliminary decision to use the cfg route.

The default behavior will be to opt-in to 64bit time, this is to not break v4.4 users. In two years we may wish to flip the meaning or add a new flag for opting-out of 64bit time when v4.4 is deprecated and v5.0 becomes the default.

Crates that require modification:

• libc - for changing the types
• esp-idf-sys - for checking the two time_t types (Rust vs esp-idf) and asserting they are consistent.

[MabezDev]

With rust-lang/libc#2913 merged, I believe we can now close this.