diff mbox

Fix memory leaks in IPA CP (PR ipa/79337).

Message ID d6286fa9-6199-6650-7167-dad3f7780372@suse.cz
State New
Headers show

Commit Message

Martin Liška Feb. 2, 2017, 12:53 p.m. UTC 
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).

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

Comments

Martin Jambor Feb. 2, 2017, 4:13 p.m. UTC | #1 
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 <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?

Thanks for looking into this,

Martin
diff mbox

Patch

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.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..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 *) {}
   /* 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