[PATCH] Fix memory leak in tree-inliner

[PATCH] Fix memory leak in tree-inliner

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

Hi.

I've spotted couple of occurrences of following memory leak seen by valgrind:

  malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
  operator new(unsigned long) (new_op.cc:50)
  remap_dependence_clique(copy_body_data*, unsigned short) (tree-inline.c:845)
  remap_gimple_op_r(tree_node**, int*, void*) (tree-inline.c:954)
  walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11498)
  walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11815)
  walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11815)
  walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11815)
  copy_debug_stmt (tree-inline.c:2869)
  copy_debug_stmts (tree-inline.c:2927)
  copy_body(copy_body_data*, long, int, basic_block_def*, basic_block_def*, basic_block_def*) (tree-inline.c:2961)
  tree_function_versioning(tree_node*, tree_node*, vec<ipa_replace_map*, va_gc, vl_embed>*, bool, bitmap_head*, bool, bitmap_head*, basic_block_def*) (tree-inline.c:5907)
  save_inline_function_body (ipa-inline-transform.c:485)
  inline_transform(cgraph_node*) (ipa-inline-transform.c:541)

Problem is that the id->dependence_map is released before copy_debug_stmts is called.

Patch can bootstrap and survives regression tests on x86_64-linux-gnu.
Ready for trunk?

Martin

[-- Attachment #2: 0001-Properly-release-memory-in-copy_body.patch --]
[-- Type: text/x-patch, Size: 1323 bytes --]

From c67fe154a24b939edad28a823c2ecb16f6e056c1 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 6 Jan 2016 11:41:31 +0100
Subject: [PATCH] Properly release memory in copy_body

gcc/ChangeLog:

2016-01-06  Martin Liska  <mliska@suse.cz>

	* tree-inline.c (copy_cfg_body): Remove eh_map deletion.
	(copy_body): Release it here.
---
 gcc/tree-inline.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 19f202e..92169e6 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -2807,17 +2807,6 @@ copy_cfg_body (copy_body_data * id, gcov_type count, int frequency_scale,
   entry_block_map->aux = NULL;
   exit_block_map->aux = NULL;
 
-  if (id->eh_map)
-    {
-      delete id->eh_map;
-      id->eh_map = NULL;
-    }
-  if (id->dependence_map)
-    {
-      delete id->dependence_map;
-      id->dependence_map = NULL;
-    }
-
   return new_fndecl;
 }
 
@@ -2963,6 +2952,17 @@ copy_body (copy_body_data *id, gcov_type count, int frequency_scale,
 			new_entry);
   copy_debug_stmts (id);
 
+  if (id->eh_map)
+    {
+      delete id->eh_map;
+      id->eh_map = NULL;
+    }
+  if (id->dependence_map)
+    {
+      delete id->dependence_map;
+      id->dependence_map = NULL;
+    }
+
   return body;
 }
 
-- 
2.8.1

Re: [PATCH] Fix memory leak in tree-inliner

[Richard Biener]

On Fri, May 6, 2016 at 12:10 PM, Martin Liška <mliska@suse.cz> wrote:
> Hi.
>
> I've spotted couple of occurrences of following memory leak seen by valgrind:
>
>   malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>   operator new(unsigned long) (new_op.cc:50)
>   remap_dependence_clique(copy_body_data*, unsigned short) (tree-inline.c:845)
>   remap_gimple_op_r(tree_node**, int*, void*) (tree-inline.c:954)
>   walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11498)
>   walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11815)
>   walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11815)
>   walk_tree_1(tree_node**, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*, tree_node* (*)(tree_node**, int*, tree_node* (*)(tree_node**, int*, void*), void*, hash_set<tree_node*, default_hash_traits<tree_node*> >*)) (tree.c:11815)
>   copy_debug_stmt (tree-inline.c:2869)
>   copy_debug_stmts (tree-inline.c:2927)
>   copy_body(copy_body_data*, long, int, basic_block_def*, basic_block_def*, basic_block_def*) (tree-inline.c:2961)
>   tree_function_versioning(tree_node*, tree_node*, vec<ipa_replace_map*, va_gc, vl_embed>*, bool, bitmap_head*, bool, bitmap_head*, basic_block_def*) (tree-inline.c:5907)
>   save_inline_function_body (ipa-inline-transform.c:485)
>   inline_transform(cgraph_node*) (ipa-inline-transform.c:541)
>
> Problem is that the id->dependence_map is released before copy_debug_stmts is called.
>
> Patch can bootstrap and survives regression tests on x86_64-linux-gnu.
> Ready for trunk?

