This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[RFC-v2] Dispose properly of registered gdbarch'es at exit.



> -----Message d'origine-----
> De?: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé?: mardi 11 décembre 2012 19:53
> À?: Pierre Muller
> Cc?: gdb-patches@sourceware.org
> Objet?: Re: [RFC] Dispose properly of registered gdbarch'es at exit.
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre>   I am trying to get GDB to release all the allocated
> Pierre> memory.
> Pierre>   Currently one of the biggest remaining chunk is the
> Pierre> allocated gdbarch.
> 
> Pierre>   The patch below is an attempt to get rid of that leak.
> 
> Thanks.
> 
> Pierre> PS: It might seem unimportant for GDB executable,
> Pierre> but having a library that has no leak is useful...
> 
> Cough cough.
> 
> I think I would like it if "final" cleanups were only ones that were
> important to gdb shutdown.  There's no reason to do much shutdown work
> if you're just going to exit.
> 
> I see a bogus-by-this-rule final cleanup in gdb_insn_length.

  It took me some time to understand your point:
  we also need to reset that static variable to NULL inside...
 Included in new version of the patch.	
 
> Anyway, I suppose you could introduce a new kind of cleanup that is only
> used by your program.
> 
> Pierre> +static void
> Pierre> +gdbarch_free_registered (void *arg)
> 
> Needs an intro comment.

  There were none in the other functions around...
Added.
 
> Pierre> +  struct gdbarch_registration *curr = *(struct
gdbarch_registration
> **)arg;
> Pierre> +  gdb_assert (curr == gdbarch_registry);
> 
> Blank line between declaration and code.
Added.
 
> The 'curr' thing is somewhat weird given the assert.
> Personally I would just ignore the argument and use the global.
I wasn't sure about unused args, do we still need anything special for
those,
otherwise I agree it would e easier to just use a dummy NULL arg.

> However, I don't mind the above; but you need a space after the closing
> paren on the cast.
 Space added. 
> Pierre> +      struct gdbarch_list *list = curr->arches;
> Pierre> +      while (list)
> 
> Blank line.
Added.
 
> Pierre> +             /* There is a gdb_assert on this inside
gdbarch_free.
> */
> Pierre> +             list->gdbarch->initialized_p = 0;
> 
> I think at this point it is better to remove the assert, since it is no
> longer correct.

  I thought that it was still useful
as it still checks that gdbarch_free isn't called by accident elsewhere...

But if you prefer, I can send another version with the assert removed from
gdbarch_free instead.

Second version of the patch below,


Pierre

2012-12-11  Pierre Muller  <muller@sourceware.org>

        * disasm.c (do_ui_file_delete): Adapt to reset static variable
        null_stream to NULL at exit.
        (gdb_insn_length): Change second parameter of make_final_cleanup
        to adapt to change in do_ui_file_delete function.
        * gdbarch.sh (gdbarch_free_registered): New static function.
        (_initialize_gdbarch): Add final cleanup for registered
        gdbarch'es.


Index: disasm.c
===================================================================
RCS file: /cvs/src/src/gdb/disasm.c,v
retrieving revision 1.49
diff -u -p -r1.49 disasm.c
--- disasm.c    13 Nov 2012 15:35:43 -0000      1.49
+++ disasm.c    11 Dec 2012 23:08:09 -0000
@@ -462,7 +462,9 @@ gdb_print_insn (struct gdbarch *gdbarch,
 static void
 do_ui_file_delete (void *arg)
 {
-  ui_file_delete (arg);
+  struct ui_file **ui = (struct ui_file **) arg;
+  ui_file_delete (*ui);
+  *ui = NULL;
 }

 /* Return the length in bytes of the instruction at address MEMADDR in
@@ -477,7 +479,7 @@ gdb_insn_length (struct gdbarch *gdbarch
   if (!null_stream)
     {
       null_stream = ui_file_new ();
-      make_final_cleanup (do_ui_file_delete, null_stream);
+      make_final_cleanup (do_ui_file_delete, &null_stream);
     }

   return gdb_print_insn (gdbarch, addr, null_stream, NULL);
Index: gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.550
diff -u -p -r1.550 gdbarch.sh
--- gdbarch.sh  21 Nov 2012 00:29:54 -0000      1.550
+++ gdbarch.sh  11 Dec 2012 23:08:11 -0000
@@ -2072,6 +2072,35 @@ gdbarch_printable_names (void)
   return arches;
 }

+/* Free all memory allocated to the registered gdbarch by
+   walikng through the gdbarch_registration chain.  */
+
+static void
+gdbarch_free_registered (void *arg)
+{
+  struct gdbarch_registration *curr = * gdbarch_registry;
+
+  while (curr)
+    {
+      struct gdbarch_list *list = curr->arches;
+
+      while (list)
+       {
+         if (list->gdbarch)
+           {
+             /* There is a gdb_assert on this inside gdbarch_free.  */
+             list->gdbarch->initialized_p = 0;
+             gdbarch_free (list->gdbarch);
+           }
+         curr->arches = list->next;
+         xfree (list);
+         list = curr->arches;
+       }
+      gdbarch_registry = curr->next;
+      xfree (curr);
+      curr = gdbarch_registry;
+    }
+}

 void
 gdbarch_register (enum bfd_architecture bfd_architecture,
@@ -2301,6 +2330,7 @@ When non-zero, architecture debugging is
                             NULL,
                             show_gdbarch_debug,
                             &setdebuglist, &showdebuglist);
+  make_final_cleanup (gdbarch_free_registered, NULL);
 }
 EOF


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]