HowardHinnant / date

A date and time library based on the C++11/14/17 <chrono> header
Other
3.08k stars 670 forks source link

4 tests failing when using system TZDB with GCC #713

Closed Tachi107 closed 2 years ago

Tachi107 commented 2 years ago

When running testit from release 3.0.1 with OPTIONS='-DONLY_C_LOCALE=1 -DUSE_OS_TZDB=1', the tests posix/ptz.pass.cpp, solar_hijri_test/parse.pass.cpp, and tz_test/zoned_time.pass.cpp fail with error terminate called after throwing an instance of 'std::system_error' - what(): Unknown error -1. When testing the latest master, two tests from tz_test/zoned_time.pass.cpp fail.

This happens when using GCC 10.3.0 on Debian Testing and GCC 11.2.0 on Debian Unstable, but not with Clang 11.1.0 on Debian Testing.

Tested with 3.0.1 with https://github.com/HowardHinnant/date/commit/052eebaf0086e6bbc5ead01c3f1a8f02496aa701 applied and with latest master.

Tachi107 commented 2 years ago

The same failures occur when not defining ONLY_C_LOCALE, so USE_OS_TZDB seems to be the reason why they are failing

HowardHinnant commented 2 years ago

Thanks for your report.

On macOS I get also get a failure on posix/ptz.pass.cpp:

Assertion failed: (is_equal(tzi->get_info(tp), tzp.get_info(tp))), function main, file ptz.pass.cpp, line 96.

The binary form of the tzdb has save==0 for "Europe/Dublin" on 2021-01-01, which is incorrect. The save should be -60min. This error in the macOS tzdb files is relatively harmless. It doesn't cause errors in mapping between local time and UTC, but it will cause the C struct tm member tm_isdst to display 0 instead of 1.

I don't know for sure if this is the same error on the platform you are seeing. And I am not replicating the other two errors you're reporting, and so am not sure what their cause is. I would not be surprised if they were also due to minor errors in the OS-supplied binary database. The binary form of the database lacks detailed information on save, and only includes whether it is zero or non-zero.

Tachi107 commented 2 years ago

Do you have any suggestions as how I could debug this?

HowardHinnant commented 2 years ago

You can cd into a directory and run testit from there. This will run only the tests in and below that directory, shortening test time.

If you can insert print statements in the test to narrow down what line the test is failing on, that can help. Also, most everything in this library is printable. For example to gather the information I posted above about ptz.pass.cpp I simply did this:

std::cerr << tzi->get_info(tp) << '\n';
std::cerr << tzp.get_info(tp) << '\n';

This printed out the sys_info for each of these timezone/time_point pairs. sys_info is documented here: https://howardhinnant.github.io/date/tz.html#sys_info

Tachi107 commented 2 years ago

After debugging a bit, I can see that the line that throws the first exception in zoned_time.pass.cpp is zoned_seconds zt = {locate_zone("America/New_York"), local_days{2017_y/jul/5}};.

Calling locate_zone() and local_days{} individually works, but it seems that the construction of zoned_seconds causes an std::system_error

Tachi107 commented 2 years ago

Uhm... I tried printing locate_zone("America/New_york")->name(), and I got and std::runtime_error saying America/New_york not found in timezone database.

->name() doesn't work with Clang either, but why do the tests pass when compiling with it?

Edit:

HowardHinnant commented 2 years ago

Wild theory. This may be a duplicate of https://github.com/HowardHinnant/date/issues/614. Did you build the IANA tzdb binary files yourself with zic? My parser only handles output built with the zic option -b fat. If these files were built without that option, that could be causing these symptoms.

I'm not sure at the moment how to test for this possibility. And modifying the parser to handle -b slim is a bigger job than I can take on right now.

Tachi107 commented 2 years ago

Did you build the IANA tzdb binary files yourself with zic?

Nope, (I never heard it before), and it happens on both my main PC and my laptop, and also in a clean Debian Testing environment (VM), so everything should be clean.

Edit: maybe the default Debian tzdb is now built with -b slim, but I don't know. Do you know how I could check it?

HowardHinnant commented 2 years ago

I've searched around and I don't see a way to check this yet. I also can't find anything on it in the Debian docs.

Tachi107 commented 2 years ago

If it can help, here's the package that contains the tzdb: https://packages.debian.org/testing/tzdata .

614 mentions that they started using -b slim by default, but by looking at the manual it seems that the default is still -b fat. If using zic without the -b argument is equal to specifying -b fat, the tzdb in Debian should be in the "fat" format, since in their build script they run zic without -b.

