[committed] libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772]

[committed] libstdc++: Fix chrono::hh_mm_ss::subseconds() [PR109772]

[Jonathan Wakely]

Tested powerpc64le-linux. Pushed to trunk.

This is a regression on gcc-13 too, but I'm undecided about the ABI
change for the branch. Generally that would be a no-go, but the affected
specializations are probably so rare that it would be OK. And we
definitely want to fix the ambiguity on the branch anyway.

-- >8 --

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>>.
---
 libstdc++-v3/include/std/chrono               | 12 ++-----
 libstdc++-v3/testsuite/std/time/hh_mm_ss/1.cc |  2 +-
 .../testsuite/std/time/hh_mm_ss/109772.cc     | 31 +++++++++++++++++++
 3 files changed, 34 insertions(+), 11 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/std/time/hh_mm_ss/109772.cc

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 );
-- 
2.40.1