Free memory used by optimization/target options

Message ID 20191105104043.ivy62bbgp7776uoy@kam.mff.cuni.cz
State New
Headers show
Series
  • Free memory used by optimization/target options
Related show

Commit Message

Jan Hubicka Nov. 5, 2019, 10:40 a.m.
Hi,
this fixes memory leak for xstrduped strings in option summaries which may
get ggc_freed by tree merging.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* optc-save-gen.awk: Generate cl_target_option_free
	and cl_optimization_option_free.
	* opth-en.awk: Declare cl_target_option_free
	and cl_optimization_option_free.
	* tree.c (free_node): Use it.

Comments

Richard Biener Nov. 5, 2019, 10:46 a.m. | #1
On Tue, 5 Nov 2019, Jan Hubicka wrote:

> Hi,

> this fixes memory leak for xstrduped strings in option summaries which may

> get ggc_freed by tree merging.

> 

> Bootstrapped/regtested x86_64-linux, OK?


OK.  If those are ever reaped by regular GC the memory still leaks, no?
So wouldn't it be better to put the strings into GC memory?

Thanks,
Richard.

> Honza

> 

> 	* optc-save-gen.awk: Generate cl_target_option_free

> 	and cl_optimization_option_free.

> 	* opth-en.awk: Declare cl_target_option_free

> 	and cl_optimization_option_free.

> 	* tree.c (free_node): Use it.

> Index: optc-save-gen.awk

> ===================================================================

> --- optc-save-gen.awk	(revision 277796)

> +++ optc-save-gen.awk	(working copy)

> @@ -802,6 +802,17 @@ for (i = 0; i < n_target_val; i++) {

>  

>  print "}";

>  

> +print "/* free heap memory used by target options  */";

> +print "void";

> +print "cl_target_option_free (struct cl_target_option *ptr ATTRIBUTE_UNUSED)";

> +print "{";

> +for (i = 0; i < n_target_str; i++) {

> +	name = var_target_str[i]

> +	print "  if (ptr->" name")";

> +	print "    free (const_cast <char *>(ptr->" name"));";

> +}

> +print "}";

> +

>  n_opt_val = 4;

>  var_opt_val[0] = "x_optimize"

>  var_opt_val_type[0] = "char "

> @@ -921,4 +932,18 @@ for (i = 0; i < n_opt_val; i++) {

>  	      print "  ptr->" name" = (" var_opt_val_type[i] ") bp_unpack_value (bp, 64);";

>  }

>  print "}";

> +print "/* Free heap memory used by optimization options  */";

> +print "void";

> +print "cl_optimization_option_free (struct cl_optimization *ptr ATTRIBUTE_UNUSED)";

> +print "{";

> +for (i = 0; i < n_opt_val; i++) {

> +	name = var_opt_val[i]

> +	otype = var_opt_val_type[i];

> +	if (otype ~ "^const char \\**$")

> +	{

> +	      print "  if (ptr->" name")";

> +	      print "    free (const_cast <char *>(ptr->" name"));";

> +	}

> +}

> +print "}";

>  }

> Index: opth-gen.awk

> ===================================================================

> --- opth-gen.awk	(revision 277796)

> +++ opth-gen.awk	(working copy)

> @@ -303,6 +303,9 @@ print "";

>  print "/* Compare two target option variables from a structure.  */";

>  print "extern bool cl_target_option_eq (const struct cl_target_option *, const struct cl_target_option *);";

>  print "";

> +print "/* Free heap memory used by target option variables.  */";

> +print "extern void cl_target_option_free (struct cl_target_option *);";

> +print "";

>  print "/* Hash option variables from a structure.  */";

>  print "extern hashval_t cl_target_option_hash (const struct cl_target_option *);";

>  print "";

> @@ -312,6 +315,9 @@ print "";

>  print "/* Compare two optimization options.  */";

>  print "extern bool cl_optimization_option_eq (cl_optimization const *ptr1, cl_optimization const *ptr2);"

>  print "";

> +print "/* Free heap memory used by optimization options.  */";

> +print "extern void cl_optimization_option_free (cl_optimization *ptr1);"

> +print "";

>  print "/* Generator files may not have access to location_t, and don't need these.  */"

>  print "#if defined(UNKNOWN_LOCATION)"

>  print "bool                                                                  "

> Index: tree.c

> ===================================================================

> --- tree.c	(revision 277796)

> +++ tree.c	(working copy)

> @@ -1170,6 +1170,10 @@ free_node (tree node)

>      vec_free (BLOCK_NONLOCALIZED_VARS (node));

>    else if (code == TREE_BINFO)

>      vec_free (BINFO_BASE_ACCESSES (node));

> +  else if (code == OPTIMIZATION_NODE)

> +    cl_optimization_option_free (TREE_OPTIMIZATION (node));

> +  else if (code == TARGET_OPTION_NODE)

> +    cl_target_option_free (TREE_TARGET_OPTION (node));