Hmmm.  But this means debug stmt remapping calls
remap_dependence_clique which may end up bumping
cfun->last_clique and thus may change code generation.

So what debug stmts contain MEM_REFs?  If you put an assert
processing_debug_stmt == 0 in
remap_dependence_clique I'd like to see a testcase that triggers it.

Richard.

> Martin

Re: [PATCH] Fix memory leak in tree-inliner

[Martin Liška]

On 05/06/2016 12:56 PM, Richard Biener wrote:
> Hmmm.  But this means debug stmt remapping calls
> remap_dependence_clique which may end up bumping
> cfun->last_clique and thus may change code generation.
> 
> So what debug stmts contain MEM_REFs?  If you put an assert
> processing_debug_stmt == 0 in
> remap_dependence_clique I'd like to see a testcase that triggers it.
> 
> Richard.

Ok, I've placed the suggested assert which is triggered for following debug statement:

(gdb) p debug_gimple_stmt(stmt)
# DEBUG D#21 => a_1(D)->dim[0].ubound

(gdb) p debug_tree(*tp)
 <mem_ref 0x7ffff66c1b40
    type <record_type 0x7ffff6a642a0 array1_unknown type_1 BLK
        size <integer_cst 0x7ffff6a46180 constant 384>
        unit size <integer_cst 0x7ffff6a23c90 constant 48>
        align 64 symtab -160828560 alias set -1 canonical type 0x7ffff6a4f000
        fields <field_decl 0x7ffff6a47980 data type <pointer_type 0x7ffff68a7348>
            unsigned DI file /home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/actual_array_constructor_1.f90 line 21 col 0
            size <integer_cst 0x7ffff6886bd0 constant 64>
            unit size <integer_cst 0x7ffff6886be8 constant 8>
            align 64 offset_align 128
            offset <integer_cst 0x7ffff6886c00 constant 0>
            bit offset <integer_cst 0x7ffff6886c48 constant 0> context <record_type 0x7ffff6a4f498 array_descriptor1> chain <field_decl 0x7ffff6a47a18 offset>>
        pointer_to_this <pointer_type 0x7ffff6a64540> reference_to_this <reference_type 0x7ffff6a645e8> chain <type_decl 0x7ffff6a511c8 D.3431>>
   
    arg 0 <ssa_name 0x7ffff66b41f8
        type <reference_type 0x7ffff6a64690 type <record_type 0x7ffff6a642a0 array1_unknown>
            public unsigned restrict DI size <integer_cst 0x7ffff6886bd0 64> unit size <integer_cst 0x7ffff6886be8 8>
            align 64 symtab 0 alias set -1 canonical type 0x7ffff6a53150>
        var <parm_decl 0x7ffff6696800 a>def_stmt GIMPLE_NOP

        version 1>
    arg 1 <integer_cst 0x7ffff6a7a438 type <reference_type 0x7ffff6a64690> constant 0>>

for the following test-case:
gfortran /home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/actual_array_constructor_1.f90 -O3 -g

Martin

Re: [PATCH] Fix memory leak in tree-inliner

[Richard Biener]

