fix odr violation for std::format with single argument std::chrono::duration #25135
+52
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have discovered an ODR violation which causes a ducktape test failure
when running them with a bazel build but not with a cmake build.
Test I tried:
`tests/rptest/tests/cluster_config_test.py::ClusterConfigTest.test_valid_settings`
Errors I get:
`Invalid cpu_profiler_sample_period_ms: '"100ms"' (YAML::TypedBadConversion<std::__2::chrono::duration<long long, std::__2::ratio<1l, 1000l>>> (yaml-cpp: error at line 1, column 1: bad conversion))`
With bazel build the property is rendered as "100ms" while with cmake
build as "100".
Under cmake, the formatting is done by the ostream insertion operator
overload in `src/v/utils/to_string.h`:
```cpp
template<typename Rep, typename Period>
inline std::ostream&
operator<<(std::ostream& o, const std::chrono::duration<Rep, Period>& d) {
fmt::print(
o,
"{}",
std::chrono::duration_cast<std::chrono::milliseconds>(d).count());
return o;
}
```
Under bazel build, formatting is handled by formatter specializations of
libfmt "fmt/chrono.h".
The behavior is influenced by the linking order. Godbolt repro
https://godbolt.org/z/EbPEx7n58
In a later commit we'll get rid of the ODR violation but before that we
need to ensure example formatting stays as it was in the binaries we
are currently shipping with cmake to avoid breaking changes and to fix
ducktape tests.
Fortunately, bounded property is used only with a reduced number of
types so we can add handling for them in a relatively simple way.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Redpanda for a long time had it's own formatting for std::chrono::millisecond via ostream insertion operator overload which fmtlib used. Now fmt ships with its own implementation for formatting chrono types (fmt/chrono.h) and end behavior depends on linking order. In this test we either hard code the strings or call count() on duration to get a numeric value the formatting of which I don't expect to change.
See previous commits in the series for details.
1. This results in ODR violation for fmt since through multiple layers
of template instantiation it ends up with 2 implementation for
printing std::chrono::duration. One is this with implicit
fmt/ostream.h support and the other is provided by fmt/chrono.h.
2. The implementation is garbage. It always prints any durations as
milliseconds.
nvartolomei
force-pushed
the
nv/remove-ostream-ins-chrono-duration
branch
from
13ec539 to
c8ed64d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backports Required
Release Notes