Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

<chrono>: Fix formatting for unusual durations #3649

Merged
merged 11 commits into from
Apr 28, 2023

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Apr 11, 2023

Fixes #3644. hh_mm_ss<_Duration> seems wrong for _Hh_mm_ss_part_underflow_to_zero to me.

Need to explore how to accept duration<I, ratio<9223372036854775806, 9223372036854775807>>. Currently libstdc++ rejects it, while libc++ accepts it (Godbolt link).

Edit: the current approach for calculation of hh_mm_ss<D>::fractional_width doesn't seem right. I'm trying to fix it.

Edit 2: now the formatting is made well-formed, but UB may happen.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner April 11, 2023 04:16
@StephanTLavavej StephanTLavavej added bug Something isn't working format C++20/23 format chrono C++20 chrono labels Apr 11, 2023
@StephanTLavavej

This comment was marked as resolved.

@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Apr 12, 2023

Currently no implementation correctly formats duration<int64_t, ratio<INT64_MAX -1, INT64_MAX>>{40} (Godbolt link). In MSVC STL, libc++, and libstdc++, hh_mm_ss is used when formatting duration and thus duration_cast is also involved. And it's extremely easy for duration_cast to cause UB when the source type is duration<int64_t, ratio<INT64_MAX -1, INT64_MAX>>.

However, it seems that [time.format] doesn't say duration_cast or hh_mm_ss is equivalently used when formatting duration, so perhaps the current standard wording is requiring correctly formatting such duration...

stl/inc/chrono Show resolved Hide resolved
stl/inc/chrono Show resolved Hide resolved
@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Apr 12, 2023

Wait, actually, I'm trying to understand the UB here... could you write up an example?

@strega-nil-ms strega-nil-ms self-requested a review April 12, 2023 19:37
@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Apr 13, 2023

Wait, actually, I'm trying to understand the UB here... could you write up an example?

I attempted to test formatting duration<int, ratio<INT64_MAX - 1, INT64_MAX>>{40}. The duration needs to undergo _Hh_mm_ss_part_underflow_to_zero, and thus _Remove_duration_part. Finally duration_cast<days>(...) needs to be called, and signed integer overflow is unavoidable.

Reduced example that contains needed runtime operations and fails to compile due to integer overflow (Godbolt link):

#include <chrono>
#include <cstdint>
#include <limits>
#include <ratio>
#include <type_traits>

using LongRatio = std::ratio<INT64_MAX - 1, INT64_MAX>;
using D = std::chrono::duration<int, LongRatio>;

constexpr auto weird40 = D{40};
constexpr auto cast_to_day = std::chrono::duration_cast<std::chrono::days>(weird40);

hh_mm_ss conversion doesn't work either (Godbolt link).

@strega-nil-ms

This comment was marked as resolved.

@frederick-vs-ja

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member

Thanks! I pushed a conflict-free merge with main and trivial header inclusions for the test.

@StephanTLavavej StephanTLavavej removed their assignment Apr 25, 2023
@StephanTLavavej StephanTLavavej self-assigned this Apr 27, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

We forgot to defend _STD max against macroization - pushed.

@StephanTLavavej StephanTLavavej merged commit bb1798f into microsoft:main Apr 28, 2023
@StephanTLavavej
Copy link
Member

Thanks for investigating and fixing this bug! ⌚ 🎉 😸

@frederick-vs-ja frederick-vs-ja deleted the duration-fmt-fix branch April 28, 2023 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chrono C++20 chrono format C++20/23 format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<chrono>: Some (weird) durations still cannot be formatted
3 participants
  翻译: