[rfc] Plug memory leaks during gdbarch initialization

[rfc] Plug memory leaks during gdbarch initialization

[Daniel Jacobowitz]

This patch fixes a number of memory leaks I found with valgrind.  There
are more, but this should cover everything that gets repeated for each
gdbarch.

Some of these are simple omissions, but one in particular requires
care: we no longer call build_gdbtypes to build the per-gdbarch
types during _initialize_gdbtypes.  Two targets that I spotted relied
on this, being i386-tdep.c and spu-tdep.c; they constructed new SIMD
types based on builtin_type_v2_int and other standard vectors, before
the vectors were guaranteed to be available.  I moved them to be
per-gdbarch like the types they depend on.

Of course while doing this I realized that there's still a problem.
Now they are created during gdbarch initialization, but the only reason
it used to work still applies: alphabetical order.  gdbtypes.c's
constructor still gets called before i386-tdep.c's because it was
registered first.

The patch is an improvement, but doesn't fix that wart.  I don't really
want to check it in without fixing the underlying problem but I don't
see a good way... anyone have a suggestion?

-- 
Daniel Jacobowitz
CodeSourcery

2007-01-23  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdbtypes.c (build_gdbtypes): Move registration of
	opaque-type-resolution to ...
	(_initialize_gdbtypes): ... here.  Don't call build_gdbtypes.
	Register swaps for builtin_type_true_char, builtin_type_bool,
	and builtin_type_vec64.
	* parse.c (_initialize_parse): Don't call build_parse.  Register
	swap for msym_tls_symbol_type.
	* f-lang.c (_initialize_f_language): Don't call build_fortran_types.
	Don't create or register builtin_type_string.
	* i386-tdep.c (_initialize_i386_tdep): Don't call i386_init_types.
	Register swaps for builtin types.
	* remote.c (add_packet_config_cmd): Free allocated strings.
	* spu-tdep.c (_initialize_spu_tdep): Don't call spu_init_vector_type.
	Register swap for builtin type.

---
 gdb/f-lang.c    |    8 --------
 gdb/gdbtypes.c  |   25 +++++++++++++------------
 gdb/i386-tdep.c |    7 ++++++-
 gdb/parse.c     |    3 +--
 gdb/remote.c    |    2 ++
 gdb/spu-tdep.c  |    3 ++-
 6 files changed, 24 insertions(+), 24 deletions(-)

Index: src/gdb/gdbtypes.c
===================================================================
--- src.orig/gdb/gdbtypes.c	2007-01-23 09:00:02.000000000 -0500
+++ src/gdb/gdbtypes.c	2007-01-23 09:03:04.000000000 -0500
@@ -3419,16 +3419,6 @@ build_gdbtypes (void)
 	       0,
 	       "bool", (struct objfile *) NULL);
 
-  /* Add user knob for controlling resolution of opaque types */
-  add_setshow_boolean_cmd ("opaque-type-resolution", class_support,
-			   &opaque_type_resolution, _("\
-Set resolution of opaque struct/class/union types (if set before loading symbols)."), _("\
-Show resolution of opaque struct/class/union types (if set before loading symbols)."), NULL,
-			   NULL,
-			   show_opaque_type_resolution,
-			   &setlist, &showlist);
-  opaque_type_resolution = 1;
-
   /* Build SIMD types.  */
   builtin_type_v4sf
     = init_simd_type ("__builtin_v4sf", builtin_type_float, "f", 4);
@@ -3719,8 +3709,6 @@ _initialize_gdbtypes (void)
 	       TYPE_FLAG_UNSIGNED,
 	       "uint128_t", (struct objfile *) NULL);
 
-  build_gdbtypes ();
-
   gdbtypes_data = gdbarch_data_register_post_init (gdbtypes_post_init);
 
   /* FIXME - For the moment, handle types by swapping them in and out.
@@ -3734,6 +3722,7 @@ _initialize_gdbtypes (void)
      a different architecture.  */
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_void);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_char);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_true_char);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_short);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_int);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_long);
@@ -3750,6 +3739,7 @@ _initialize_gdbtypes (void)
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_complex);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_double_complex);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_string);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_bool);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_v4sf);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_v4si);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_v16qi);
@@ -3767,6 +3757,7 @@ _initialize_gdbtypes (void)
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_v2_int32);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_v8_int8);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_v4_int16);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_vec64);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_vec128);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_void_data_ptr);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_void_func_ptr);
@@ -3889,4 +3880,14 @@ When enabled, ranking of the functions i
 			    NULL,
 			    show_overload_debug,
 			    &setdebuglist, &showdebuglist);
+
+  /* Add user knob for controlling resolution of opaque types */
+  add_setshow_boolean_cmd ("opaque-type-resolution", class_support,
+			   &opaque_type_resolution, _("\
+Set resolution of opaque struct/class/union types (if set before loading symbols)."), _("\
+Show resolution of opaque struct/class/union types (if set before loading symbols)."), NULL,
+			   NULL,
+			   show_opaque_type_resolution,
+			   &setlist, &showlist);
+  opaque_type_resolution = 1;
 }
Index: src/gdb/parse.c
===================================================================
--- src.orig/gdb/parse.c	2007-01-23 09:00:02.000000000 -0500
+++ src/gdb/parse.c	2007-01-23 09:03:04.000000000 -0500
@@ -1405,14 +1405,13 @@ _initialize_parse (void)
   type_stack = (union type_stack_elt *)
     xmalloc (type_stack_size * sizeof (*type_stack));
 
