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