Re: [PATCH 2/3] statistics: Fix destruction ordering issues with mem_alloc_description

On Sat, Aug 1, 2026 at 4:04 PM Lewis Hyatt <[email protected]> wrote:
>
> The previous patch fixed the documented issue PR28734 with
> --enable-gather-detailed-mem-stats, but there remain a couple other issues
> not previously reported:
>
>     a) Around 200 tests still fail with statistics gathering enabled,
>        including:
>            gfortran.dg/gomp/class-firstprivate-1.f90   -O  (internal compiler error: in check_complete_insertion, at hash-table.h:572) gfortran.sum
>        as well as most GCOV and JIT tests.
>
>     b) bootstrap fails when configured with:
>            --enable-gather-detailed-mem-stats --with-build-config=bootstrap-lto
>        with similar ICEs in lots of places during stage 2.
>
> These both have the same cause. The classes that implement the statistics
> gathering are file-local static variables that are referenced from the
> destructors of the data structures (hash_map, vec, ggc, etc) that they are
> tracking. Consequently, any static instance of such a data structure is
> subject to destruction order issues; if the mem_alloc_description is
> destroyed before the static object it is tracking, then problems will arise.
>
> It seems that the cleanest solution is to arrange that mem_alloc_description
> objects are never destroyed, given that they persist anyway for the duration
> of the process. It does not seem desirable to forbid static instances of all
> data structures, since these seem to be used in many places.

