[PATCH] libstdc++: Remove dependency on std::atomic<unsigned> in tzdb.cc

[Tomasz Kaminski]

On Thu, Jun 25, 2026 at 2:24 PM Tomasz Kaminski <tkaminsk@redhat.com> wrote:

>
>
> On Thu, Jun 25, 2026 at 2:17 PM Jonathan Wakely <jwakely@redhat.com>
> wrote:
>
>> On Thu, 25 Jun 2026 at 13:08, Jonathan Wakely <jwakely@redhat.com> wrote:
>> >
>> > On Thu, 25 Jun 2026 at 13:01, Tomasz Kaminski <tkaminsk@redhat.com>
>> wrote:
>> > >
>> > >
>> > >
>> > > On Thu, Jun 25, 2026 at 1:42 PM Jonathan Wakely <jwakely@redhat.com>
>> wrote:
>> > >>
>> > >> My r17-471-ge79f0f818c0e42 change to optimize handling of leap
>> seconds
>> > >> introduced a hard dependency on std::atomic<unsigned>, which causes
>> > >> problems for targets without atomic word operations, like Cortex-M0.:
>> > >> https://gcc.gnu.org/pipermail/gcc-patches/2026-June/719704.html
>> > >>
>> > >> This patch replaces the num_leap_seconds variable with a struct which
>> > >> decides whether to use std::atomic_ref<unsigned> or perform all
>> accesses
>> > >> while holding a lock on the pre-existing mutex used for the tzdb_list
>> > >> singleton.
>> > >>
>> > >> The workaround is a bit ugly and fragile, because it assumes that
>> there
>> > >> is only one caller of num_leap_seconds.set and that the list_mutex()
>> is
>> > >> locked by that caller iff the tzdb_list doesn't use
>> atomic<shared_ptr<>>
>> > >> (which is enforced via preprocessor checks).
>> > >
>> > > We could have two different methods, set_under_lock and set_atomic
>> > > and call them in branches:
>> >
>> > I considered that, but it means duplicating the _S_cache_list_head
>> > call, because ...
>> >
>> >
>> > > #if USE_ATOMIC_SHARED_PTR
>> > >     new_head_ptr->next = curr;
>> > >     while (!_S_head_owner.compare_exchange_strong(curr, new_head))
>> > >       {
>> > >         if (curr->db.version == new_head_ptr->db.version)
>> > >           return curr->db;
>> > >         new_head_ptr->next = curr;
>> > >       }
>> > >     // XXX small window here where _S_head_cache still points to
>> previous tzdb.
>> > >      _S_cache_list_head(new_head_ptr); <-- This is empty if
>> USE_ATOMIC_SHARED_PTR is false,
>> > >
>>   so do not define it.
>> >
>> > Strictly speaking, it's empty for ! USE_ATOMIC_LIST_HEAD and in theory
>> > the code is designed to support the case where we have
>> > USE_ATOMIC_LIST_HEAD and ! USE_ATOMIC_SHARED_PTR. For example, if
>> > benchmarking shows that the mutex performs better than the
>> > atomic<shared_ptr<>>, maybe only on particular platforms.
>> >
>> > So it's true that _S_cache_list_head is empty for !
>> > USE_ATOMIC_SHARED_PTR *today* but the design doesn't require that.
>> >
>> >
>> >
>> > >     set_lockfree(....)
>> > > #else
>> > >     lock_guard<mutex> l(list_mutex());
>> > >     if (const _Node* h = _S_head_owner.get())
>> > >       {
>> > >         if (h->db.version == new_head_ptr->db.version)
>> > >           return h->db;
>> > >         new_head_ptr->next = _S_head_owner;
>> > >       }
>> > >     _S_head_owner = std::move(new_head);
>> > >     set_atomic(....)
>> > > #endif
>> > > I think I would preffer that.
>>
>>
>> How about doing this instead:
>>
> I am not convinced that if we ever add a non-empty _S_head_cache, it will
> not be moved
> inside block. If we use mutex (instead of atomic<shared_ptr>) we will need
> to deal with the
> the fact that a different mutex is already taken, thus avoiding deadlock.
>
> The suggested change has exactly the same number of lines, as duplicating
> _S_head_cache
> would cause, and it simply "smarter" (not in positive way) than
> alternative of using different names.
>
But I can also live with it, passing the "lock" object helps to indicate
that the function requires to be called
under it.

> @@ -1583,6 +1583,7 @@ constinit tzdb_list::_Node::NumLeapSeconds
>> tzdb_list::_Node::num_leap_seconds;
>>   {
>>     _Node* new_head_ptr = new_head.get();
>> #if USE_ATOMIC_SHARED_PTR
>> +    const int lock = 0; // dummy variable to pass to
>> num_leap_seconds.set.
>>     new_head_ptr->next = curr;
>>     while (!_S_head_owner.compare_exchange_strong(curr, new_head))
>>       {
>> @@ -1592,7 +1593,7 @@ constinit tzdb_list::_Node::NumLeapSeconds
>> tzdb_list::_Node::num_leap_seconds;
>>       }
>>     // XXX small window here where _S_head_cache still points to previous
>> tzdb.
>> #else
>> -    lock_guard<mutex> l(list_mutex());
>> +    lock_guard<mutex> lock(list_mutex());
>>     if (const _Node* h = _S_head_owner.get())
>>       {
>>        if (h->db.version == new_head_ptr->db.version)
>> @@ -1605,7 +1606,7 @@ constinit tzdb_list::_Node::NumLeapSeconds
>> tzdb_list::_Node::num_leap_seconds;
>>
>>     // This allows __recent_leap_second_info() to know that it can use
>>     // get_tzdb_list()->begin()->leap_seconds to get new leap seconds.
>> -    num_leap_seconds.set(new_head_ptr->db.leap_seconds.size());
>> +    num_leap_seconds.set(new_head_ptr->db.leap_seconds.size(), lock);
>>
>>     return new_head_ptr->db;
>>   }
>>
>>
>> And then define the set member like this:
>>
>>   // Called by _Node::_S_replace_head
>> #if ATOMIC_INT_LOCK_FREE == 2
>>   void
>>   set(unsigned val, int)
>>   {
>>     atomic_ref<unsigned> ref(count);
>>     // The release op here synchronizes with the acquire op in get().
>>     ref.store(val, memory_order::release);
>>   }
>> #else
>>   void
>>   set(unsigned val, const lock_guard<mutex>&)
>>   {
>>     // XXX The only caller of this function locks list_mutex() so we would
>>     // deadlock if we locked it again here.
>>     count = val;
>>   }
>> #endif
>>
>>
>> If something ever gets out of sync and we don't have a lock_guard in
>> scope when trying to call the non-atomic form, it won't compile.
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://gcc.gnu.org/pipermail/libstdc++/attachments/20260625/f2207ee9/attachment-0001.htm>