[Bug ipa/97346] New: IPA reference reference_vars_to_consider leaks

[Bug ipa/97346] New: IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

            Bug ID: 97346
           Summary: IPA reference reference_vars_to_consider leaks
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---

==20551== 96 (8 direct, 88 indirect) bytes in 1 blocks are definitely lost in
loss record 535 of 730
==20551==    at 0x4C2E94F: operator new(unsigned long) (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==20551==    by 0x14A6E6B: void vec_alloc<tree_node*>(vec<tree_node*, va_heap,
vl_ptr>*&, unsigned int) (vec.h:1567)
==20551==    by 0x24BB41F: ipa_init() (ipa-reference.c:461)
==20551==    by 0x24BB896: generate_summary() (ipa-reference.c:597)
==20551==    by 0x24BC18A: propagate() (ipa-reference.c:768)
==20551==    by 0x24BD88C: (anonymous
namespace)::pass_ipa_reference::execute(function*) (ipa-reference.c:1299)
==20551==    by 0x121A924: execute_one_pass(opt_pass*) (passes.c:2509)
==20551==    by 0x121B893: execute_ipa_pass_list(opt_pass*) (passes.c:2936)
==20551==    by 0xC6ED1D: ipa_passes() (cgraphunit.c:2700)

the use of this global is quite odd, with two allocations, one release
site using just 'delete' and one conditional freeing the vector but
unconditionally NULLing the pointer to it.

I wonder if the array should be made local to a few functions to make
lifetime more obvious.

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
At least

diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 4a6c011c525..6df4be09471 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -1114,7 +1113,8 @@ ipa_reference_write_optimization_summary (void)
          }
       }
   lto_destroy_simple_output_block (ob);
-  delete reference_vars_to_consider;
+  vec_free (reference_vars_to_consider);
+  reference_vars_to_consider = NULL;
 }

 /* Deserialize the ipa info for lto.  */

is maybe obvious.  Then there's

diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 4a6c011c525..f0555929340 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -1056,6 +1056,7 @@ ipa_reference_write_optimization_summary (void)
   auto_bitmap ltrans_statics;
   int i;

+  gcc_assert (!reference_vars_to_consider);
   vec_alloc (reference_vars_to_consider, ipa_reference_vars_uids);
   reference_vars_to_consider->safe_grow (ipa_reference_vars_uids, true);


that will likely trip because of the ipa_init allocation.  And

diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 4a6c011c525..93b2b677a76 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -964,8 +964,10 @@ propagate (void)
     }

   if (dump_file)
-    vec_free (reference_vars_to_consider);
-  reference_vars_to_consider = NULL;
+    {
+      vec_free (reference_vars_to_consider);
+      reference_vars_to_consider = NULL;
+    }
   return remove_p ? TODO_remove_functions : 0;
 }


anyway, it all looks like a mess ;)

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Ping.  This is annoying and pops up with each and every valgrind
--leak-check=full ...

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[hubicka at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-02-08
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |hubicka at gcc dot gnu.org

--- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I will check

If I recall correctly, during analysis the vector is only collected for dumps,
so that is why vec_free is conditional. Since vec_free is noop on NULL, we
could just free it anyway.  I think it is the delete call that causes the leak.

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
So like the following?  Note the leak is the allcoation from ipa_init
being not released when we do the vec_alloc in
ipa_reference_write_optimization_summary (maybe this function wants to
use its own private vector?!)

diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 2ea2a6d5327..328fa8f732c 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -966,8 +966,7 @@ propagate (void)
       ipa_ref_var_info_summaries = NULL;
     }

-  if (dump_file)
-    vec_free (reference_vars_to_consider);
+  vec_free (reference_vars_to_consider);
   reference_vars_to_consider = NULL;
   return remove_p ? TODO_remove_functions : 0;
 }
@@ -1059,7 +1058,7 @@ ipa_reference_write_optimization_summary (void)
   auto_bitmap ltrans_statics;
   int i;

-  vec_alloc (reference_vars_to_consider, ipa_reference_vars_uids);
+  vec_truncate (reference_vars_to_consider, 0);
   reference_vars_to_consider->safe_grow (ipa_reference_vars_uids, true);

   /* See what variables we are interested in.  */
