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.
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?
Thanks for looking into this,
Martin
