[libc++][chrono] Adds tzdb_list implementation.

[Mordante]

Summary

This is the first step to implement time zone support in libc++. This
adds the complete tzdb_list class and a minimal tzdb class. The tzdb
class only contains the version, which is used by reload_tzdb.

Next to these classes it contains documentation and build system support
needed for time zone support. The code depends on the IANA Time Zone
Database, which should be available on the platform used or provided by
the libc++ vendors.

The code is labeled as experimental since there will be ABI breaks
during development; the tzdb class needs to have the standard headers.

Implements parts of:

• P0355 Extending <chrono> to Calendars and Time Zones

Addresses:

• LWG3319 Properly reference specification of IANA time zone database

[Mordante]

Rebased and CI fixes.

[ldionne]

libcxx/CMakeLists.txt

94

We could instead disable this by default except on Linux. This would avoid breaking the build for everyone on major platforms right now (which would happen if we switch to #error in the dylib).

libcxx/docs/Status/Cxx20.rst

54

libcxx/docs/UsingLibcxx.rst

530

I think we should document this in a separate page with implementation-defined behavior.

libcxx/include/__availability

179

356

libcxx/include/__chrono/tzdb_list.h

42

As part of this patch?

80

94–97

I think we should try to make it as clear as possible that this is not intended to be used by users.

Actually, we don't have to declare it here at all, we can only declare + override it in the test suite. We should add a comment explaining that this is necessary for testing in tz.cpp where that function is marked as _LIBCPP_WEAK.

libcxx/include/__config

1428–1435

Since we need to implement __libcpp_tzdb_directory in the dylib anyway, I think we should move this to tz.cpp to keep it as localized as possible.

libcxx/include/chrono

835

Is this missing _LIBCPP_HAS_NO_TIME_ZONE_DATABASE?

libcxx/src/tz.cpp

23

119

This should be a #error instead.

libcxx/test/libcxx/transitive_includes.gen.py

67

Also no-tzdb?

libcxx/utils/libcxx/test/features.py

571

This one would be removed if we turned tzdb support to OFF by default on platforms that don't support it.

[Mordante]

libcxx/include/__chrono/tzdb_list.h

42

Had a look and I was wondering whether we want to keep documenting our _LIBCPP_NODISCARD_EXT since the list is incomplete.

[Mordante]

Rebased and addresses review comments.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[ldionne]

LGTM with comments!

libcxx/CMakeLists.txt

92–93

Seems clearer.

libcxx/include/__chrono/tzdb_list.h

42

I think we should try to be consistent in this patch. We can talk about not documenting all of the _LIBCPP_NODISCARD_EXT in a rst document anymore (and I think I'd be in favour of that), but I think we should just be consistent with the current state of the code as of checking this patch in.

45

Have you considered making this a pimpl to hide the definition of the class into the dylib? It might help with keeping the added dependencies minimal, and it would also give us more freedom to change the implementation in the future (although IDK whether we'll ever need/want that freedom). I'm not asking you to do that, but I think it should at least be considered.

libcxx/test/std/time/time.zone/time.zone.db/time.zone.db.tzdb/tzdb.members.pass.cpp

36

We also need to do something like static_assert(std::is_same<decltype(tzdb.version), std::string>), don't we?

Right now tzdb.version could be anything with an assignment operator that returns a std::string, and the test would still pass.

[Mordante]

Thanks for the review!

libcxx/include/__chrono/tzdb_list.h

45

I had not really considered that since it's typically not something we do. The big disadvantage of the pimpl pattern is that it moves code to the dylib causing it not to be available on Apple platforms. In this case that is not a real issue; other parts of the TZDB are already in the dylib.

Calling the dylib might give some overhead. I don't think that is a huge issue. Looking at the amount of work needed to use time zones I don't see it as very fast code in the first place.

libcxx/include/module.modulemap.in

322

No longer needed.

[Mordante]

Rebased and addresses review comments.

[Mordante]

Retrigger CI.

[Mordante]

CI fixes.

[EricWF]

OK, here's a ton of comments. Thanks for addressing them.

libcxx/include/__chrono/tzdb.h

1

Why is this its own header?

libcxx/include/__chrono/tzdb_list.h

30

This conjunct deserves its own macro.

37

This should not be a public easily accessible constructor. At minimum it should take an unspellable tag type and not be single argument.

45

And this function can be replaced with *begin().

The fewer functions we put in the dylib the better. It's not strictly about speed, it's about surface area.

49

It seems unnecessary to export this function given that it's only used internally.

Please remove it.

54

Please remove these two functions from the dylib and instead just have them call begin() and end().

libcxx/include/__config

401

This macro is poorly named.

libcxx/modules/std/chrono.inc

213

I understand this #if 0 was existing.

But it's never OK practice to check into source #if 0. In particular without any explanation as to why it exists.

Code that needs to be wrapped in #if 0 isn't ready to be checked in.

libcxx/src/CMakeLists.txt

329

Why are you checking three options here?

It seems like if LIBCXX_ENABLE_TIME_ZONE_DATABASE is set, we should either error because we can't provide it without localization and filesystem, or we should provide it.

libcxx/src/tz.cpp

77

Nit. It doesn't look like this function "parses a string".

107

__matches sounds like it's a predicate. __consume_match makes more sense.

Also you don't need ugly identifiers inside source files. And its preferable that you don't use them.
It makes it appear as though these function are exported from the dylib or are from the headers.

127

Should this be nodestroy?

127

I would find a different way to initialize this that doesn't require having a public constructor.

140

This whole parsing bit seems overly complicated considering the pattern it's matching.
Can't we just read some large chunk out of the file, and then use string_view to walk it and parse it?

libcxx/src/tzdb_list.cpp

42

Doesn't this operation make the whole "thread safety" bit moot?

If you can erase an arbitrary member in the list, then that erase is potentially racy for any iterator on or before that element?

So either all the locking is unneeded, or we need to replace forward_list with a hand rolled atomic linked list. (Which shouldn't be too hard given the limited operations it needs to support).

72

Why is this a shared_mutex?

83

This should never be called.

libcxx/test/libcxx/diagnostics/chrono.nodiscard_extensions.compile.pass.cpp

25

Just make these normal tests and call the functions.

libcxx/test/libcxx/experimental/fexperimental-library.compile.pass.cpp

34

So it has complete TZDB?

libcxx/test/std/time/time.zone/time.zone.db/time.zone.db.list/iterators.pass.cpp

12

This is expected to fail if we have a complete tzdb implementation?

libcxx/test/support/test_tzdb.h

23

This is a clever way to test the internals. Good thinking.

Though it feels like this should probably return a string?

libcxx/utils/ci/buildkite-pipeline.yml

684

Please remove this configuration.

We don't have the capacity for it.

libcxx/utils/ci/run-buildbot

462

this doesn't need its own configuration right now.

It's also not clear to me that we need a configuration switch here.

If the user doesn't touch the tzdb interface, the fact that there is no database won't crop up.
So introducing a whole new configuration here is a bad idea.

Also, we effectively test this configuration when we test without filesystem.
Please remove it.

[abrachet]

We are seeing build failures after this patch. See https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-mac-xarm64/b8770674124156502625/overview.

FAILED: libcxx/src/CMakeFiles/cxx_experimental.dir/tzdb_list.cpp.o 
/opt/s/w/ir/x/w/llvm_build/./bin/clang++ --target=armv7-unknown-linux-gnueabihf --sysroot=/opt/s/w/ir/x/w/cipd/linux -DLIBCXX_BUILDING_LIBCXXABI -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_ENABLE_HARDENED_MODE -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_LINK_PTHREAD_LIB -D_LIBCPP_LINK_RT_LIB -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/opt/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/include/c++/v1 -I/opt/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/include/armv7-unknown-linux-gnueabihf/c++/v1 -I/opt/s/w/ir/x/w/llvm-llvm-project/libcxxabi/include -resource-dir=/opt/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/./lib/clang/18 --target=armv7-unknown-linux-gnueabihf -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -ffile-prefix-map=/opt/s/w/ir/x/w/llvm_build/tools/clang/stage2-bins/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins=../llvm_build/tools/clang/stage2-bins/runtimes/runtimes-armv7-unknown-linux-gnueabihf-bins -ffile-prefix-map=/opt/s/w/ir/x/w/llvm-llvm-project/= -no-canonical-prefixes -O2 -g -DNDEBUG -std=c++20 -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -Wextra -Wnewline-eof -Wshadow -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wunused-template -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-suggest-override -Wno-error -D_LIBCPP_ENABLE_EXPERIMENTAL -MD -MT libcxx/src/CMakeFiles/cxx_experimental.dir/tzdb_list.cpp.o -MF libcxx/src/CMakeFiles/cxx_experimental.dir/tzdb_list.cpp.o.d -o libcxx/src/CMakeFiles/cxx_experimental.dir/tzdb_list.cpp.o -c /opt/s/w/ir/x/w/llvm-llvm-project/libcxx/src/tzdb_list.cpp
/opt/s/w/ir/x/w/llvm-llvm-project/libcxx/src/tzdb_list.cpp:29:7: error: use of undeclared identifier 'tzdb_list'
   29 | class tzdb_list::__impl {
      |       ^
/opt/s/w/ir/x/w/llvm-llvm-project/libcxx/src/tzdb_list.cpp:81:27: error: use of undeclared identifier 'tzdb_list'
   81 | _LIBCPP_EXPORTED_FROM_ABI tzdb_list::tzdb_list(tzdb&& __tzdb) : __impl_{new __impl(std::move(__tzdb))} {}
      |                           ^
...

Would you mind taking a look?

[vvereschaka]

Hi @Mordante ,

here is few failed libc++ tests on Arm/Aarch64 Ubuntu targets with the following error:

libc++abi: terminating due to uncaught exception of type std::runtime_error: corrupt tzdb: expected character '#'

• https://lab.llvm.org/buildbot/#/builders/60/builds/13787
• https://lab.llvm.org/buildbot/#/builders/119/builds/14997

[Mordante]

Thanks for the reports. I'll have a look at the failed builds now.

[Mordante]

@abrachet @vvereschaka I added work-arounds for the issues you encountered. These are not proper fixes but should unblock the CI. Please let me know when the issues are not solved. I will keep an eye on the llvm buildbots too.

[Mordante]

@EricWF Please ping me on Discord when you have finished the review, then I will look at a followup patch.

libcxx/include/__chrono/tzdb.h

1

Because nowadays (for at least two years) we tend to work with small headers. I'm open to discuss that, but then we should have a general policy discussion about this and not in individual patches.

libcxx/modules/std/chrono.inc

213

In general I agree with that statement, however I feel this is a great exception of the rule.

I added the entire synopis to all module files and commented out the parts libc++ has not implemented. This makes it easier to enable them once these parts are implemented. Especially when the CI fails it shows a named declaration std::chrono::ambiguous_local_time which can be grepped here.

libcxx/src/CMakeLists.txt

329

There are other places in libc++ that use multiple macros to guard them. For example, fstream requires both LIBCXX_ENABLE_FILESYSTEM and LIBCXX_ENABLE_LOCALIZATION. I did the same here based on that precedence.

libcxx/src/tz.cpp

107

We use both ugly and non-uqly naming in our dylib. I prefer ugly; it makes it easy to move code from the dylib to header without the need for uglyfication.

140

I used that in the past and I was not happy with that code. Note at the moment only a version number is parsed. There is a lot more to be parsed in followup patches,

libcxx/src/tzdb_list.cpp

42

Based on the wording in http://eel.is/c++draft/time.zone.db.list#lib:begin,tzdb_list this matches the wording and its intend. Which part does not make sense to you?

72

The write access is only needed when this object is modified. In practice this should be very rare so read operations should be concurrent.

The main reason for writing is when reloading the TZDB, which requires a new database to be released and updated by the system administrator. New databases happen a handful of times per year, so it's only useful for long running applications.

I'll add a comment to explain this.

83

What do you mean?

libcxx/test/libcxx/diagnostics/chrono.nodiscard_extensions.compile.pass.cpp

25

I really don't understand what you mean. Note this test only tests the nodiscard extension nothing else. This is the same of other tests in this directory. There are different tests that test the behaviour of these funtions.

What is the problem with this test?

libcxx/test/libcxx/experimental/fexperimental-library.compile.pass.cpp

34

Can you elaborate what you mean. The TZDB feature is experimental and incomplete. There are several additional patches in the works to get somewhat useful behaviour.

libcxx/test/support/test_tzdb.h

23

Why the returned value is only used for parsing so a temporary is fine. Note that this is only used for testing purposes.

libcxx/utils/ci/buildkite-pipeline.yml

684

Since we intend to ship libc++ with TZDB disabled we should test it. The TZDB adds quite some code in its final incarnation. This is useful for embedded systems.

I agree we should look at how we want to do testing. What I would like is something GitLab offers. Make some tests manually. These are not executed by default, but you can start them manually when you want to test them. Next to that nightly builds will enable them automatically.

I think this would apply to several of our parts disabled tests. Most of them are very specific.
I hope we can look at that when Phabricator is retired and we can move our CI to GitHub actions.