utils: replace boost::date_time usage with c++20 std::chrono#30462
utils: replace boost::date_time usage with c++20 std::chrono#30462theuni wants to merge 2 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
This was noticed while working on #30434, which requires us to install |
Concept ACK on that. |
Taken from commit 8a932116795f4046ba8ed79ce33717c7331a1490 See documentation here: https://howardhinnant.github.io/date/date.html This is meant to be a full implementation of the c++20 additions to std::chrono. This functionality is only available in libstd++ >= 14, and libc++ >= 19, meaning that it will be years before we can require availability in the std namespace. Use of this lib will allow us to drop our current dependency on boost::date_time, as well as to begin using (at least) formatting with std::chrono.
…teTime Because the functions in date.h are intended to line up exactly with the c++20 implementation in the std namespace, attempt to use std::chrono if a conforming version is found. While testing, I noticed that libstdc++ breaks for very old timestamps, so guard against that and add an additional test.
|
🚧 At least one of the CI tasks failed. HintsMake sure to run all tests locally, according to the documentation. The failure may happen due to a number of reasons, for example:
Leave a comment here, if you need help tracking down a confusing failure. |
|
|
||
| // Guard against broken implementations which can't handle very old time | ||
| // values. Speficically, libstdc++ (as of v14) is broken at least on x86_64 | ||
| // with times before 1677-09-21T00:12:44Z . |
There was a problem hiding this comment.
Can you add more details, whether this is a crash/UB, or just wrong behavior? Is there a bug?
There was a problem hiding this comment.
Specifically the comparison against the epoch (0) fails. So, if the timestamp is old enough, this should fail but doesn't:
assert tp < epochYou can play around here: https://godbolt.org/z/G8r837oGf
I'll add more specific info. I should probably submit a bug report upstream as well.
There was a problem hiding this comment.
Ah, so it is UB/crash, according to ubsan:
/opt/compiler-explorer/gcc-14.1.0/include/c++/14.1.0/bits/chrono.h:227:38: runtime error: signed integer overflow: -9223372037 * 1000000000 cannot be represented in type 'long int'
There was a problem hiding this comment.
Would be good to add a fuzz target as well to https://github.com/google/oss-fuzz/tree/master/projects/libstdcpp
Can you give some examples where it would be useful? Right now it is only used in a single place in the wallet.
Is the boost header footprint larger than this header? Also, by adding this header the locale linter alerts, so I wonder if the new stuff changes the behavior for users, or if the linter just alerts about unused code. Finally, |
I was assuming that it'd be useful in other places where it's simply cumbersome to deal with these conversions like logging. Or as a simplification for
Heh. I modified #30434 to only install the headers required by boost::date_time (and its dependencies, recursively). The result: cory@nowhere:19:48:40:~/dev/bitcoin (cmake-boost-depends)
$ find depends/x86_64-pc-linux-gnu/include/boost/ -type f -name "*.hpp" | wc -l
4268I won't even bother adding up the lines of code, but I'm quite sure it's less than 8k :)
Hmm, will have a look at this.
I started down this rabbit hole intending to simply rewrite this function, but ended up afraid of introducing bugs due to the locale footguns. Maybe I'm over-complicating it? I think if you look at this as: "use c++20 functionality with a bridge for compilers which don't support it yet", this PR makes perfect sense. But given the size of that bridge, I agree it's maybe not worth it. I suppose I was also curious if anyone else was waiting on this functionality for other code. |
Happy to try myself, but https://en.cppreference.com/w/cpp/chrono/year_month_day (which can convert to system clock) is available, so using |
And if there were any issues that I am missing, commit fa72dcb would be wrong as well, no? |
|
@theuni Are you still working on this? My suggestion from #30462 (comment) should be easy to implement, unless I am missing something. Happy to pick that up, if this pull is up-for-grabs. |
|
@maflcko All yours :) |
Ok, it is only 18 lines of code, see #31391 |
5 participants
This would be a very straightforward and uncontroversial change if not for the sad state of std lib implementations which implement this functionality.
As of now,
std::chrono::parseand friends (c++20 additions) are only available as of gcc 14 and (unreleased) clang 19.Formatting in chrono is quite useful, so I don't think it makes sense to wait years until we require those compilers.
Instead of waiting around, this PR takes the sledgehammer approach of adding a fully-conformant implementation of the new c++20 chrono features (from the author of
std::chronohimself). More info available here: https://howardhinnant.github.io/date/date.htmlA convenience header adds the
chrono_parsenamespace, which uses the std lib if possible, anddate.hotherwise.This allows us to get rid of our last use of
boost::date_time, and we can begin using more modern c++20-isms as well.