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