time_t size is inconsistent
#2758
Comments
slaff
pushed a commit
that referenced
this issue
Apr 12, 2024
This PR improves test coverage for the `DateTime` class and adds a number of necessary fixes. It also adds some obvious omissions. **Compare day/month names without case-sensitivity** Allows more use cases when converting strings. **Ensure host builds (linux) use 64-bit `time_t`** Defaults to 32-bit if not specified by `_TIME_BITS`. Now consistent with embedded toolchains which use fixed size (all but IDF 4.x are 64-bit). See issue #2758. **Document and add static check for time_t range** CI will fail build if incorrect. For legacy toolchains (Windows host, IDF 4.x) will fail if it ever gets fixed as a reminder. **`mktime` broken by axtls. Correct implementation available in newlib.** Calls esp8266 ROM function which doesn't support 64-bit time_t. **Fix `isLeapYear` implementation** Doesn't account for century. **Add enumeration for Month value** For backward compatibility methods use standard types, but makes code easier to read and avoids off-by-one errors in application code. **Fix `toUnixTime` to accommodate negative values** `time_t` can represent dates before 1 Jan 1970. **Fix `toUnixTime` parameter handling outside normal range** Comment reads: "Seconds, minutes, hours and days may be any value, e.g. to calculate the value for 300 days since 1970 (epoch), set day=300" This requires a more appropriate parameter type (int) and casting so calculations are 64-bit. **Apply `const` to DateTime methods** As appropriate. **Add `fromISO8601` method** Complements `toISO8601`. Update Basic_DateTime sample so strings can be interpreted. **Expand HTTP string conversion** Various RFC versions suggest more relaxed interpretation of strings. Accept e.g. "Sun" or "Sunday". Also "Sunasdlfkj" but do we care? Make leading day of week optional, since the value gets discarded anyway Fold whitespace **Fix `setTime`, `fromUnixTime` methods** Results are not always correct. e.g. Out by a whole day for `0x66152e16`. Code in `gmtime_r` markedly different, and not bloaty, so just use that. **Provide `gmtime_r` implementation for Windows host builds** Windows does have `gmtime` but doesn't behave like glibc/newlib does. Safest to just copy the code from newlib. https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/time/gmtime_r.c **Use DateTime to produce default IFS TimeStamp string** Just output UTC, localising will likely be wrong. **Include milliseconds with '%T' format if non-zero** Consistent with ISO time strings. This is different from libC `strftime` behaviour, but then `struct tm` doesn't have a fractional seconds field. **Update tests** Generate test data using python script Check dates before 1970 Include 64-bit-only tests Verify `setTime` calls with out-of-range offsets **Pull out some utility methods. Some internal functions could be useful so add those as static methods:** bool isLeapYear(uint16_t year); uint8_t getMonthDays(uint8_t month, uint16_t year); String getLocaleDayName(uint8_t day); String getLocaleMonthName(uint8_t month); String getIsoDayName(uint8_t day); String getIsoMonthName(uint8_t month); uint16_t getDaysInYear(uint16_t year); **Replace helper macros with inline functions**
This was referenced Apr 22, 2024
Merged
slaff
pushed a commit
that referenced
this issue
Apr 23, 2024
Further to #2758 host builds should generally match code run on hardware. As mentioned this is not straightforward for Windows, but for Linux we should expect 64-bits as the default. GCC 10+ can make use of `_TIME_BITS` to ensure we get this behaviour. More specifically, this is a feature of `glibc` shipped with those versions. For earlier versions, even though the compiler is 64-bit we are building in 32-bit mode and `time_t` stays stuck in 32-bits. There are various discussions about this but the short answer is that prior to GCC 10 (libc 4) 32-bit applications get 32-bit `time_t`. Instead of generating a `static_assert` (which stops compilation) we get a compiler warning - this still requires building in STRICT mode.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Background
I'm working on some improvements to the
DateTimeclass and one issue is with using signed 32-bit values fortime_t.First, I'm not sure what size
time_tactually is, so did a couple of CI builds with astatic_assert(sizeof(time_t) == 8)statement inDateTime.cpp.So a successful run ✓indicates time_t is 64-bits, and failed run X indicates 32-bits.
(1) Current develop branch
(2) With
_TIME_BITS=64Note that all esp32 SOCs have the same result.
(NB. These results are different from my original post, since corrected.)
time_tis fixed at 64-bits._TIME_BITSdefine.Library versions
time_tis C-library dependent.For non-Windows systems GCC ships with GNU-C library (glibc). Although built for 64-bit systems it's used to compile in 32-bit mode, so defaults to 32-bit
time_t.Embedded architectures (esp8266, rp2040, esp32) use a version of glibc called
newlibwhich provides similar functionality but more tailored to embedded systems. This has a build option which fixestime_tat either 32-bit or 64-bit.Conclusion
We should move Linux host builds to 64-bit
time_tmode for consistency. See https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fTIME_005fBITS.Tests need to accommodate both 32-bit and 64-bit
time_tvariants for now, but when IDF 4.x support is removed that won't be necessary.Fixing host builds for Windows is probably going to require migration to mingw64.
Windows users are strongly encouraged to use WSL for Sming development. (Or, preferably, move to Linux.)
The text was updated successfully, but these errors were encountered: