| Message ID | 65d45d04-19d2-7c07-2412-f19983800dd4@suse.cz |
|---|---|
| State | New |
| Headers | show |
On 02/02/2017 09:37 AM, Martin Liška wrote: > 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 <mliska@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 <mliska@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 >> > > 0001-Fix-memory-leaks-in-IPA-CP-PR-ipa-79337-v2.patch > > > From 0086e44dc0ca43737db6e94c1f29cad903d6b39f Mon Sep 17 00:00:00 2001 > From: marxin <mliska@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 <mliska@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. OK. jeff
From 0086e44dc0ca43737db6e94c1f29cad903d6b39f Mon Sep 17 00:00:00 2001 From: marxin <mliska@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 <mliska@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