Re: [PATCH 1/2] Memory statistics enhancement.

[Jeff Law <law@redhat.com>]

On 06/01/2015 10:16 AM, mliska wrote:
> Hi.
>
> Following 2 patches improve memory statistics infrastructure. First one
> ports pool allocator to the new infrastructure. And the second one makes
> column alignment properly.
>
> Both can bootstrap on x86_64-linux-pc and survive regression tests.
>
> Ready for trunk?
> Thank you,
> Martin
>
> Port pool-allocator memory stats to a new infrastructure.
>
> gcc/ChangeLog:
>
> 2015-06-02  Martin Liska  <mliska@suse.cz>
>
> 	* alloc-pool.c (allocate_pool_descriptor): Remove.
> 	(struct pool_output_info): Likewise.
> 	(print_alloc_pool_statistics): Likewise.
> 	(dump_alloc_pool_statistics): Likewise.
> 	* alloc-pool.h (struct pool_usage): New struct.
> 	(pool_allocator::initialize): Change usage of memory statistics
> 	to a new interface.
> 	(pool_allocator::release): Likewise.
> 	(pool_allocator::allocate): Likewise.
> 	(pool_allocator::remove): Likewise.
> 	* mem-stats-traits.h (enum mem_alloc_origin): Add new enum value
> 	for a pool allocator.
> 	* mem-stats.h (struct mem_location): Add new ctor.
> 	(struct mem_usage): Add counter for number of
> 	instances.
> 	(mem_alloc_description::register_descriptor): New overload of
> 	the function.
  -

> diff --git a/gcc/alloc-pool.h b/gcc/alloc-pool.h
> index 96a1342..a1727ce 100644
> --- a/gcc/alloc-pool.h
> +++ b/gcc/alloc-pool.h

> +  /* Dump usage coupled to LOC location, where TOTAL is sum of all rows.  */
> +  inline void dump (mem_location *loc, mem_usage &total) const
> +  {
> +    char s[4096];
> +    sprintf (s, "%s:%i (%s)", loc->get_trimmed_filename (),
> +	     loc->m_line, loc->m_function);
Static sized buffer used in a sprintf where the strings are potentially 
user controlled.   Not good, even in dumping code, still not good.

> +
> +    s[48] = '\0';
?!?  Presumably you're just truncating the output line here for the 
subsequent fprintf call.  Consider using a const with a symbolic name 
rather than the magic "48".  I say "consider" because there's magic 
constants all over the place in the dumping code. So it may not be worth 
the effort.  Your call.

  +
> +  /* Dump header with NAME.  */
> +  static inline void dump_header (const char *name)
> +  {
> +    fprintf (stderr, "%-32s%-48s %6s%11s%16s%17s%12s\n", "Pool name", name,
> +	     "Pools", "Leak", "Peak", "Times", "Elt size");
> +    print_dash_line ();
> +  }
> +
> +  /* Dump footer.  */
> +  inline void dump_footer ()
> +  {
> +    print_dash_line ();
> +    fprintf (stderr, "%s%75li%10li\n", "Total", (long)m_instances,
> +	     (long)m_allocated);
> +    print_dash_line ();
> +  }
Note the header is static inline, footer is just inline.  Please try to 
make them consistent.

  @@ -235,10 +301,10 @@ pool_allocator<T>::release ()
>         free (block);
>       }
>
> -  if (GATHER_STATISTICS && false)
> +  if (GATHER_STATISTICS)
>       {
> -      alloc_pool_descriptor *desc = allocate_pool_descriptor (m_name);
> -      desc->current -= (m_elts_allocated - m_elts_free) * m_elt_size;
> +      pool_allocator_usage.release_instance_overhead (this,
> +	(m_elts_allocated - m_elts_free) * m_elt_size);
Looks like line wrapping needs to be fixed.


Clearly the biggest issue is that static sized buffer used to hold the 
results of sprintf...  Once that and the smaller issues are fixed, this 
is OK.

jeff