From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2181) id B3ED93857347; Thu, 11 May 2023 20:19:16 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B3ED93857347 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1683836356; bh=lVSKSBL5k4S5jXr9h2+psNBmKCAiz1jyKpG0v5cdgnw=; h=From:To:Subject:Date:From; b=hst3qdmdNB65Su2qEz0GN5+1gpgHLZmmZuIJ/WB63OhK0L14W2qLQL9I4/qTHBC7G dHFh2eHjOcTT+7Qcs5AwX4PiEMeTh91siA/lJHOn6ORUVZpu+6VEyK9CsykBhEqGNg 5tgMK0mNqv4X0L8PYOu22sy1S88glJ4pzlirpsWg= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jonathan Wakely To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r14-740] libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772] X-Act-Checkin: gcc X-Git-Author: Jonathan Wakely X-Git-Refname: refs/heads/master X-Git-Oldrev: c62e945492afbbd2a09896fc7b0b07f7e719a606 X-Git-Newrev: aa39ed4467db0cf18f300fcf475e09c4496527cf Message-Id: <20230511201916.B3ED93857347@sourceware.org> Date: Thu, 11 May 2023 20:19:16 +0000 (GMT) List-Id: https://gcc.gnu.org/g:aa39ed4467db0cf18f300fcf475e09c4496527cf commit r14-740-gaa39ed4467db0cf18f300fcf475e09c4496527cf Author: Jonathan Wakely Date: Wed May 10 16:15:03 2023 +0100 libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772] I borked the logic in r13-4526-g5329e1a8e1480d so that the selected partial specialization of hh_mm_ss::__subseconds might not be able to represent the correct number of subseconds. This can result in a truncated value being stored for the subseconds, e.g., 4755859375 gets truncated to 460892079 because the correct value doesn't fit in uint_least32_t. Instead of checking whether the maximum value of the incoming duration type can be represented, we would need to check whether that maximum value can be represented after being converted to the correct precision type: template static constexpr bool __fits = duration_cast(_Duration::max()).count() <= duration_values<_Tp>::max(); However, this can fail to compile, due to integer overflow in the constexpr multiplications. Instead, we could limit the check to the case where the incoming duration has the same period as the precision, where no conversion is needed and so no overflow can happen. But that seems of very limited value, as it would only benefit specializations like hh_mm_ss>, which can only represent a time-of-day between -00:00:00.0215 and +00:00:00.0215 measured in picoseconds! Additionally, the hh_mm_ss::__subseconds partial specializations do not have disjoint constraints, so that some hh_mm_ss specializations result in ambiguities tying to match a __subseconds partial specialization. The most practical fix is to just stop using the __fits variable template in the constraints of the partial specializations. This fixes the truncated values by not selecting an inappropriate partial specialization, and fixes the ambiguous match by ensuring the constraints are disjoint. Fixing this changes the layout of some specializations, so is an ABI change. It only affects specializations that have a small (less than 64-bit) representation type and either a very small period (e.g. like the picosecond specialization above) or a non-power-of-ten period like ratio<1, 1024>. For example both hh_mm_ss> and hh_mm_ss> are affected (increasing from 16 bytes to 24 on x86_64), but hh_mm_ss> and hh_mm_ss> are not affected. libstdc++-v3/ChangeLog: PR libstdc++/109772 * include/std/chrono (hh_mm_ss::__fits): Remove variable template. (hh_mm_ss::__subseconds): Remove __fits from constraints. * testsuite/std/time/hh_mm_ss/109772.cc: New test. * testsuite/std/time/hh_mm_ss/1.cc: Adjust expected size for hh_mm_ss>. Diff: --- libstdc++-v3/include/std/chrono | 12 ++------- libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc | 2 +- libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc | 31 ++++++++++++++++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/libstdc++-v3/include/std/chrono b/libstdc++-v3/include/std/chrono index 7bfc9b79acf..660e8d2b746 100644 --- a/libstdc++-v3/include/std/chrono +++ b/libstdc++-v3/include/std/chrono @@ -2398,17 +2398,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return {}; } }; - // True if the maximum constructor argument can be represented in _Tp. - template - static constexpr bool __fits - = duration_values::max() - <= duration_values<_Tp>::max(); - template requires (!treat_as_floating_point_v<_Rep>) && ratio_less_v<_Period, ratio<1, 1>> - && (ratio_greater_equal_v<_Period, ratio<1, 250>> - || __fits) + && ratio_greater_equal_v<_Period, ratio<1, 250>> struct __subseconds> { unsigned char _M_r{}; @@ -2421,8 +2414,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template requires (!treat_as_floating_point_v<_Rep>) && ratio_less_v<_Period, ratio<1, 250>> - && (ratio_greater_equal_v<_Period, ratio<1, 4000000000>> - || __fits) + && ratio_greater_equal_v<_Period, ratio<1, 4000000000>> struct __subseconds> { uint_least32_t _M_r{}; diff --git a/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc b/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc index f8a3e115af3..85f991c5e03 100644 --- a/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc +++ b/libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc @@ -109,8 +109,8 @@ size() static_assert(sizeof(hh_mm_ss>) == sizeof(S1)); struct S2 { long long h; char m, s; bool neg; int ss; }; static_assert(sizeof(hh_mm_ss>) == sizeof(S2)); - static_assert(sizeof(hh_mm_ss>) == sizeof(S2)); struct S3 { long long h; char m, s; bool neg; long long ss; }; + static_assert(sizeof(hh_mm_ss>) == sizeof(S3)); static_assert(sizeof(hh_mm_ss>) == sizeof(S3)); struct S4 { long long h; char m, s; bool neg; double ss; }; static_assert(sizeof(hh_mm_ss>) == sizeof(S4)); diff --git a/libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc b/libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc new file mode 100644 index 00000000000..36137f283d2 --- /dev/null +++ b/libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc @@ -0,0 +1,31 @@ +// { dg-options "-std=gnu++20" } +// { dg-do compile { target c++20 } } + +// PR libstdc++/109772 Memory layout optimization of chrono::hh_mm_ss is wrong + +#include +#include + +using std::chrono::hh_mm_ss; +using std::chrono::duration; +using std::ratio; + +constexpr bool +test_truncated() +{ + using int_1_1024 = duration>; + static_assert( hh_mm_ss::fractional_width == 10 ); + + hh_mm_ss t1(int_1_1024(487)); + VERIFY( t1.subseconds().count() == (10'000'000'000 * 487 / 1024) ); + + hh_mm_ss t2(int_1_1024(1023)); + VERIFY( t2.subseconds().count() == (10'000'000'000 * 1023 / 1024) ); + + return true; +} + +static_assert( test_truncated() ); + +// Check for ambiguous partial specializations of hh_mm_ss::__subseconds: +static_assert( hh_mm_ss>>::fractional_width == 6 );