@@ -1117,7 +1116,8 @@ ipa_reference_write_optimization_summary (void)
          }
       }
   lto_destroy_simple_output_block (ob);
-  delete reference_vars_to_consider;
+  vec_free (reference_vars_to_consider);
+  reference_vars_to_consider = NULL;
 }

 /* Deserialize the ipa info for lto.  */

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> So like the following?  Note the leak is the allcoation from ipa_init
> being not released when we do the vec_alloc in
> ipa_reference_write_optimization_summary (maybe this function wants to
> use its own private vector?!)
> 
> diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
> index 2ea2a6d5327..328fa8f732c 100644
> --- a/gcc/ipa-reference.c
> +++ b/gcc/ipa-reference.c
> @@ -966,8 +966,7 @@ propagate (void)
>        ipa_ref_var_info_summaries = NULL;
>      }
>  
> -  if (dump_file)
> -    vec_free (reference_vars_to_consider);
> +  vec_free (reference_vars_to_consider);
>    reference_vars_to_consider = NULL;
>    return remove_p ? TODO_remove_functions : 0;
>  }
> @@ -1059,7 +1058,7 @@ ipa_reference_write_optimization_summary (void)
>    auto_bitmap ltrans_statics;
>    int i;
>  
> -  vec_alloc (reference_vars_to_consider, ipa_reference_vars_uids);
> +  vec_truncate (reference_vars_to_consider, 0);

vec_safe_truncate

>    reference_vars_to_consider->safe_grow (ipa_reference_vars_uids, true);
>  
>    /* See what variables we are interested in.  */
> @@ -1117,7 +1116,8 @@ ipa_reference_write_optimization_summary (void)
>           }
>        }
>    lto_destroy_simple_output_block (ob);
> -  delete reference_vars_to_consider;
> +  vec_free (reference_vars_to_consider);
> +  reference_vars_to_consider = NULL;
>  }
>  
>  /* Deserialize the ipa info for lto.  */

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
I'm testing the following which fixes the leak(s)

diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 2ea2a6d5327..a6b79fb9381 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -966,8 +966,7 @@ propagate (void)
       ipa_ref_var_info_summaries = NULL;
     }

-  if (dump_file)
-    vec_free (reference_vars_to_consider);
+  vec_free (reference_vars_to_consider);
   reference_vars_to_consider = NULL;
   return remove_p ? TODO_remove_functions : 0;
 }
@@ -1059,6 +1058,7 @@ ipa_reference_write_optimization_summary (void)
   auto_bitmap ltrans_statics;
   int i;

+  vec_free (reference_vars_to_consider);
   vec_alloc (reference_vars_to_consider, ipa_reference_vars_uids);
   reference_vars_to_consider->safe_grow (ipa_reference_vars_uids, true);

@@ -1117,7 +1117,8 @@ ipa_reference_write_optimization_summary (void)
          }
       }
   lto_destroy_simple_output_block (ob);
-  delete reference_vars_to_consider;
+  vec_free (reference_vars_to_consider);
+  reference_vars_to_consider = NULL;
 }

 /* Deserialize the ipa info for lto.  */

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[hubicka at ucw dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #7 from Jan Hubicka <hubicka at ucw dot cz> ---
I tested yesterday this one (which makes the lifetime bit more explicit
- during propagation it is for dumps only). Sorry for not posting it
earlier. I just wanted to double check tha tleak is gone.

diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
index 2ea2a6d5327..84c018ff57c 100644
--- a/gcc/ipa-reference.c
+++ b/gcc/ipa-reference.c
@@ -458,8 +458,8 @@ ipa_init (void)

   ipa_init_p = true;

-  vec_alloc (reference_vars_to_consider, 10);
-
+  if (dump_file)
+    vec_alloc (reference_vars_to_consider, 10);

   if (ipa_ref_opt_sum_summaries != NULL)
     {
@@ -967,8 +967,12 @@ propagate (void)
     }

   if (dump_file)
-    vec_free (reference_vars_to_consider);
-  reference_vars_to_consider = NULL;
+    {
+      vec_free (reference_vars_to_consider);
+      reference_vars_to_consider = NULL;
+    }
+  else
+    gcc_checking_assert (!reference_vars_to_consider);
   return remove_p ? TODO_remove_functions : 0;
 }

