[libcxx-commits] [libcxx] [libc++] P0718R2: Implementation of std::atomic<shared_ptr<T>> and std::atomic<weak_ptr<T>> (PR #194215)

[Vladislav Semykin via libcxx-commits]

================
@@ -0,0 +1,39 @@
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___MEMORY_ATOMIC_SHARED_PTR_H
+#define _LIBCPP___MEMORY_ATOMIC_SHARED_PTR_H
+
+#include <__config>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#  pragma GCC system_header
+#endif
+
+// _LIBCPP_FORCE_LOCK_BASED_ATOMIC_SHARED_PTR selects the stolen-bit spinlock
+// even on DWCAS targets. Local A/B-benchmark hook for
+// libcxx/test/benchmarks/atomic_shared_ptr_lock_based.bench.cpp; not a
+// supported public configuration. Must be consistent across the whole program.
+#if defined(_LIBCPP_FORCE_LOCK_BASED_ATOMIC_SHARED_PTR)
+#  define _LIBCPP_HAS_LOCKFREE_ATOMIC_SHARED_PTR 0
+#elif _LIBCPP_HAS_THREADS &&                                                                                           \
----------------
ViNN280801 wrote:

To be honest I wouldn't have even thought of that, thank you so much for your very valuable comments and for making me think more broadly, it's very useful and very cool experience. It took me a really long time to research before answering, sorry for this delay.

The current dispatch picks between two protocol-incompatible implementations per-TU, not two lowerings of the same op: `atomic_shared_ptr_lock_free.h` does one 128-bit CAS/`load`/`store` on the whole struct with no locking at all, while `atomic_shared_ptr_lock_based.h` steals a spinlock bit from `__ctrl_` and does a plain relaxed store to `__ptr_` under that lock. If TU A (`-mcx16`) and TU B (without `-mcx16`) both touch the same `atomic<shared_ptr<Foo>>`, TU B's `store()` never does a 128-bit RMW at all, and TU A's `load()` never looks at TU B's lock bit - that's a real data race, not just a layout mismatch.

On "always use the lock-free/DWCAS variant": I checked this against the actual runtime library sources rather than assuming it holds everywhere libc++ ships. On glibc/x86-64 there's real precedent - GCC's libatomic added a runtime CPUID check via ifunc specifically so a non-`-mcx16` TU (calling into libatomic) and a `-mcx16` TU (inlining `cmpxchg16b`) converge on the same instruction ([Szabolcs Nagy, gcc-list, Nov 2016](https://gcc.gnu.org/legacy-ml/gcc/2016-11/msg00101.html).

But [GCC PR 70191](https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70191) - open and unconfirmed since 2016 - documents exactly the failure mode for targets without ifunc (the bug's own comment thread confirms the glibc-side fix independently: "Note on Linux ifunc is used to support cmpxchg16b on x86_64"). It was reproduced concretely on Solaris: `-march=nehalem` inlines `cmpxchg16b`, code compiled without that flag links against `__atomic_load_16` in libatomic, which has no CPUID check and always uses a lock-based implementation - the two are incompatible if linked together and run on hardware that does support cmpxchg16b. Nagy's own comment on the bug generalizes the scope: "affects all x86 targets (except *gnu unless gcc was configured with `--disable-gnu-indirect-function`)" - so musl, Android, BSD, and Solaris are all affected when paired with GCC's libatomic.

The toolchain libc++ actually pairs with on those targets is usually clang + compiler-rt, not GCC + libatomic, so I checked that runtime directly too: compiler-rt's generic 16-byte fallback (`lib/builtins/atomic.c`) has no runtime CPU-feature check either - `IS_LOCK_FREE_16` is resolved by the flags compiler-rt itself was built with, so a baseline-built compiler-rt always takes the lock-table path regardless of what the calling TU was compiled with. Compiler-rt's maintainers are aware this file is a hazard on shared-linking platforms for exactly this reason - `COMPILER_RT_EXCLUDE_ATOMIC_BUILTIN` defaults to On specifically "to avoid misbehavior on platforms that use shared libraries, where multiple definitions of the fallback locks could result in improperly synchronized access to atomic variables" ([llvm-embedded-toolchain-for-Arm#634](https://github.com/ARM-software/LLVM-embedded-toolchain-for-Arm/issues/634)). So on musl/Android/BSD/Solaris the danger you're describing is a live, acknowledged, still-open gap in the ecosystem, not a hypothetical - it's just one layer down, in the runtime library rather than in libc++ itself, unless something else in that toolchain does the CPUID check. AArch64 looks better: compiler-rt ships out-of-line LSE helpers (`lib/builtins/aarch64/lse.S`) gated by a process-global `__aarch64_have_lse_atomics` set from HWCAP at startup, so that case looks like it should converge, but I haven't verified the 16-byte case specifically follows the same outline-atomics path as the smaller sizes.

So I agree "always lock-free, build on the runtime's own dispatch" is the right direction, but I don't think it's safe to claim as a universal guarantee for every threading-enabled target yet. Do you know of an existing guarantee (or counterexample) for the musl/Android/BSD case specifically? If there isn't one, this needs either a documented restriction, or that runtime dispatch actually has to be added (in libc++ or upstream in compiler-rt) for those targets.


https://github.com/llvm/llvm-project/pull/194215