From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22229 invoked by alias); 2 Jun 2015 13:58:41 -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 22213 invoked by uid 89); 2 Jun 2015 13:58:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 02 Jun 2015 13:58:40 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id EAC2C19F997; Tue, 2 Jun 2015 13:58:38 +0000 (UTC) Received: from localhost.localdomain (ovpn-113-154.phx2.redhat.com [10.3.113.154]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t52Dwc6e016644; Tue, 2 Jun 2015 09:58:38 -0400 Message-ID: <556DB68E.1050807@redhat.com> Date: Tue, 02 Jun 2015 14:00:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: mliska , gcc-patches@gcc.gnu.org Subject: Re: [PATCH 1/2] Memory statistics enhancement. References: In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00203.txt.bz2 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. @@ -235,10 +301,10 @@ pool_allocator::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