From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10063 invoked by alias); 3 Jun 2015 14:11:36 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 9388 invoked by uid 89); 3 Jun 2015 14:11:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.8 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Wed, 03 Jun 2015 14:11:35 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 36F55AC8A for ; Wed, 3 Jun 2015 14:11:31 +0000 (UTC) Message-ID: <556F0B12.70106@suse.cz> Date: Wed, 03 Jun 2015 14:27:00 -0000 From: =?windows-1252?Q?Martin_Li=9Aka?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: gcc-patches@gcc.gnu.org Subject: Re: [PATCH 1/2] Memory statistics enhancement. References: <556DB68E.1050807@redhat.com> <556DC634.80103@suse.cz> <556DE50D.7040303@redhat.com> <556EC584.9030004@suse.cz> <556EFE93.4080905@redhat.com> In-Reply-To: <556EFE93.4080905@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00326.txt.bz2 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 >>>>>> >>>>>> * 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