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 <mli...@suse.cz> >> 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 <mli...@suse.cz> >> >> 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<ipa_param_descriptor, va_gc> *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<ipa_node_params *> (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 <mli...@suse.cz> 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 <mli...@suse.cz> 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<ipa_param_descriptor, va_gc> *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<ipa_node_params *> (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 <T *> 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 <map_hash, T *>::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> ()) 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