This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[RFC-v2] Dispose properly of registered gdbarch'es at exit.
- From: "Pierre Muller" <pierre dot muller at ics-cnrs dot unistra dot fr>
- To: "'Tom Tromey'" <tromey at redhat dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Wed, 12 Dec 2012 00:13:52 +0100
- Subject: [RFC-v2] Dispose properly of registered gdbarch'es at exit.
- References: <10150.4984115765$1355246247@news.gmane.org> <87sj7cbeui.fsf@fleche.redhat.com>
> -----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