]> git.ipfire.org Git - thirdparty/glibc.git/commitdiff
atomic: Remove atomic_forced_read
authorWilco Dijkstra <wilco.dijkstra@arm.com>
Fri, 3 Oct 2025 18:36:00 +0000 (18:36 +0000)
committerWilco Dijkstra <wilco.dijkstra@arm.com>
Wed, 8 Oct 2025 12:59:30 +0000 (12:59 +0000)
Remove the odd atomic_forced_read which is neither atomic nor forced.
Some uses are completely redundant, so simply remove them.  In other cases
the intended use is to force a memory ordering, so use acquire load for those.
In yet other cases their purpose is unclear, for example __nscd_cache_search
appears to allow concurrent accesses to the cache while it is being garbage
collected by another thread!  Use relaxed atomic loads here to block spills
from accidentally reloading memory that is being changed.

Passes regress on AArch64, OK for commit?

elf/dl-lookup.c
include/atomic.h
malloc/malloc-debug.c
nptl/pthread_sigqueue.c
nscd/nscd_helper.c

index 2f5cd674f58b816d2f0317fe3aaab4bb8ac163ef..c32362da4c3813fd269048d88c67a748f377d1e5 100644 (file)
@@ -342,12 +342,12 @@ do_lookup_x (const char *undef_name, unsigned int new_hash,
             const struct r_found_version *const version, int flags,
             struct link_map *skip, int type_class, struct link_map *undef_map)
 {
-  size_t n = scope->r_nlist;
-  /* Make sure we read the value before proceeding.  Otherwise we
+  /* Make sure we read r_nlist before r_list, or otherwise we
      might use r_list pointing to the initial scope and r_nlist being
      the value after a resize.  That is the only path in dl-open.c not
-     protected by GSCOPE.  A read barrier here might be to expensive.  */
-  __asm volatile ("" : "+r" (n), "+m" (scope->r_list));
+     protected by GSCOPE.  This works if all updates also use a store-
+     release or release barrier.  */
+  size_t n = atomic_load_acquire (&scope->r_nlist);
   struct link_map **list = scope->r_list;
 
   do
@@ -541,15 +541,13 @@ add_dependency (struct link_map *undef_map, struct link_map *map, int flags)
   if (is_nodelete (map, flags))
     return 0;
 
-  struct link_map_reldeps *l_reldeps
-    = atomic_forced_read (undef_map->l_reldeps);
-
   /* Make sure l_reldeps is read before l_initfini.  */
-  atomic_read_barrier ();
+  struct link_map_reldeps *l_reldeps
+    = atomic_load_acquire (&undef_map->l_reldeps);
 
   /* Determine whether UNDEF_MAP already has a reference to MAP.  First
      look in the normal dependencies.  */
-  struct link_map **l_initfini = atomic_forced_read (undef_map->l_initfini);
+  struct link_map **l_initfini = undef_map->l_initfini;
   if (l_initfini != NULL)
     {
       for (i = 0; l_initfini[i] != NULL; ++i)
@@ -583,7 +581,7 @@ add_dependency (struct link_map *undef_map, struct link_map *map, int flags)
         it can e.g. point to unallocated memory.  So avoid the optimizer
         treating the above read from MAP->l_serial as ensurance it
         can safely dereference it.  */
-      map = atomic_forced_read (map);
+      __asm ("" : "=r" (map) : "0" (map));
 
       /* From this point on it is unsafe to dereference MAP, until it
         has been found in one of the lists.  */
index a9eb4d740bce204e7745d09ef2e08c1ea834a4c6..12c439632ccab87e69fc7b9471a3c9f7a31331ca 100644 (file)
 #endif
 
 
-#ifndef atomic_forced_read
-# define atomic_forced_read(x) \
-  ({ __typeof (x) __x; __asm ("" : "=r" (__x) : "0" (x)); __x; })
-#endif
-
 /* This is equal to 1 iff the architecture supports 64b atomic operations.  */
 #ifndef __HAVE_64B_ATOMICS
 #error Unable to determine if 64-bit atomics are present.
index 0bb57841eebb74cb8097577b2577f18d1be6442a..15867e23c56416236e1be163125b9176538e18b9 100644 (file)
@@ -155,7 +155,7 @@ static size_t pagesize;
 static void *
 __debug_malloc (size_t bytes)
 {
-  void *(*hook) (size_t, const void *) = atomic_forced_read (__malloc_hook);
+  void *(*hook) (size_t, const void *) = __malloc_hook;
   if (__glibc_unlikely (hook != NULL))
     return (*hook)(bytes, RETURN_ADDRESS (0));
 
@@ -179,7 +179,7 @@ strong_alias (__debug_malloc, malloc)
 static void
 __debug_free (void *mem)
 {
-  void (*hook) (void *, const void *) = atomic_forced_read (__free_hook);
+  void (*hook) (void *, const void *) = __free_hook;
   if (__glibc_unlikely (hook != NULL))
     {
       (*hook)(mem, RETURN_ADDRESS (0));
@@ -201,8 +201,7 @@ strong_alias (__debug_free, free)
 static void *
 __debug_realloc (void *oldmem, size_t bytes)
 {
-  void *(*hook) (void *, size_t, const void *) =
-    atomic_forced_read (__realloc_hook);
+  void *(*hook) (void *, size_t, const void *) = __realloc_hook;
   if (__glibc_unlikely (hook != NULL))
     return (*hook)(oldmem, bytes, RETURN_ADDRESS (0));
 
@@ -230,8 +229,7 @@ strong_alias (__debug_realloc, realloc)
 static void *
 _debug_mid_memalign (size_t alignment, size_t bytes, const void *address)
 {
-  void *(*hook) (size_t, size_t, const void *) =
-    atomic_forced_read (__memalign_hook);
+  void *(*hook) (size_t, size_t, const void *) = __memalign_hook;
   if (__glibc_unlikely (hook != NULL))
     return (*hook)(alignment, bytes, address);
 
@@ -330,7 +328,7 @@ __debug_calloc (size_t nmemb, size_t size)
       return NULL;
     }
 
-  void *(*hook) (size_t, const void *) = atomic_forced_read (__malloc_hook);
+  void *(*hook) (size_t, const void *) = __malloc_hook;
   if (__glibc_unlikely (hook != NULL))
     {
       void *mem = (*hook)(bytes, RETURN_ADDRESS (0));
index cd7d8cc77532575469f8fb554d043ceae1f23a00..6d47ce16254e43d586ae9b7d25ba764020c69250 100644 (file)
@@ -33,7 +33,7 @@ __pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
   /* Force load of pd->tid into local variable or register.  Otherwise
      if a thread exits between ESRCH test and tgkill, we might return
      EINVAL, because pd->tid would be cleared by the kernel.  */
-  pid_t tid = atomic_forced_read (pd->tid);
+  pid_t tid = atomic_load_relaxed (&pd->tid);
   if (__glibc_unlikely (tid <= 0))
     /* Not a valid thread handle.  */
     return ESRCH;
index 4082316a92e4c3e18d62c55c1967e39c154c023f..e1603334645a3b252bc7c3ee7796f375632b6592 100644 (file)
@@ -454,7 +454,6 @@ __nscd_cache_search (request_type type, const char *key, size_t keylen,
   size_t datasize = mapped->datasize;
 
   ref_t trail = mapped->head->array[hash];
-  trail = atomic_forced_read (trail);
   ref_t work = trail;
   size_t loop_cnt = datasize / (MINIMUM_HASHENTRY_SIZE
                                + offsetof (struct datahead, data) / 2);
@@ -468,17 +467,18 @@ __nscd_cache_search (request_type type, const char *key, size_t keylen,
       /* Although during garbage collection when moving struct hashentry
         records around we first copy from old to new location and then
         adjust pointer from previous hashentry to it, there is no barrier
-        between those memory writes.  It is very unlikely to hit it,
-        so check alignment only if a misaligned load can crash the
-        application.  */
+        between those memory writes!!! This is extremely risky on any
+        modern CPU which can reorder memory accesses very aggressively.
+        Check alignment, both as a partial consistency check and to avoid
+        crashes on targets which require atomic loads to be aligned.  */
       if ((uintptr_t) here & (__alignof__ (*here) - 1))
        return NULL;
 
       if (type == here->type
          && keylen == here->len
-         && (here_key = atomic_forced_read (here->key)) + keylen <= datasize
+         && (here_key = atomic_load_relaxed (&here->key)) + keylen <= datasize
          && memcmp (key, mapped->data + here_key, keylen) == 0
-         && ((here_packet = atomic_forced_read (here->packet))
+         && ((here_packet = atomic_load_relaxed (&here->packet))
              + sizeof (struct datahead) <= datasize))
        {
          /* We found the entry.  Increment the appropriate counter.  */
@@ -497,7 +497,7 @@ __nscd_cache_search (request_type type, const char *key, size_t keylen,
            return dh;
        }
 
-      work = atomic_forced_read (here->next);
+      work = atomic_load_relaxed (&here->next);
       /* Prevent endless loops.  This should never happen but perhaps
         the database got corrupted, accidentally or deliberately.  */
       if (work == trail || loop_cnt-- == 0)
@@ -514,7 +514,7 @@ __nscd_cache_search (request_type type, const char *key, size_t keylen,
          if (trail + MINIMUM_HASHENTRY_SIZE > datasize)
            return NULL;
 
-         trail = atomic_forced_read (trailelem->next);
+         trail = atomic_load_relaxed (&trailelem->next);
        }
       tick = 1 - tick;
     }