Edit: yes, zic now uses -b small by default. See https://github.com/eggert/tz/blob/8b409e22d7e1b70bf9364b014a8e359908a507a9/NEWS#L462

This means that all systems with an updated tzdb are running a version built with zic -b small.

HowardHinnant commented 2 years ago

Right. But I don't know if Debian is using an older zic or the latest 2021e zic. zic is packaged separately from tzdata: https://www.iana.org/time-zones

There's a decent chance Debian is using the latest tzcode. But I'm not sure how to confirm that.

Tachi107 commented 2 years ago

See the updated comment Oh, and zic --version - zic (Debian GLIBC 2.32-4) 2.32

HowardHinnant commented 2 years ago

I don't have a quick fix for this, sorry.

Tachi107 commented 2 years ago

Oh, and zic --version - zic (Debian GLIBC 2.32-4) 2.32

Uhm... Looking at glibc's source, it seems that they are still using tzcode 2020a (https://github.com/bminor/glibc/commit/61d64408a1f42b0340d37ea0c90a9f028ffb1bfd).

This probably means that Debian is shipping a tzdb updated to version 2021e, but built with a version of zic that is based on version 2020a. So, in theory, Debian's zic still defaults to -b fat.

I don't have a quick fix for this, sorry.

Don't worry, I don't need a quick fix. I'm here to help ;)

HowardHinnant commented 2 years ago

Thanks much. Your latest research appears to indicate my -b slim theory is a wild goose chase...

HowardHinnant commented 2 years ago

Uhm... I tried printing locate_zone("America/New_york")->name(), and I got and std::runtime_error saying America/New_york not found in timezone database.

I can fix this one! The y should be Y: "America/New_York". :-)

Tachi107 commented 2 years ago

The y should be Y: "America/New_York". :-)

Oh, right... :/

Tachi107 commented 2 years ago

Wait... Maybe that's my fault

Tachi107 commented 2 years ago

Sorry! I'm just dumb, I guess. The lowercase y is my fault, your tests already use New_York... They are still failing, but that's unrelated to the lowercase y.

Tachi107 commented 2 years ago

To recap, https://github.com/HowardHinnant/date/issues/713#issuecomment-957242022 is still accurate, except for _"locate_zone()->name("America/New_york") always throws an exception with GCC and Clang"_

The relevant part is that zoned_seconds zt = {locate_zone("America/New_York"), local_days{2017_y/jul/5}}} throws an exception with GCC, but works with Clang

HowardHinnant commented 2 years ago

Does the .what() from the exception have anything useful?

Tachi107 commented 2 years ago

Nope, "Unknown error -1"

Tachi107 commented 2 years ago

I tried running the test under LLDB.

The zoned_time constructor fails when trying to set the local_time parameter.

tp_(zone_->to_sys(t)) -> to_sys_impl(tp, choose{}, std::true_type{}) -> auto i = get_info(tp) -> get_info_impl(date::floor<std::chrono::seconds>(tp)) -> init(); -> std::call_once(*adjusted_, [this]() {const_cast<time_zone*>(this)->init_impl();}); -> exception.

So, the exception is trowed by std::call_once, and luckily I've encountered this issue before. Let me compile the test with -pthread and... It works! For some reason, std::call_once requires -pthread when using GCC.

Edit: yep, using -pthread solves all the issues

HowardHinnant commented 2 years ago

Thanks much for you persistent efforts on this. I should've guessed this one, as I've seen it before.

DavisVaughan commented 2 years ago

The -pthread requirement is mentioned in the linux installation docs too:

Linux specific:

You may have to use -lpthread. If you're getting a mysterious crash on first access to the database, it is likely that you aren't linking to the pthread library. The pthread library is used to assure that that library initialization is thread safe in a multithreaded environment.

https://howardhinnant.github.io/date/tz.html#Installation

Tachi107 commented 2 years ago

The -pthread requirement is mentioned in the linux installation docs too

I should read manuals more often 😅️

elfin-sbreuers commented 6 months ago

@HowardHinnant would it be an option to state in the documentation of your timezone solution that only the fat version of the timezone data is supported?

We observed on our embedded system that the day light saving time was not properly detected. It was working nicely on our Ubuntu system. And copying the Ubuntu files to the embedded device solved the issue as well. Eventually we found this additional option for building the tzdata package with -b fat. Maybe it helps other people to find the reason quicker, if they knew that there is this limitation.

Thanks anyways for your beautiful piece of code.