On Thu, 25 Jun 2026 at 13:38, Tomasz Kaminski <[email protected]> wrote:
>
>
>
> On Thu, Jun 25, 2026 at 2:24 PM Tomasz Kaminski <[email protected]> wrote:
>>
>>
>>
>> On Thu, Jun 25, 2026 at 2:17 PM Jonathan Wakely <[email protected]> wrote:
>>>
>>> On Thu, 25 Jun 2026 at 13:08, Jonathan Wakely <[email protected]> wrote:
>>> >
>>> > On Thu, 25 Jun 2026 at 13:01, Tomasz Kaminski <[email protected]> wrote:
>>> > >
>>> > >
>>> > >
>>> > > On Thu, Jun 25, 2026 at 1:42 PM Jonathan Wakely <[email protected]>
>>> > > 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.
Yes, I like that idiom for "must be called locked". But that idiom
could still be used with your "call_locked" function. I'll implement
that and see how I like 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.
>>>