On Fri, May 6, 2016 at 2:35 PM, Martin Liška <mliska@suse.cz> wrote:
> On 05/06/2016 12:56 PM, Richard Biener wrote:
>> Hmmm.  But this means debug stmt remapping calls
>> remap_dependence_clique which may end up bumping
>> cfun->last_clique and thus may change code generation.
>>
>> So what debug stmts contain MEM_REFs?  If you put an assert
>> processing_debug_stmt == 0 in
>> remap_dependence_clique I'd like to see a testcase that triggers it.
>>
>> Richard.
>
> Ok, I've placed the suggested assert which is triggered for following debug statement:
>
> (gdb) p debug_gimple_stmt(stmt)
> # DEBUG D#21 => a_1(D)->dim[0].ubound
>
> (gdb) p debug_tree(*tp)
>  <mem_ref 0x7ffff66c1b40
>     type <record_type 0x7ffff6a642a0 array1_unknown type_1 BLK
>         size <integer_cst 0x7ffff6a46180 constant 384>
>         unit size <integer_cst 0x7ffff6a23c90 constant 48>
>         align 64 symtab -160828560 alias set -1 canonical type 0x7ffff6a4f000
>         fields <field_decl 0x7ffff6a47980 data type <pointer_type 0x7ffff68a7348>
>             unsigned DI file /home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/actual_array_constructor_1.f90 line 21 col 0
>             size <integer_cst 0x7ffff6886bd0 constant 64>
>             unit size <integer_cst 0x7ffff6886be8 constant 8>
>             align 64 offset_align 128
>             offset <integer_cst 0x7ffff6886c00 constant 0>
>             bit offset <integer_cst 0x7ffff6886c48 constant 0> context <record_type 0x7ffff6a4f498 array_descriptor1> chain <field_decl 0x7ffff6a47a18 offset>>
>         pointer_to_this <pointer_type 0x7ffff6a64540> reference_to_this <reference_type 0x7ffff6a645e8> chain <type_decl 0x7ffff6a511c8 D.3431>>
>
>     arg 0 <ssa_name 0x7ffff66b41f8
>         type <reference_type 0x7ffff6a64690 type <record_type 0x7ffff6a642a0 array1_unknown>
>             public unsigned restrict DI size <integer_cst 0x7ffff6886bd0 64> unit size <integer_cst 0x7ffff6886be8 8>
>             align 64 symtab 0 alias set -1 canonical type 0x7ffff6a53150>
>         var <parm_decl 0x7ffff6696800 a>def_stmt GIMPLE_NOP
>
>         version 1>
>     arg 1 <integer_cst 0x7ffff6a7a438 type <reference_type 0x7ffff6a64690> constant 0>>
>
> for the following test-case:
> gfortran /home/marxin/Programming/gcc/gcc/testsuite/gfortran.dg/actual_array_constructor_1.f90 -O3 -g

Ok.  I suggest you instead do sth like

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c   (revision 236021)
+++ gcc/tree-inline.c   (working copy)
@@ -840,7 +840,7 @@ is_parm (tree decl)
 static unsigned short
 remap_dependence_clique (copy_body_data *id, unsigned short clique)
 {
-  if (clique == 0)
+  if (clique == 0 || processing_debug_stmt)
     return 0;
   if (!id->dependence_map)
     id->dependence_map = new hash_map<dependence_hash, unsigned short>;

does that resolve the issue?  If so, this is ok for trunk and branches.

Thanks,
Richard.

> Martin
>
>
>

Re: [PATCH] Fix memory leak in tree-inliner

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

On 05/09/2016 12:57 PM, Richard Biener wrote:
> does that resolve the issue?  If so, this is ok for trunk and branches.

It does. I'll commit the patch to trunk after it finishes regression tests.
Related patches for branches will be prepared after that.

Martin

[-- Attachment #2: 0001-Handle-memory-leak-in-tree-inline.c.patch --]
[-- Type: text/x-patch, Size: 867 bytes --]

From 53c0a7fe057fd2ddd1ab4cea1d5ce47bba6dfa0b Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Mon, 9 May 2016 16:02:15 +0200
Subject: [PATCH] Handle memory leak in tree-inline.c.

gcc/ChangeLog:

2016-01-06  Martin Liska  <mliska@suse.cz>

	* tree-inline.c (remap_dependence_clique): Do not remap
	debugging statements.
---
 gcc/tree-inline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 2ee3f63..e571140 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -840,7 +840,7 @@ is_parm (tree decl)
 static unsigned short
 remap_dependence_clique (copy_body_data *id, unsigned short clique)
 {
-  if (clique == 0)
+  if (clique == 0 || processing_debug_stmt)
     return 0;
   if (!id->dependence_map)
     id->dependence_map = new hash_map<dependence_hash, unsigned short>;
-- 
2.8.1