-  build_parse ();
-
   /* FIXME - For the moment, handle types by swapping them in and out.
      Should be using the per-architecture data-pointer and a large
      struct. */
   DEPRECATED_REGISTER_GDBARCH_SWAP (msym_text_symbol_type);
   DEPRECATED_REGISTER_GDBARCH_SWAP (msym_data_symbol_type);
   DEPRECATED_REGISTER_GDBARCH_SWAP (msym_unknown_symbol_type);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (msym_tls_symbol_type);
   deprecated_register_gdbarch_swap (NULL, 0, build_parse);
 
   add_setshow_zinteger_cmd ("expression", class_maintenance,
Index: src/gdb/f-lang.c
===================================================================
--- src.orig/gdb/f-lang.c	2007-01-23 09:00:02.000000000 -0500
+++ src/gdb/f-lang.c	2007-01-23 09:03:04.000000000 -0500
@@ -567,8 +567,6 @@ build_fortran_types (void)
 void
 _initialize_f_language (void)
 {
-  build_fortran_types ();
-
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_f_character);
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_f_logical); 
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_f_logical_s1); 
@@ -582,14 +580,8 @@ _initialize_f_language (void)
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_f_complex_s16); 
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_f_complex_s32); 
   DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_f_void); 
-  DEPRECATED_REGISTER_GDBARCH_SWAP (builtin_type_string); 
   deprecated_register_gdbarch_swap (NULL, 0, build_fortran_types);
 
-  builtin_type_string =
-    init_type (TYPE_CODE_STRING, TARGET_CHAR_BIT / TARGET_CHAR_BIT,
-	       0,
-	       "character string", (struct objfile *) NULL);
-
   add_language (&f_language_defn);
 }
 
Index: src/gdb/i386-tdep.c
===================================================================
--- src.orig/gdb/i386-tdep.c	2007-01-23 09:00:02.000000000 -0500
+++ src/gdb/i386-tdep.c	2007-01-23 09:03:04.000000000 -0500
@@ -2521,5 +2521,10 @@ is \"default\"."),
 
   /* Initialize the i386-specific register groups & types.  */
   i386_init_reggroups ();
-  i386_init_types();
+
+  DEPRECATED_REGISTER_GDBARCH_SWAP (i386_eflags_type);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (i386_mmx_type);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (i386_sse_type);
+  DEPRECATED_REGISTER_GDBARCH_SWAP (i386_mxcsr_type);
+  deprecated_register_gdbarch_swap (NULL, 0, i386_init_types);
 }
Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2007-01-23 09:00:02.000000000 -0500
+++ src/gdb/remote.c	2007-01-23 09:03:04.000000000 -0500
@@ -784,6 +784,8 @@ add_packet_config_cmd (struct packet_con
 				set_remote_protocol_packet_cmd,
 				show_remote_protocol_packet_cmd,
 				&remote_set_cmdlist, &remote_show_cmdlist);
+  xfree (set_doc);
+  xfree (show_doc);
   /* set/show remote NAME-packet {auto,on,off} -- legacy.  */
   if (legacy)
     {
Index: src/gdb/spu-tdep.c
===================================================================
--- src.orig/gdb/spu-tdep.c	2007-01-23 09:23:42.000000000 -0500
+++ src/gdb/spu-tdep.c	2007-01-23 09:24:40.000000000 -0500
@@ -1107,5 +1107,6 @@ _initialize_spu_tdep (void)
 {
   register_gdbarch_init (bfd_arch_spu, spu_gdbarch_init);
 
-  spu_init_vector_type ();
+  DEPRECATED_REGISTER_GDBARCH_SWAP (spu_builtin_type_vec128);
+  deprecated_register_gdbarch_swap (NULL, 0, spu_init_vector_type);
 }

Re: [rfc] Plug memory leaks during gdbarch initialization

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> The patch is an improvement, but doesn't fix that wart.  I don't really
> want to check it in without fixing the underlying problem but I don't
> see a good way... anyone have a suggestion?

I could imagine having each file's _initialize function start with:

void
_initialize_foo (void)
{
  static initialized;

  if (initialized)
    return;
  initialized = 1;

  ...
}

A sed script could get the job done.  Then, they could call each other
with wild abandon.  For extra points, you could do:

void
_initialize_foo (void)
{
  static int initialized;

  assert (initialized != 1);
  if (initialized)
    return;
  initialized = 1;

  ...

  initialized = 2;
}

So you'd catch circular dependencies.

Re: [rfc] Plug memory leaks during gdbarch initialization

[Daniel Jacobowitz]

On Tue, Jan 23, 2007 at 07:25:01AM -0800, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@false.org> writes:
> > The patch is an improvement, but doesn't fix that wart.  I don't really
> > want to check it in without fixing the underlying problem but I don't
> > see a good way... anyone have a suggestion?
> 
> I could imagine having each file's _initialize function start with:

Now that they're in deprecated_register_gdbarch_swap, this isn't enough
- we no longer rely on _initialize_foo's order directly, but indirectly
through the order of initializer functions in the swap list.

Hmm, but your change would still fix it, with judicious addition of
comments that order matters for swaps...

-- 
Daniel Jacobowitz
CodeSourcery