@@ -1059,6 +1063,7 @@ ipa_reference_write_optimization_summary (void)
   auto_bitmap ltrans_statics;
   int i;

+  gcc_checking_assert (!reference_vars_to_consider);
   vec_alloc (reference_vars_to_consider, ipa_reference_vars_uids);
   reference_vars_to_consider->safe_grow (ipa_reference_vars_uids, true);

@@ -1117,7 +1122,7 @@ ipa_reference_write_optimization_summary (void)
          }
       }
   lto_destroy_simple_output_block (ob);
-  delete reference_vars_to_consider;
+  vec_free (reference_vars_to_consider);
 }

 /* Deserialize the ipa info for lto.  */

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #7)
> I tested yesterday this one (which makes the lifetime bit more explicit
> - during propagation it is for dumps only). Sorry for not posting it
> earlier. I just wanted to double check tha tleak is gone.
> 
> diff --git a/gcc/ipa-reference.c b/gcc/ipa-reference.c
> index 2ea2a6d5327..84c018ff57c 100644
> --- a/gcc/ipa-reference.c
> +++ b/gcc/ipa-reference.c
> @@ -458,8 +458,8 @@ ipa_init (void)
>  
>    ipa_init_p = true;
>  
> -  vec_alloc (reference_vars_to_consider, 10);
> -
> +  if (dump_file)
> +    vec_alloc (reference_vars_to_consider, 10);
>  
>    if (ipa_ref_opt_sum_summaries != NULL)
>      {
> @@ -967,8 +967,12 @@ propagate (void)
>      }
>  
>    if (dump_file)
> -    vec_free (reference_vars_to_consider);
> -  reference_vars_to_consider = NULL;
> +    {
> +      vec_free (reference_vars_to_consider);
> +      reference_vars_to_consider = NULL;
> +    }
> +  else
> +    gcc_checking_assert (!reference_vars_to_consider);
>    return remove_p ? TODO_remove_functions : 0;
>  }
>  
> @@ -1059,6 +1063,7 @@ ipa_reference_write_optimization_summary (void)
>    auto_bitmap ltrans_statics;
>    int i;
>  
> +  gcc_checking_assert (!reference_vars_to_consider);
>    vec_alloc (reference_vars_to_consider, ipa_reference_vars_uids);
>    reference_vars_to_consider->safe_grow (ipa_reference_vars_uids, true);
>  
> @@ -1117,7 +1122,7 @@ ipa_reference_write_optimization_summary (void)
>  	  }
>        }
>    lto_destroy_simple_output_block (ob);
> -  delete reference_vars_to_consider;
> +  vec_free (reference_vars_to_consider);

maybe set reference_vars_to_consider to NULL here for consistency,
otherwise also looks good to me!

>  }
>  
>  /* Deserialize the ipa info for lto.  */

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jan Hubicka <hubicka@gcc.gnu.org>:

https://gcc.gnu.org/g:9966699d7a9d8e35c0c4cf9a945bcf90ef874f2d

commit r11-7240-g9966699d7a9d8e35c0c4cf9a945bcf90ef874f2d
Author: Jan Hubicka <jh@suse.cz>
Date:   Sun Feb 14 23:24:44 2021 +0100

    Fix memory leak in ipa-refernece

    2021-02-14  Jan Hubicka  <hubicka@ucw.cz>
                Richard Biener  <rguether@suse.de>

            PR ipa/97346
            * ipa-reference.c (ipa_init): Only conditinally initialize
            reference_vars_to_consider.
            (propagate): Conditionally deninitialize
reference_vars_to_consider.
            (ipa_reference_write_optimization_summary): Sanity check that
            reference_vars_to_consider is not allocated.

[Bug ipa/97346] IPA reference reference_vars_to_consider leaks

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97346

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.