[Bug libstdc++/109772] New: Memory layout optimization of std::chrono::hh_mm_ss is wrong

[Bug libstdc++/109772] New: Memory layout optimization of std::chrono::hh_mm_ss is wrong

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

            Bug ID: 109772
           Summary: Memory layout optimization of std::chrono::hh_mm_ss is
                    wrong
           Product: gcc
           Version: 13.1.0
            Status: UNCONFIRMED
          Keywords: ABI
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: redi at gcc dot gnu.org
  Target Milestone: ---

#include <chrono>
#include <iostream>

int main()
{
    std::chrono::hh_mm_ss hms(std::chrono::duration<int, std::ratio<1,
1024>>{1511});
    std::cout << hms.subseconds().count() << '\n';
}

This prints 460892079 but the correct answer is 4755859375

It's being truncated to uint32_t.

[Bug libstdc++/109772] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-05-08
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
   Target Milestone|---                         |13.2
     Ever confirmed|0                           |1

[Bug libstdc++/109772] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I had a brain fart with this constraint for choosing the subseconds
representation:

        // 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();

What matters is not the _Duration type, but the precision type. Given a
_Duration like duration<int64_t, ratio<1, 1024>> we can easily fit the maximum
fractional seconds value of 1023 in a 32-bit integer. But what we actually need
to store is not a count of 1023 [1/1024]s units, but 9990234375
[1/10000000000]s units. And 9990234375 doesn't fit in 32 bits.

[Bug libstdc++/109772] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #0)
> std::chrono::hh_mm_ss hms(std::chrono::duration<int, std::ratio<1, 1024>>{1511});

The bug affects this example because __fits<uint_least32_t> is true, because
duration_values<int>::max() <= duration_values<uint_least32_t>::max() is true.
So the library decides it can use uint_least32_t to store the subseconds part.
However, the value we need to store is duration<int64_t, ratio<1,
10'000'000'000>>(4755859375) not duration<int, ratio<1, 1024>(1511 % 1024) and
so we truncate 9990234375 to 32 bits.

The bug doesn't affect uses of hh_mm_ss with the standard duration aliases in
<chrono>, e.g. hh_mm_ss<nanoseconds>, because the standard chrono aliases with
higher precision than one second all use 64-bit rep types, so
__fits<uint_least32_t> is false. And in practice the number of users of
hh_mm_ss with durations with unusual periods and 32-bit rep types is probably
tiny. And hh_mm_ss should not be used for storage, only as a short-lived stack
variable ... so I think we can probably just fix this for 13.2 even though
that's theoretically an ABI break (the C++20 ABI isn't final yet, but we
usually try to keep it stable between minor releases like 13.1 and 13.2). We
can consider adding an abi_tag attribute to hh_mm_ss for 13.2 (but not on
trunk).

[Bug libstdc++/109772] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Or if problematic uses of hh_mm_ss are as rare as I expect, we could just not
fix it on the gcc-13 branch, only on trunk.

[Bug libstdc++/109772] [13/14 Regression] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
There's another problem which is that hh_mm_ss<duration<char, ratio<1, 11>>>
fails to compile:

/home/jwakely/gcc/13/include/c++/13.0.1/chrono: In instantiation of 'class
std::chrono::hh_mm_ss<std::chrono::duration<char, std::ratio<1, 11> > >':
hms.cc:12:63:   required from here
/home/jwakely/gcc/13/include/c++/13.0.1/chrono:2439:37: error: ambiguous
template instantiation for 'struct
std::chrono::hh_mm_ss<std::chrono::duration<char, std::ratio<1, 11> >
>::__subseconds<std::chrono::duration<long int, std::ratio<1, 1000000> > >'
 2439 |         __subseconds<precision>     _M_ss{};
      |                                     ^~~~~
/home/jwakely/gcc/13/include/c++/13.0.1/chrono:2412:18: note: candidates are:
'template<class _Duration> template<class _Rep, class _Period>  requires
!(treat_as_floating_point_v<_Rep>) && (ratio_less_v<_Period, std::ratio<1, 1>
>) && ((ratio_greater_equal_v<_Period, std::ratio<1, 250> >) ||
(__fits<unsigned char>)) struct
std::chrono::hh_mm_ss<_Duration>::__subseconds<std::chrono::duration<_Rep,
_Period> > [with _Rep = long int; _Period = std::ratio<1, 1000000>; _Duration =
std::chrono::duration<char, std::ratio<1, 11> >]'
 2412 |           struct __subseconds<duration<_Rep, _Period>>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/jwakely/gcc/13/include/c++/13.0.1/chrono:2426:18: note:                
'template<class _Duration> template<class _Rep, class _Period>  requires
!(treat_as_floating_point_v<_Rep>) && (ratio_less_v<_Period, std::ratio<1, 250>
>) && ((ratio_greater_equal_v<_Period, std::ratio<1, 4000000000> >) ||
(__fits<uint_least32_t>)) struct
std::chrono::__subseconds<std::chrono::duration<_Rep, _Period> > [with _Rep =
long int; _Period = std::ratio<1, 1000000>; _Duration =
std::chrono::duration<char, std::ratio<1, 11> >]'
 2426 |           struct __subseconds<duration<_Rep, _Period>>
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Bug libstdc++/109772] [13/14 Regression] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

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>>.

[Bug libstdc++/109772] [13 Regression] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.2                        |13.3

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 13.2 is being released, retargeting bugs to GCC 13.3.

[Bug libstdc++/109772] [13 Regression] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug libstdc++/109772] [13 Regression] Memory layout optimization of std::chrono::hh_mm_ss is wrong

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109772

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.3                        |13.4

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 13.3 is being released, retargeting bugs to GCC 13.4.