OK.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * mem-stats.h (mem_alloc_description::mem_alloc_description): Make
>         private.
>         (mem_alloc_description::~mem_alloc_description): Disallow
>         destruction entirely.
>         (mem_alloc_description::instance): New function.
>         * alloc-pool.cc (dump_alloc_pool_statistics): Adapt to
>         pool_allocator_usage being a function instead of a variable.
>         * alloc-pool.h (pool_allocator_usage): New function.
>         (TBlockAllocator>::initialize): Adapt to
>         pool_allocator_usage being a function instead of a variable.
>         (TBlockAllocator>::release): Likewise.
>         (TBlockAllocator>::allocate): Likewise.
>         (TBlockAllocator>::remove): Likewise.
>         * bitmap.cc (bitmap_mem_desc): Change from a static variable to an
>         inline function.
>         (bitmap_register): Adapt to bitmap_mem_desc being a function instead
>         of a variable.
>         (register_overhead): Likewise.
>         (release_overhead): Likewise.
>         (bitmap_list_find_element): Likewise.
>         (bitmap_tree_splay): Likewise.
>         (bitmap_tree_find_element): Likewise.
>         (dump_bitmap_statistics): Likewise.
>         * bitmap.h (bitmap_mem_desc): Remove unneeded extern declaration.
>         * ggc-common.cc (ggc_mem_desc): Change from a static variable to an
>         inline function.
>         (ggc_record_overhead): Adapt to ggc_mem_desc being a function
>         instead of a variable.
>         (ggc_free_overhead): Likewise
>         (ggc_prune_overhead_list): Likewise.
>         * vec.cc (vec_mem_desc): Change from a static variable to a
>         function.
>         (vec_prefix::register_overhead): Adapt to vec_mem_desc being a
>         function instead of a variable.
>         (vec_prefix::release_overhead): Likewise.
>         (dump_vec_loc_statistics): Likewise.
>         * hash-table.cc (hash_table_usage): Move to...
>         * hash-table.h (hash_table_usage): ...here, and use new singleton
>         interface.
> ---
>  gcc/alloc-pool.cc |  3 +--
>  gcc/alloc-pool.h  | 14 +++++++++-----
>  gcc/bitmap.cc     | 26 +++++++++++++++-----------
>  gcc/bitmap.h      |  3 ---
>  gcc/ggc-common.cc | 22 +++++++++++++---------
>  gcc/hash-table.cc | 11 -----------
>  gcc/hash-table.h  |  6 +++++-
>  gcc/mem-stats.h   | 41 ++++++++++++++++++-----------------------
>  gcc/vec.cc        | 22 +++++++++++++---------
>  9 files changed, 74 insertions(+), 74 deletions(-)
>
> diff --git a/gcc/alloc-pool.cc b/gcc/alloc-pool.cc
> index 8f35fee6821..cb69b326271 100644
> --- a/gcc/alloc-pool.cc
> +++ b/gcc/alloc-pool.cc
> @@ -24,7 +24,6 @@ along with GCC; see the file COPYING3.  If not see
>  #include "alloc-pool.h"
>
>  ALLOC_POOL_ID_TYPE last_id;
> -mem_alloc_description<pool_usage> pool_allocator_usage;
>  bool after_memory_report = false;
>
>  /* Output per-alloc_pool memory usage statistics.  */
> @@ -34,5 +33,5 @@ dump_alloc_pool_statistics (void)
>    if (! GATHER_STATISTICS)
>      return;
>
> -  pool_allocator_usage.dump (ALLOC_POOL_ORIGIN);
> +  pool_allocator_usage ().dump (ALLOC_POOL_ORIGIN);
>  }
> diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
> index f81c1794137..3c801789edf 100644
> --- a/gcc/alloc-pool.h
> +++ b/gcc/alloc-pool.h
> @@ -100,7 +100,11 @@ public:
>    const char *m_pool_name;
>  };
>
> -extern mem_alloc_description<pool_usage> pool_allocator_usage;
> +inline auto &
> +pool_allocator_usage ()
> +{
> +  return mem_alloc_description<pool_usage>::instance<ALLOC_POOL_ORIGIN> ();
> +}
>
>  #if 0
>  /* If a pool with custom block size is needed, one might use the following
> @@ -274,7 +278,7 @@ base_pool_allocator <TBlockAllocator>::initialize ()
>
>    if (GATHER_STATISTICS)
>      {
> -      pool_usage *u = pool_allocator_usage.register_descriptor
> +      pool_usage *u = pool_allocator_usage ().register_descriptor
>         (this, new mem_location (m_location));
>
>        u->m_element_size = m_elt_size;
> @@ -315,7 +319,7 @@ base_pool_allocator <TBlockAllocator>::release ()
>
>    if (GATHER_STATISTICS && !after_memory_report)
>      {
> -      pool_allocator_usage.release_instance_overhead
> +      pool_allocator_usage ().release_instance_overhead
>         (this, (m_elts_allocated - m_elts_free) * m_elt_size);
>      }
>
> @@ -357,7 +361,7 @@ base_pool_allocator <TBlockAllocator>::allocate ()
>
>    if (GATHER_STATISTICS)
>      {
> -      pool_allocator_usage.register_instance_overhead (m_elt_size, this);
> +      pool_allocator_usage ().register_instance_overhead (m_elt_size, this);
>      }
>
>  #ifdef ENABLE_VALGRIND_ANNOTATIONS
> @@ -458,7 +462,7 @@ base_pool_allocator <TBlockAllocator>::remove (void *object)
>
>    if (GATHER_STATISTICS)
>      {
> -      pool_allocator_usage.release_instance_overhead (this, m_elt_size);
> +      pool_allocator_usage ().release_instance_overhead (this, m_elt_size);
>      }
>  }
>
> diff --git a/gcc/bitmap.cc b/gcc/bitmap.cc
> index 6bf0ee99d04..d896a551394 100644
> --- a/gcc/bitmap.cc
> +++ b/gcc/bitmap.cc
> @@ -40,7 +40,11 @@ using bitmap_splay_tree
>    = splay_tree_without_parent<bitmap_splay_tree_accessors>;
>
>  /* Memory allocation statistics purpose instance.  */
> -mem_alloc_description<bitmap_usage> bitmap_mem_desc;
> +inline auto &
> +bitmap_mem_desc ()
> +{
> +  return mem_alloc_description<bitmap_usage>::instance<BITMAP_ORIGIN> ();
> +}
>
>  /* Static zero-initialized bitmap obstack used for default initialization
>     of bitmap_head.  */
> @@ -54,8 +58,8 @@ bitmap_register (bitmap b MEM_STAT_DECL)
>    gcc_assert (b->alloc_descriptor == 0);
>    b->alloc_descriptor = alloc_descriptor_max_uid++;
>
> -  bitmap_mem_desc.register_descriptor (b->get_descriptor (), BITMAP_ORIGIN,
> -                                      false FINAL_PASS_MEM_STAT);
> +  bitmap_mem_desc ().register_descriptor (b->get_descriptor (), BITMAP_ORIGIN,
> +                                         false FINAL_PASS_MEM_STAT);
>  }
>
>  /* Account the overhead.  */
> @@ -63,8 +67,8 @@ static void
>  register_overhead (bitmap b, size_t amount)
>  {
>    unsigned *d = b->get_descriptor ();
> -  if (bitmap_mem_desc.contains_descriptor_for_instance (d))
> -    bitmap_mem_desc.register_instance_overhead (amount, d);
> +  if (bitmap_mem_desc ().contains_descriptor_for_instance (d))
> +    bitmap_mem_desc ().register_instance_overhead (amount, d);
>  }
>
>  /* Release the overhead.  */
> @@ -72,8 +76,8 @@ static void
>  release_overhead (bitmap b, size_t amount, bool remove_from_map)
>  {
>    unsigned *d = b->get_descriptor ();
> -  if (bitmap_mem_desc.contains_descriptor_for_instance (d))
> -    bitmap_mem_desc.release_instance_overhead (d, amount, remove_from_map);
> +  if (bitmap_mem_desc ().contains_descriptor_for_instance (d))
> +    bitmap_mem_desc ().release_instance_overhead (d, amount, remove_from_map);
>  }
>
>
> @@ -374,7 +378,7 @@ bitmap_list_find_element (bitmap head, unsigned int indx)
>       call initialize function.  */
>    bitmap_usage *usage = NULL;
>    if (GATHER_STATISTICS)
> -    usage = bitmap_mem_desc.get_descriptor_for_instance (head);
> +    usage = bitmap_mem_desc ().get_descriptor_for_instance (head);
>
>    /* This bitmap has more than one element, and we're going to look
>       through the elements list.  Count that as a search.  */
> @@ -481,7 +485,7 @@ bitmap_tree_splay (bitmap head, bitmap_element *t, unsigned int indx)
>
>    bitmap_usage *usage = NULL;
>    if (GATHER_STATISTICS)
> -    usage = bitmap_mem_desc.get_descriptor_for_instance (head);
> +    usage = bitmap_mem_desc ().get_descriptor_for_instance (head);
>
>    N.prev = N.next = NULL;
>    l = r = &N;
> @@ -587,7 +591,7 @@ bitmap_tree_find_element (bitmap head, unsigned int indx)
>       call initialize function.  */
>    bitmap_usage *usage = NULL;
>    if (GATHER_STATISTICS)
> -    usage = bitmap_mem_desc.get_descriptor_for_instance (head);
> +    usage = bitmap_mem_desc ().get_descriptor_for_instance (head);
>
>    /* This bitmap has more than one element, and we're going to look
>       through the elements list.  Count that as a search.  */
> @@ -2857,7 +2861,7 @@ dump_bitmap_statistics (void)
>    if (!GATHER_STATISTICS)
>      return;
>
> -  bitmap_mem_desc.dump (BITMAP_ORIGIN);
> +  bitmap_mem_desc ().dump (BITMAP_ORIGIN);
>  }
>
>  DEBUG_FUNCTION void
> diff --git a/gcc/bitmap.h b/gcc/bitmap.h
> index c6fd91ee90a..88ff5cf020e 100644
> --- a/gcc/bitmap.h
> +++ b/gcc/bitmap.h
> @@ -272,9 +272,6 @@ public:
>    uint64_t m_search_iter;
>  };
>
> -/* Bitmap memory description.  */
> -extern mem_alloc_description<bitmap_usage> bitmap_mem_desc;
> -
>  /* Fundamental storage type for bitmap.  */
>
>  typedef unsigned long BITMAP_WORD;
> diff --git a/gcc/ggc-common.cc b/gcc/ggc-common.cc
> index 6d3dca78f03..8752fad81b7 100644
> --- a/gcc/ggc-common.cc
> +++ b/gcc/ggc-common.cc
> @@ -1269,7 +1269,11 @@ public:
>  };
>
>  /* GCC memory description.  */
> -static mem_alloc_description<ggc_usage> ggc_mem_desc;
> +inline auto &
> +ggc_mem_desc ()
> +{
> +  return mem_alloc_description<ggc_usage>::instance<GGC_ORIGIN> ();
> +}
>
>  /* Dump per-site memory statistics.  */
>
> @@ -1281,17 +1285,17 @@ dump_ggc_loc_statistics ()
>
>    ggc_collect (GGC_COLLECT_FORCE);
>
> -  ggc_mem_desc.dump (GGC_ORIGIN);
> +  ggc_mem_desc ().dump (GGC_ORIGIN);
>  }
>
>  /* Record ALLOCATED and OVERHEAD bytes to descriptor NAME:LINE (FUNCTION).  */
>  void
>  ggc_record_overhead (size_t allocated, size_t overhead, void *ptr MEM_STAT_DECL)
>  {
> -  ggc_usage *usage = ggc_mem_desc.register_descriptor (ptr, GGC_ORIGIN, false
> -                                                      FINAL_PASS_MEM_STAT);
> +  ggc_usage *usage = ggc_mem_desc ().register_descriptor (ptr, GGC_ORIGIN, false
> +                                                         FINAL_PASS_MEM_STAT);
>
> -  ggc_mem_desc.register_object_overhead (usage, allocated + overhead, ptr);
> +  ggc_mem_desc ().register_object_overhead (usage, allocated + overhead, ptr);
>    usage->register_overhead (allocated, overhead);
>  }
>
> @@ -1299,7 +1303,7 @@ ggc_record_overhead (size_t allocated, size_t overhead, void *ptr MEM_STAT_DECL)
>  void
>  ggc_free_overhead (void *ptr)
>  {
> -  ggc_mem_desc.release_object_overhead (ptr);
> +  ggc_mem_desc ().release_object_overhead (ptr);
>  }
>
>  /* After live values has been marked, walk all recorded pointers and see if
> @@ -1309,13 +1313,13 @@ ggc_prune_overhead_list (void)
>  {
>    typedef hash_map<const void *, std::pair<ggc_usage *, size_t > > map_t;
>
> -  map_t::iterator it = ggc_mem_desc.m_reverse_object_map->begin ();
> +  map_t::iterator it = ggc_mem_desc ().m_reverse_object_map->begin ();
>
> -  for (; it != ggc_mem_desc.m_reverse_object_map->end (); ++it)
> +  for (; it != ggc_mem_desc ().m_reverse_object_map->end (); ++it)
>      if (!ggc_marked_p ((*it).first))
>        {
>          (*it).second.first->m_collected += (*it).second.second;
> -       ggc_mem_desc.m_reverse_object_map->remove ((*it).first);
> +       ggc_mem_desc ().m_reverse_object_map->remove ((*it).first);
>        }
>  }
>
> diff --git a/gcc/hash-table.cc b/gcc/hash-table.cc
> index 22a08b2735b..20650d1ac53 100644
> --- a/gcc/hash-table.cc
> +++ b/gcc/hash-table.cc
> @@ -101,17 +101,6 @@ hash_table_higher_prime_index (unsigned long n)
>    return low;
>  }
>
> -/* Return a reference to the lazily initialized hash-table usage description.
> -   This needs to be a function rather than a simple global variable so that it
> -   is reliably initialized before hash table variables in other files such as
> -   sem_item::m_type_hash_cache.  */
> -mem_alloc_description<mem_usage>&
> -hash_table_usage ()
> -{
> -  static mem_alloc_description<mem_usage> usage;
> -  return usage;
> -}
> -
>  /* Support function for statistics.  */
>  void dump_hash_table_loc_statistics (void)
>  {
> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
> index 0bee2850b8c..664c3e0ce45 100644
> --- a/gcc/hash-table.h
> +++ b/gcc/hash-table.h
> @@ -636,7 +636,11 @@ private:
>  #include "mem-stats.h"
>  #include "hash-map.h"
>
> -extern mem_alloc_description<mem_usage>& hash_table_usage (void);
> +inline auto &
> +hash_table_usage ()
> +{
> +  return mem_alloc_description<mem_usage>::instance<HASH_TABLE_ORIGIN> ();
> +}
>
>  /* Support function for statistics.  */
>  extern void dump_hash_table_loc_statistics (void);
> diff --git a/gcc/mem-stats.h b/gcc/mem-stats.h
> index 2d4de15ccff..dd83c919877 100644
> --- a/gcc/mem-stats.h
> +++ b/gcc/mem-stats.h
> @@ -276,7 +276,23 @@ public:
>  template <class T>
>  class mem_alloc_description
>  {
> +
> +  /* Constructor is private to enforce singleton.  */
> +  mem_alloc_description ();
> +
> +  /* Destruction is not allowed, since we might be tracking
> +     static objects with undefined destruction order.  */
> +  ~mem_alloc_description () = delete;
> +
>  public:
> +
> +  template<mem_alloc_origin>
> +  static auto &instance ()
> +  {
> +    static const auto self = new mem_alloc_description;
> +    return *self;
> +  }
> +
>    struct mem_location_hash : nofree_ptr_hash <mem_location>
>    {
>      static hashval_t
> @@ -306,11 +322,6 @@ public:
>    typedef hash_map <const void *, std::pair<T *, size_t> > reverse_object_map_t;
>    typedef std::pair <mem_location *, T *> mem_list_t;
>
> -  /* Default constructor.  */
> -  mem_alloc_description ();
> -
> -  /* Default destructor.  */
> -  ~mem_alloc_description ();
>
>    /* Returns true if instance PTR is registered by the memory description.  */
>    bool contains_descriptor_for_instance (const void *ptr);
> @@ -558,29 +569,13 @@ template <class T>
>  inline
>  mem_alloc_description<T>::mem_alloc_description ()
>  {
> +  /* Note it is important to pass false for the 4th argument (GATHER_MEM_STATS)
> +     to avoid infinite recursion in instance ().  */
>    m_map = new mem_map_t (13, false, false, false);
>    m_reverse_map = new reverse_mem_map_t (13, false, false, false);
>    m_reverse_object_map = new reverse_object_map_t (13, false, false, false);
>  }
>
> -/* Default destructor.  */
> -
> -template <class T>
> -inline
> -mem_alloc_description<T>::~mem_alloc_description ()
> -{
> -  for (typename mem_map_t::iterator it = m_map->begin (); it != m_map->end ();
> -       ++it)
> -    {
> -      delete (*it).first;
> -      delete (*it).second;
> -    }
> -
> -  delete m_map;
> -  delete m_reverse_map;
> -  delete m_reverse_object_map;
> -}
> -
>  /* Get all tracked instances registered by the description. Items are filtered
>     by ORIGIN type, LENGTH is return value where we register the number of
>     elements in the list. If we want to process custom order, CMP comparator
> diff --git a/gcc/vec.cc b/gcc/vec.cc
> index 0740e495848..8313adde2b2 100644
> --- a/gcc/vec.cc
> +++ b/gcc/vec.cc
> @@ -111,7 +111,11 @@ public:
>  };
>
>  /* Vector memory description.  */
> -static mem_alloc_description <vec_usage> vec_mem_desc;
> +inline auto &
> +vec_mem_desc ()
> +{
> +  return mem_alloc_description<vec_usage>::instance<VEC_ORIGIN> ();
> +}
>
>  /* Account the overhead.  */
>
> @@ -119,10 +123,10 @@ void
>  vec_prefix::register_overhead (void *ptr, size_t elements,
>                                size_t element_size MEM_STAT_DECL)
>  {
> -  vec_mem_desc.register_descriptor (ptr, VEC_ORIGIN, false
> -                                   FINAL_PASS_MEM_STAT);
> +  vec_mem_desc ().register_descriptor (ptr, VEC_ORIGIN, false
> +                                      FINAL_PASS_MEM_STAT);
>    vec_usage *usage
> -    = vec_mem_desc.register_instance_overhead (elements * element_size, ptr);
> +    = vec_mem_desc ().register_instance_overhead (elements * element_size, ptr);
>    usage->m_element_size = element_size;
>    usage->m_items += elements;
>    if (usage->m_items_peak < usage->m_items)
> @@ -135,11 +139,11 @@ void
>  vec_prefix::release_overhead (void *ptr, size_t size, size_t elements,
>                               bool in_dtor MEM_STAT_DECL)
>  {
> -  if (!vec_mem_desc.contains_descriptor_for_instance (ptr))
> -    vec_mem_desc.register_descriptor (ptr, VEC_ORIGIN,
> +  if (!vec_mem_desc ().contains_descriptor_for_instance (ptr))
> +    vec_mem_desc ().register_descriptor (ptr, VEC_ORIGIN,
>                                       false FINAL_PASS_MEM_STAT);
> -  vec_usage *usage = vec_mem_desc.release_instance_overhead (ptr, size,
> -                                                            in_dtor);
> +  vec_usage *usage = vec_mem_desc ().release_instance_overhead (ptr, size,
> +                                                               in_dtor);
>    usage->m_items -= elements;
>  }
>
> @@ -173,7 +177,7 @@ vec_prefix::calculate_allocation_1 (unsigned alloc, unsigned desired)
>  void
>  dump_vec_loc_statistics (void)
>  {
> -  vec_mem_desc.dump (VEC_ORIGIN);
> +  vec_mem_desc ().dump (VEC_ORIGIN);
>  }
>
>  /* Gets the next token from STR delimited by DELIMS (deliminator not included