This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix memory leaks in IPA CP (PR ipa/79337).
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
>
>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