From patchwork Thu Feb 2 16:37:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Martin_Li=C5=A1ka?= X-Patchwork-Id: 723184 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vDlz62Xwnz9s7D for ; Fri, 3 Feb 2017 03:37:37 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="n6Ql/W5t"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; q=dns; s=default; b=EtYn67YFXUk+gXA4v oA2FpMiyFz/Uy7tSml8dx0gkBH7BQT61Cl+iJDaohUuilEBrdR/0BduAxMnpwtwq 4toQKJFnNdRgEnWxi5V5FyFEPn6GhiqLrYpm5wl1Y2GPVlovtfpKvvNXRL7CFYN5 bdEamtSJuyV9ZFNkgXm6nAhz5A= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :subject:to:references:from:message-id:date:mime-version :in-reply-to:content-type; s=default; bh=fF47wq6reJa5v5aJJqgxBZd L6u0=; b=n6Ql/W5txqpzUqtwBYkz0TaWJu3Syu0SGFbY5DxWRotLsSULNnkIpL9 BMWGx46FxyfSnSy7d3DORtAMmdK7nXzfBbsVvjdLSA4tFjK7P1y1Dgu/+AgzWsWb doIhtHvQEmN8fNtIdMifGu5qVv5YQ2FvxdURdiVKToTwv7w4KUM0= Received: (qmail 75617 invoked by alias); 2 Feb 2017 16:37:25 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 75556 invoked by uid 89); 2 Feb 2017 16:37:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, SPF_PASS autolearn=ham version=3.3.2 spammy=approve X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Feb 2017 16:37:10 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9953AAD62; Thu, 2 Feb 2017 16:37:06 +0000 (UTC) Subject: Re: [PATCH] Fix memory leaks in IPA CP (PR ipa/79337). To: GCC Patches , Kugan Vivekanandarajah References: <20170202161313.fti6eqt42nwh5eyv@virgil.suse.cz> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: <65d45d04-19d2-7c07-2412-f19983800dd4@suse.cz> Date: Thu, 2 Feb 2017 17:37:06 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170202161313.fti6eqt42nwh5eyv@virgil.suse.cz> X-IsSubscribed: yes On 02/02/2017 05:13 PM, Martin Jambor wrote: > Hi, > > On Thu, Feb 02, 2017 at 01:53:35PM +0100, Martin Liska wrote: >> Hello. >> >> As mentioned in the PR, there is memory leak that is caused by fact, that ipa_node_params_t >> does release memory just in ipa_node_params_t::remove. That's wrong because the callback is called >> just when cgraph_removal_hook is triggered. Thus the proper implementation is to move it do destructor. >> Similar should be done for construction (currently being in ipa_node_params_t::insert). > > Ah, that did not occur to me. Still, I must say that destructors > called by the garbage collector are tough to wrap one's head around. > Or at least my head. Mine too. It took me some time before I realized what's wrong :) > > I can't approve it but I like the patch, with a little nit... > >> >> Apart from that, I noticed that when using GGC memory, the machinery implicitly calls dtors for types >> that have __has_trivial_destructor == true. That's case of ipa_node_params_t, but as symbol_summary already >> calls a dtor before ggc_free is called. Problem comes when hash_table marks a memory slot as empty and GGC finalizer >> calls dtor for an invalid memory. Thus I believe not using such finalizer is proper fix. >> >> Last change fixes issue where ipa_free_all_node_params destroys a symbol_summary (living in GGC memory) and dtor >> is called for second time via ggc release mechanism. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Ready to be installed? >> Martin > >> From 08a3ecda47d9d52cd4bb6435aaf18b5b099ef683 Mon Sep 17 00:00:00 2001 >> From: marxin >> Date: Thu, 2 Feb 2017 11:13:13 +0100 >> Subject: [PATCH] Fix memory leaks in IPA CP (PR ipa/79337). >> >> gcc/ChangeLog: >> >> 2017-02-02 Martin Liska >> >> PR ipa/79337 >> * ipa-prop.c (ipa_node_params_t::insert): Remove current >> implementation. >> (ipa_node_params_t::remove): Likewise. >> * ipa-prop.h (ipa_node_params::ipa_node_params): Make default >> initialization from ipa_node_params_t::insert. >> (ipa_node_params::~ipa_node_params): Move from >> ipa_node_params_t::release. >> * symbol-summary.h (symbol_summary::m_released): New member. >> Do not release a summary twice. Do not allow to call finalizer >> for types of a summary that live in GGC memory. >> --- >> gcc/ipa-prop.c | 32 -------------------------------- >> gcc/ipa-prop.h | 28 ++++++++++++++++++++++++++-- >> gcc/symbol-summary.h | 27 ++++++++++++++------------- >> 3 files changed, 40 insertions(+), 47 deletions(-) >> > > ... > >> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h >> index 93a2390c321..8c5ba25fcec 100644 >> --- a/gcc/ipa-prop.h >> +++ b/gcc/ipa-prop.h >> @@ -320,6 +320,12 @@ struct GTY(()) ipa_param_descriptor >> >> struct GTY((for_user)) ipa_node_params >> { >> + /* Default constructor. */ >> + ipa_node_params (); >> + >> + /* Default destructor. */ >> + ~ipa_node_params (); >> + >> /* Information about individual formal parameters that are gathered when >> summaries are generated. */ >> vec *descriptors; >> @@ -356,6 +362,24 @@ struct GTY((for_user)) ipa_node_params >> unsigned versionable : 1; >> }; >> >> +inline >> +ipa_node_params::ipa_node_params () >> +: descriptors (NULL), lattices (NULL), ipcp_orig_node (NULL), >> + known_csts (vNULL), known_contexts (vNULL), analysis_done (0), >> + node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone (0), >> + node_dead (0), node_within_scc (0), node_calling_single_call (0), >> + versionable (0) >> +{ >> +} >> + >> +inline >> +ipa_node_params::~ipa_node_params () >> +{ >> + free (lattices); >> + known_csts.release (); >> + known_contexts.release (); >> +} >> + >> /* Intermediate information that we get from alias analysis about a particular >> parameter in a particular basic_block. When a parameter or the memory it >> references is marked modified, we use that information in all dominated >> @@ -580,9 +604,9 @@ public: >> function_summary (table, ggc) { } >> >> /* Hook that is called by summary when a node is deleted. */ >> - virtual void insert (cgraph_node *, ipa_node_params *info); >> + virtual void insert (cgraph_node *, ipa_node_params *) {} >> /* Hook that is called by summary when a node is deleted. */ >> - virtual void remove (cgraph_node *, ipa_node_params *info); >> + virtual void remove (cgraph_node *, ipa_node_params *) {} > > ...that these could be just removed, no? Yes. Changed in attached patch. Martin > > Thanks for looking into this, > > Martin > From 0086e44dc0ca43737db6e94c1f29cad903d6b39f Mon Sep 17 00:00:00 2001 From: marxin Date: Thu, 2 Feb 2017 11:13:13 +0100 Subject: [PATCH] Fix memory leaks in IPA CP (PR ipa/79337). gcc/ChangeLog: 2017-02-02 Martin Liska PR ipa/79337 * ipa-prop.c (ipa_node_params_t::insert): Remove current implementation. (ipa_node_params_t::remove): Likewise. * ipa-prop.h (ipa_node_params::ipa_node_params): Make default initialization from removed ipa_node_params_t::insert. (ipa_node_params::~ipa_node_params): Move from removed ipa_node_params_t::release. * symbol-summary.h (symbol_summary::m_released): New member. Do not release a summary twice. Do not allow to call finalizer for types of a summary that live in GGC memory. --- gcc/ipa-prop.c | 32 -------------------------------- gcc/ipa-prop.h | 28 ++++++++++++++++++++++++---- gcc/symbol-summary.h | 27 ++++++++++++++------------- 3 files changed, 38 insertions(+), 49 deletions(-) diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c index 3ef3d4fae9e..d031a70caa4 100644 --- a/gcc/ipa-prop.c +++ b/gcc/ipa-prop.c @@ -3736,38 +3736,6 @@ ipa_add_new_function (cgraph_node *node, void *data ATTRIBUTE_UNUSED) ipa_analyze_node (node); } -/* Initialize a newly created param info. */ - -void -ipa_node_params_t::insert (cgraph_node *, ipa_node_params *info) -{ - info->lattices = NULL; - info->ipcp_orig_node = NULL; - info->known_csts = vNULL; - info->known_contexts = vNULL; - info->analysis_done = 0; - info->node_enqueued = 0; - info->do_clone_for_all_contexts = 0; - info->is_all_contexts_clone = 0; - info->node_dead = 0; - info->node_within_scc = 0; - info->node_calling_single_call = 0; - info->versionable = 0; -} - -/* Frees all dynamically allocated structures that the param info points - to. */ - -void -ipa_node_params_t::remove (cgraph_node *, ipa_node_params *info) -{ - free (info->lattices); - /* Lattice values and their sources are deallocated with their alocation - pool. */ - info->known_csts.release (); - info->known_contexts.release (); -} - /* Hook that is called by summary when a node is duplicated. */ void diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 93a2390c321..8f7eb088813 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -320,6 +320,12 @@ struct GTY(()) ipa_param_descriptor struct GTY((for_user)) ipa_node_params { + /* Default constructor. */ + ipa_node_params (); + + /* Default destructor. */ + ~ipa_node_params (); + /* Information about individual formal parameters that are gathered when summaries are generated. */ vec *descriptors; @@ -356,6 +362,24 @@ struct GTY((for_user)) ipa_node_params unsigned versionable : 1; }; +inline +ipa_node_params::ipa_node_params () +: descriptors (NULL), lattices (NULL), ipcp_orig_node (NULL), + known_csts (vNULL), known_contexts (vNULL), analysis_done (0), + node_enqueued (0), do_clone_for_all_contexts (0), is_all_contexts_clone (0), + node_dead (0), node_within_scc (0), node_calling_single_call (0), + versionable (0) +{ +} + +inline +ipa_node_params::~ipa_node_params () +{ + free (lattices); + known_csts.release (); + known_contexts.release (); +} + /* Intermediate information that we get from alias analysis about a particular parameter in a particular basic_block. When a parameter or the memory it references is marked modified, we use that information in all dominated @@ -579,10 +603,6 @@ public: ipa_node_params_t (symbol_table *table, bool ggc): function_summary (table, ggc) { } - /* Hook that is called by summary when a node is deleted. */ - virtual void insert (cgraph_node *, ipa_node_params *info); - /* Hook that is called by summary when a node is deleted. */ - virtual void remove (cgraph_node *, ipa_node_params *info); /* Hook that is called by summary when a node is duplicated. */ virtual void duplicate (cgraph_node *node, cgraph_node *node2, diff --git a/gcc/symbol-summary.h b/gcc/symbol-summary.h index 1274be78083..3bcd14522c8 100644 --- a/gcc/symbol-summary.h +++ b/gcc/symbol-summary.h @@ -37,7 +37,8 @@ class GTY((user)) function_summary public: /* Default construction takes SYMTAB as an argument. */ function_summary (symbol_table *symtab, bool ggc = false): m_ggc (ggc), - m_map (13, ggc), m_insertion_enabled (true), m_symtab (symtab) + m_map (13, ggc), m_insertion_enabled (true), m_released (false), + m_symtab (symtab) { m_symtab_insertion_hook = symtab->add_cgraph_insertion_hook @@ -60,23 +61,19 @@ public: /* Destruction method that can be called for GGT purpose. */ void release () { - if (m_symtab_insertion_hook) - m_symtab->remove_cgraph_insertion_hook (m_symtab_insertion_hook); + if (m_released) + return; - if (m_symtab_removal_hook) - m_symtab->remove_cgraph_removal_hook (m_symtab_removal_hook); - - if (m_symtab_duplication_hook) - m_symtab->remove_cgraph_duplication_hook (m_symtab_duplication_hook); - - m_symtab_insertion_hook = NULL; - m_symtab_removal_hook = NULL; - m_symtab_duplication_hook = NULL; + m_symtab->remove_cgraph_insertion_hook (m_symtab_insertion_hook); + m_symtab->remove_cgraph_removal_hook (m_symtab_removal_hook); + m_symtab->remove_cgraph_duplication_hook (m_symtab_duplication_hook); /* Release all summaries. */ typedef typename hash_map ::iterator map_iterator; for (map_iterator it = m_map.begin (); it != m_map.end (); ++it) release ((*it).second); + + m_released = true; } /* Traverses all summarys with a function F called with @@ -99,7 +96,9 @@ public: /* Allocates new data that are stored within map. */ T* allocate_new () { - return m_ggc ? new (ggc_alloc ()) T() : new T () ; + /* Call gcc_internal_because we do not want to call finalizer for + a type T. We call dtor explicitly. */ + return m_ggc ? new (ggc_internal_alloc (sizeof (T))) T () : new T () ; } /* Release an item that is stored within map. */ @@ -216,6 +215,8 @@ private: cgraph_2node_hook_list *m_symtab_duplication_hook; /* Indicates if insertion hook is enabled. */ bool m_insertion_enabled; + /* Indicates if the summary is released. */ + bool m_released; /* Symbol table the summary is registered to. */ symbol_table *m_symtab; -- 2.11.0