On 06/03/2015 03:18 PM, Jeff Law wrote: > On 06/03/2015 03:14 AM, Martin Liška wrote: >> 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. > Right, but why does that affect linkage? Sorry to keep harping on this > minor issue, but something just doesn't make any sense to me.
Hi. That's fine, I would like to explain that difference. Each of these two functions are called in a bit different way: T::dump_header (mem_location::get_origin_name (origin)); // that's why the function is static total.dump_footer (); // that's why the function is method Martin > >> >> 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. > Excellent. Thanks, > > jeff