[gcc r14-740] libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772]

[gcc r14-740] libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772]

[Jonathan Wakely]

https://gcc.gnu.org/g:aa39ed4467db0cf18f300fcf475e09c4496527cf

commit r14-740-gaa39ed4467db0cf18f300fcf475e09c4496527cf
Author: Jonathan Wakely <jwakely@redhat.com>
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<typename _Tp>
             static constexpr bool __fits
               = duration_cast<precision>(_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<duration<int, std::pico>>, 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<duration<int, std::pico>> and
    hh_mm_ss<duration<int, ratio<1, 1024>> are affected (increasing from 16
    bytes to 24 on x86_64), but hh_mm_ss<duration<int, ratio<1, 1000>> and
    hh_mm_ss<duration<long, ratio<1, 1024>> 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<duration<int, std::pico>>.

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<typename _Tp>
-	  static constexpr bool __fits
-	    = duration_values<typename _Duration::rep>::max()
-		<= duration_values<_Tp>::max();
-
 	template<typename _Rep, typename _Period>
 	  requires (!treat_as_floating_point_v<_Rep>)
 	    && ratio_less_v<_Period, ratio<1, 1>>
-	    && (ratio_greater_equal_v<_Period, ratio<1, 250>>
-		  || __fits<unsigned char>)
+	    && ratio_greater_equal_v<_Period, ratio<1, 250>>
 	  struct __subseconds<duration<_Rep, _Period>>
 	  {
 	    unsigned char _M_r{};
@@ -2421,8 +2414,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	template<typename _Rep, typename _Period>
 	  requires (!treat_as_floating_point_v<_Rep>)
 	    && ratio_less_v<_Period, ratio<1, 250>>
-	    && (ratio_greater_equal_v<_Period, ratio<1, 4000000000>>
-		  || __fits<uint_least32_t>)
+	    && ratio_greater_equal_v<_Period, ratio<1, 4000000000>>
 	  struct __subseconds<duration<_Rep, _Period>>
 	  {
 	    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<duration<int, std::centi>>) == sizeof(S1));
   struct S2 { long long h; char m, s; bool neg; int ss; };
   static_assert(sizeof(hh_mm_ss<duration<int, std::milli>>) == sizeof(S2));
-  static_assert(sizeof(hh_mm_ss<duration<int, std::pico>>) == sizeof(S2));
   struct S3 { long long h; char m, s; bool neg; long long ss; };
+  static_assert(sizeof(hh_mm_ss<duration<int, std::pico>>) == sizeof(S3));
   static_assert(sizeof(hh_mm_ss<duration<long long, std::pico>>) == sizeof(S3));
   struct S4 { long long h; char m, s; bool neg; double ss; };
   static_assert(sizeof(hh_mm_ss<duration<double, std::micro>>) == 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 <chrono>
+#include <testsuite_hooks.h>
+
+using std::chrono::hh_mm_ss;
+using std::chrono::duration;
+using std::ratio;
+
+constexpr bool
+test_truncated()
+{
+  using int_1_1024 = duration<int, ratio<1, 1024>>;
+  static_assert( hh_mm_ss<int_1_1024>::fractional_width == 10 );
+
+  hh_mm_ss<int_1_1024> t1(int_1_1024(487));
+  VERIFY( t1.subseconds().count() == (10'000'000'000 * 487 / 1024) );
+
+  hh_mm_ss<int_1_1024> 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<duration<char, ratio<1, 11>>>::fractional_width == 6 );