>    ggc_free (node);

>  }

>  

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
Jan Hubicka Nov. 5, 2019, 10:49 a.m. | #2
> On Tue, 5 Nov 2019, Jan Hubicka wrote:

> 

> > Hi,

> > this fixes memory leak for xstrduped strings in option summaries which may

> > get ggc_freed by tree merging.

> > 

> > Bootstrapped/regtested x86_64-linux, OK?

> 

> OK.  If those are ever reaped by regular GC the memory still leaks, no?

> So wouldn't it be better to put the strings into GC memory?

I am not sure why those are malloced, but I think there was some fun
with gengtype/awk generator interactions. Also I think all those option
nodes come into cl_option_hash that is fully permanent.

Honza
Martin Liška Nov. 6, 2019, 9:22 a.m. | #3
On 11/5/19 11:40 AM, Jan Hubicka wrote:
> +	print "  if (ptr->" name")";

> +	print "    free (const_cast <char *>(ptr->" name"));";


If I'm correct, you can call free even for a NULL pointer.

Martin
Jeff Law Nov. 6, 2019, 5:48 p.m. | #4
On 11/6/19 2:22 AM, Martin Liška wrote:
> On 11/5/19 11:40 AM, Jan Hubicka wrote:

>> +    print "  if (ptr->" name")";

>> +    print "    free (const_cast <char *>(ptr->" name"));";

> 

> If I'm correct, you can call free even for a NULL pointer.

You can and I think we expunged all the NULL tests before calling free
years ago.

jeff

Patch

Index: optc-save-gen.awk
===================================================================
--- optc-save-gen.awk	(revision 277796)
+++ optc-save-gen.awk	(working copy)
@@ -802,6 +802,17 @@  for (i = 0; i < n_target_val; i++) {
 
 print "}";
 
+print "/* free heap memory used by target options  */";
+print "void";
+print "cl_target_option_free (struct cl_target_option *ptr ATTRIBUTE_UNUSED)";
+print "{";
+for (i = 0; i < n_target_str; i++) {
+	name = var_target_str[i]
+	print "  if (ptr->" name")";
+	print "    free (const_cast <char *>(ptr->" name"));";
+}
+print "}";
+
 n_opt_val = 4;
 var_opt_val[0] = "x_optimize"
 var_opt_val_type[0] = "char "
@@ -921,4 +932,18 @@  for (i = 0; i < n_opt_val; i++) {
 	      print "  ptr->" name" = (" var_opt_val_type[i] ") bp_unpack_value (bp, 64);";
 }
 print "}";
+print "/* Free heap memory used by optimization options  */";
+print "void";
+print "cl_optimization_option_free (struct cl_optimization *ptr ATTRIBUTE_UNUSED)";
+print "{";
+for (i = 0; i < n_opt_val; i++) {
+	name = var_opt_val[i]
+	otype = var_opt_val_type[i];
+	if (otype ~ "^const char \\**$")
+	{
+	      print "  if (ptr->" name")";
+	      print "    free (const_cast <char *>(ptr->" name"));";
+	}
+}
+print "}";
 }
Index: opth-gen.awk
===================================================================
--- opth-gen.awk	(revision 277796)
+++ opth-gen.awk	(working copy)
@@ -303,6 +303,9 @@  print "";
 print "/* Compare two target option variables from a structure.  */";
 print "extern bool cl_target_option_eq (const struct cl_target_option *, const struct cl_target_option *);";
 print "";
+print "/* Free heap memory used by target option variables.  */";
+print "extern void cl_target_option_free (struct cl_target_option *);";
+print "";
 print "/* Hash option variables from a structure.  */";
 print "extern hashval_t cl_target_option_hash (const struct cl_target_option *);";
 print "";
@@ -312,6 +315,9 @@  print "";
 print "/* Compare two optimization options.  */";
 print "extern bool cl_optimization_option_eq (cl_optimization const *ptr1, cl_optimization const *ptr2);"
 print "";
+print "/* Free heap memory used by optimization options.  */";
+print "extern void cl_optimization_option_free (cl_optimization *ptr1);"
+print "";
 print "/* Generator files may not have access to location_t, and don't need these.  */"
 print "#if defined(UNKNOWN_LOCATION)"
 print "bool                                                                  "
Index: tree.c
===================================================================
--- tree.c	(revision 277796)
+++ tree.c	(working copy)
@@ -1170,6 +1170,10 @@  free_node (tree node)
     vec_free (BLOCK_NONLOCALIZED_VARS (node));
   else if (code == TREE_BINFO)
     vec_free (BINFO_BASE_ACCESSES (node));
+  else if (code == OPTIMIZATION_NODE)
+    cl_optimization_option_free (TREE_OPTIMIZATION (node));
+  else if (code == TARGET_OPTION_NODE)
+    cl_target_option_free (TREE_TARGET_OPTION (node));
   ggc_free (node);
 }