This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Mordante on Jul 1 2023, 7:15 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGf78f93bc9fd4: [libc++][chrono] Adds tzdb_list implementation.
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

Diff Detail

Event Timeline

Mordante created this revision.Jul 1 2023, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2023, 7:15 AM
Mordante added inline comments.Jul 1 2023, 7:18 AM
libcxx/CMakeLists.txt
325

This patch needs to be added to the release notes. However since this is the start of a rather large feature it makes sense to post-pone landing this after LLVM-17 has branched. To avoid merge conflicts the release notes aren't updated yet.

Mordante updated this revision to Diff 536582.Jul 2 2023, 5:09 AM

CI fixes.

Mordante updated this revision to Diff 536585.Jul 2 2023, 5:43 AM

CI fixes.

Mordante updated this revision to Diff 536607.Jul 2 2023, 9:27 AM

CI fixes.

Mordante updated this revision to Diff 536609.Jul 2 2023, 9:42 AM

CI fixes.

Mordante updated this revision to Diff 536610.Jul 2 2023, 9:55 AM

Retrigger CI.

Mordante updated this revision to Diff 536617.Jul 2 2023, 10:44 AM

Reenable full CI.

Mordante published this revision for review.Jul 2 2023, 12:34 PM
Mordante added inline comments.
libcxx/include/module.modulemap.in
322

Add @requires_LIBCXX_ENABLE_LOCALIZATION@ and filesystem here and next file.

libcxx/src/CMakeLists.txt
322

Review note.

This patch can avoid the dependency to the file system. However future patches will require the file system std::chrono::current_zone() on Unix-like systems needs to resolve the target of the symlink /etc/localtime which points to something like /usr/share/zoneinfo/Europe/Berlin. In this case Europe/Berlin is the name of the current time zone.

Localization could be avoided by using a C file I/O API. This might make sense when there are a lot of vendors with filesystem support, but without locale support.

Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2023, 12:34 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik added inline comments.
libcxx/docs/Status/Cxx20.rst
52

Maybe a status page would be appropriate?

libcxx/docs/UsingLibcxx.rst
564–567

Why do you make this a static? The string literal already has static storage duration.

libcxx/include/__chrono/tzdb.h
20

Including headers shouldn't depend on -fexperimental-library. Same for #pragma GCC system_header.

libcxx/include/__config
401

Why isn't the macro simply defined in the #ifdef above?

libcxx/src/tz.cpp
43

Why don't you put this in an anonymous namespace instead? Also, these functions and arguments don't have to be _Uglified, since they are only in the sources. I would also move this into the global namespace to avoid confusion.

125–141

I would keep all the annotations exclusively in the headers. That makes this code much nicer to read.

Mordante marked 5 inline comments as done.Aug 12 2023, 5:53 AM
Mordante added inline comments.
libcxx/docs/Status/Cxx20.rst
52

I think keeping the note would be better. I expect most of the missing parts done this cycle. The Stream output remark is permanent. That part of the paper will never be done.

libcxx/docs/UsingLibcxx.rst
564–567

I've added a comment to explain the requirement.

libcxx/include/__chrono/tzdb.h
20

This is how we have done this in the past for format too.

libcxx/src/tz.cpp
43

I prefer static over anonymous namespaces. We have other parts of the dylib uglified too. This makes it easier to move code to the header when possible.

125–141

I prefer to keep the declaration and definition the same.

Mordante updated this revision to Diff 549601.Aug 12 2023, 5:53 AM
Mordante marked 4 inline comments as done.

Rebased and addresses review comments.

ldionne added inline comments.
libcxx/CMakeLists.txt
325

I think we should remove the ability to set a custom path and always use the system defaults. We should work with the various platforms we support to make sure that they provide these resources, and if they don't then it would be reasonable for that part of functionality not to be supported at all.

332

However, we should add something like LIBCXX_ENABLE_TIME_ZONE_SUPPORT, which would act like LIBCXX_ENABLE_FILESYSTEM and others. I would expect e.g. embedded systems to turn that off, but it should be on by default.

libcxx/docs/BuildingLibcxx.rst
25

I don't think this documentation is required anymore if we assume platform support when LIBCXX_ENABLE_TIME_ZONE_SUPPORT=ON.

307

This goes too.

libcxx/docs/DesignDocs/TimeZone.rst
20
46–49
51–54
56–61
65–66
81
86

This will go away.

92

Point of discussion: do we really want to let users override this? Unless we have clear uses cases for that, I would recommend not exposing this to users at first. We can do something ad-hoc for our testing needs.

If we decide not to expose this to users, let's make it clear by not saying "this allows applications to specify their own database path [...]".

115
116–119
128–131
132–136
144–146
libcxx/docs/UsingLibcxx.rst
533

This should be updated.

591
596–597
Mordante marked 21 inline comments as done.Aug 18 2023, 8:55 AM
Mordante added inline comments.
libcxx/docs/DesignDocs/TimeZone.rst
92

I'll remove the part of user specific applications.

Mordante updated this revision to Diff 551535.Aug 18 2023, 8:55 AM
Mordante marked an inline comment as done.

Addresses review comments.

Mordante updated this revision to Diff 551747.Aug 19 2023, 6:53 AM

Rebased and CI fixes.

ldionne added inline comments.Aug 29 2023, 9:59 AM
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 marked 13 inline comments as done.Aug 30 2023, 11:27 AM
Mordante added inline comments.
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 updated this revision to Diff 554791.Aug 30 2023, 11:27 AM

Rebased and addresses review comments.

Mordante updated this revision to Diff 555055.Aug 31 2023, 8:09 AM

CI fixes.

ldionne accepted this revision.Sep 5 2023, 8:32 AM

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.

This revision is now accepted and ready to land.Sep 5 2023, 8:32 AM
Mordante marked 5 inline comments as done.Sep 5 2023, 10:49 AM

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 updated this revision to Diff 555902.Sep 5 2023, 10:56 AM
Mordante marked an inline comment as done.

Rebased and addresses review comments.

Mordante updated this revision to Diff 555907.Sep 5 2023, 11:32 AM

Retrigger CI.

Mordante updated this revision to Diff 556041.Sep 6 2023, 8:06 AM

CI fixes.

This revision was landed with ongoing or failed builds.Sep 6 2023, 11:48 AM
This revision was automatically updated to reflect the committed changes.
EricWF reopened this revision.Sep 6 2023, 1:25 PM
EricWF added a subscriber: EricWF.

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

libcxx/include/__chrono/tzdb.h
2

Why is this its own header?

libcxx/include/__chrono/tzdb_list.h
31

This conjunct deserves its own macro.

38

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

46

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.

50

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

Please remove it.

55

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
328

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
78

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

108

__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.

128

Should this be nodestroy?

128

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

141

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
43

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).

73

Why is this a shared_mutex?

84

This should never be called.

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

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
13

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

libcxx/test/support/test_tzdb.h
24

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
461

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.

This revision is now accepted and ready to land.Sep 6 2023, 1:25 PM

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?

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 '#'

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

@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 marked 5 inline comments as done.Sep 7 2023, 12:07 PM

@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
2

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
328

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
108

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.

141

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
43

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?

73

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.

84

What do you mean?

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

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
24

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.