On Tue, Jun 3, 2025 at 7:53 PM Jonathan Wakely <jwakely....@gmail.com>
wrote:
>
>
> On Tue, 3 Jun 2025, 16:07 Tomasz Kaminski, <tkami...@redhat.com> wrote:
>
>>
>>
>> On Tue, Jun 3, 2025 at 4:40 PM Jonathan Wakely <jwak...@redhat.com>
>> wrote:
>>
>>> On Tue, 3 Jun 2025 at 14:46, Jonathan Wakely <jwak...@redhat.com> wrote:
>>> >
>>> > For some 32-bit targets Glibc supports changing the size of time_t to
>>> be
>>> > 64 bits by defining _TIME_BITS=64. That causes an ABI change which
>>> > would affect std::chrono::system_clock::to_time_t. Because to_time_t is
>>> > not a function template, its mangled name does not depend on the return
>>> > type, so it has the same mangled name whether it returns a 32-bit
>>> time_t
>>> > or a 64-bit time_t. On targets where the size of time_t can be selected
>>> > at preprocessing time, that can cause ODR violations, e.g. the linker
>>> > selects a definition of to_time_t that returns a 32-bit value but a
>>> > caller expects 64-bit and so reads 32 bits of garbage from the stack.
>>> >
>>> > This commit adds always_inline to to_time_t when time_t has been
>>> changed
>>> > from a 32-bit type to a 64-bit type by defining _TIME_BITS=64. This
>>> > ensures that callers expecting a 64-bit time_t can't link to a
>>> > definition returning a 32-bit time_t.
>>> >
>>> > We use the internal Glibc macro __USE_TIME64_REDIRECTS to detect the
>>> > case where time_t defaults to 32-bit for the target but has been
>>> > explicitly changed to 64-bit by the user.
>>> >
>>> > libstdc++-v3/ChangeLog:
>>> >
>>> > PR libstdc++/99832
>>> > * include/bits/chrono.h (system_clock::to_time_t): Add always
>>> > inline_attribute for 64-bit time_t on 32-bit target.
>>> > * testsuite/20_util/system_clock/time64.cc: New test.
>>> > ---
>>> >
>>> > Tested x86_64-linux (-m64 and -m32, with recent glibc).
>>> >
>>> > libstdc++-v3/include/bits/chrono.h | 3 +++
>>> > .../testsuite/20_util/system_clock/time64.cc | 21 +++++++++++++++++++
>>> > 2 files changed, 24 insertions(+)
>>> > create mode 100644
>>> libstdc++-v3/testsuite/20_util/system_clock/time64.cc
>>> >
>>> > diff --git a/libstdc++-v3/include/bits/chrono.h
>>> b/libstdc++-v3/include/bits/chrono.h
>>> > index fad216203d2f..5c6ee759b381 100644
>>> > --- a/libstdc++-v3/include/bits/chrono.h
>>> > +++ b/libstdc++-v3/include/bits/chrono.h
>>> > @@ -1244,6 +1244,9 @@ _GLIBCXX_BEGIN_INLINE_ABI_NAMESPACE(_V2)
>>> > now() noexcept;
>>> >
>>> > // Map to C API
>>> > +#ifdef __USE_TIME64_REDIRECTS
>>>
>>> Florian suggested not relying on this internal glibc macro (which was
>>> not present in the first versions of glibc to support _TIME_BITS=64).
>>>
>>> We can just make the always_inline attribute unconditional, which does
>>> no harm. It's a tiny function that just extracts an integer from the
>>> time and does an integer division to convert nanoseconds to seconds,
>>> so always_inline is appropriate. And this way no targets will get a
>>> dependency on any to_time_t symbol, with any mangling or any return
>>> type.
>>>
>>> Existing objects which were already compiled before the attribute was
>>> added will still work, because the function is inline so those objects
>>> will already have a COMDAT definition of the symbol (or will have
>>> inlined it anyway).
>>>
>>> The only case that can't work is linking together existing objects
>>> which were compiled with and without -D_TIME_BITS=64 before libstdc++
>>> knew how to support that macro, but we can't fix those, the objects
>>> compiled with -D_TIME_BITS=64 might need to be recompiled.
>>>
>> Does from_time_t have the same problem, where arguments have different
>> width?
>>
>
> No, the mangled name always depends on parameters types (except for extern
> "C" functions).
>
> So when time_t is long that function generates a symbol with a different
> name to the case where time_t is long long.
>
Ah thanks. But I think think I would make sense for to_time_t and
from_time_t to be always inline. They implementation are symmetric, and
equally smal..
>
>
>
>
>>
>>> > + [[__gnu__::__always_inline__]]
>>> > +#endif
>>> > static std::time_t
>>> > to_time_t(const time_point& __t) noexcept
>>> > {
>>> > diff --git a/libstdc++-v3/testsuite/20_util/system_clock/time64.cc
>>> b/libstdc++-v3/testsuite/20_util/system_clock/time64.cc
>>> > new file mode 100644
>>> > index 000000000000..3cbf80e0f06e
>>> > --- /dev/null
>>> > +++ b/libstdc++-v3/testsuite/20_util/system_clock/time64.cc
>>> > @@ -0,0 +1,21 @@
>>> > +// { dg-options "-D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -O0 -g0" }
>>> > +// { dg-do compile { target *-*-linux-gnu } }
>>> > +// { dg-require-effective-target c++20 }
>>> > +// { dg-require-effective-target ilp32 }
>>> > +// { dg-final { scan-assembler-not "system_clock9to_time_t" } }
>>> > +
>>> > +#include <chrono>
>>> > +
>>> > +template<typename T>
>>> > +std::time_t
>>> > +test()
>>> > +{
>>> > + using std::chrono::system_clock;
>>> > +
>>> > + if constexpr (sizeof(T) == 8)
>>> > + return system_clock::to_time_t(system_clock::now());
>>> > + else // _TIME_BITS=64 had no effect, maybe an old Glibc
>>> > + return 0;
>>> > +}
>>> > +
>>> > +auto t = test<std::time_t>();
>>> > --
>>> > 2.49.0
>>> >
>>>
>>>
