From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 3065 invoked by alias); 3 Jun 2015 09:14:50 -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 3056 invoked by uid 89); 3 Jun 2015 09:14:49 -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 09:14:48 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5019FAC73; Wed, 3 Jun 2015 09:14:45 +0000 (UTC) Message-ID: <556EC584.9030004@suse.cz> Date: Wed, 03 Jun 2015 09:16: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: Jeff Law , 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> In-Reply-To: <556DE50D.7040303@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00285.txt.bz2 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. 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