On 06/02/2015 07:17 PM, Jeff Law wrote:
> On 06/02/2015 09:05 AM, Martin Liška wrote:
>> On 06/02/2015 03:58 PM, Jeff Law wrote:
>>> 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 <mli...@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.
> It doesn't look like you did anything with this. Is there a reason that the
> dump_header and dump_footer have different linkage? Also the linkage/return
> type for dump_header should be on its own line.
>
> With that fixed, this is OK for the trunk.
>
> jeff
Hi Jeff.
Different linkage is because of dump_header is just a generic header unrelated
to any real numbers.
On the other hand, dump_footer is called on a total instance.
I've just identified that the whole memory statistics infrastructure lacks
correct GNU formatting
in case of C++ member functions. I'm going to fix it in a different patch.
Thanks,
Martin
