This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] dangling cleanup in find_frame_funname
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Tue, 21 May 2013 09:33:01 +0400
- Subject: Re: [RFA] dangling cleanup in find_frame_funname
- References: <51953293 dot 1020502 at redhat dot com> <20130520092358 dot GO4017 at adacore dot com> <519A6E0A dot 3000701 at redhat dot com>
> I'm all for making things easier to maintain! :-)
Cool :)!
> 2013-05-20 Keith Seitz <keiths@redhat.com>
>
> * ada-lang.c (is_known_support_routine): Add explicit free of
> 'func_name' from find_frame_funname.
> (ada_unhandled_exception_name_addr_from_raise): Add cleanups
> for func_name from find_frame_funname.
> * python/py-frame.c (frapy_name): Add explicit free of
> 'name' from find_frame_funname.
> * stack.c (find_frame_funname): Add comment explaining that
> funcp must be freed by the caller.
> Return copy of symbol names instead of pointers.
> (print_frame): Add a cleanup for 'funname' from
> find_frame_funname.
> * stack.h (find_frame_funname): Remove "const" from
> 'funname' parameter.
Looks good to me too. Just one tiny request, if I may:
> @@ -11198,6 +11202,7 @@ ada_unhandled_exception_name_addr_from_raise (void)
> int frame_level;
> struct frame_info *fi;
> struct ada_inferior_data *data = get_ada_inferior_data (current_inferior ());
> + struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
>
> /* To determine the name of this exception, we need to select
> the frame corresponding to RAISE_SYM_NAME. This frame is
> @@ -11210,15 +11215,21 @@ ada_unhandled_exception_name_addr_from_raise (void)
>
> while (fi != NULL)
> {
> - const char *func_name;
> + char *func_name;
> enum language func_lang;
>
> find_frame_funname (fi, &func_name, &func_lang, NULL);
> - if (func_name != NULL
> - && strcmp (func_name, data->exception_info->catch_exception_sym) == 0)
> - break; /* We found the frame we were looking for... */
> - fi = get_prev_frame (fi);
> + if (func_name != NULL)
> + {
> + make_cleanup (xfree, func_name);
> +
> + if (strcmp (func_name,
> + data->exception_info->catch_exception_sym) == 0)
> + break; /* We found the frame we were looking for... */
> + fi = get_prev_frame (fi);
> + }
> }
> + do_cleanups (old_chain);
Would you mind moving the call to make_cleanup that sets old_chain
just before the "while". To me, that's useful, because the make_cleanup/
do_cleanup calls bracket the while loop, showing that this is the
context where the cleanup is used.
And while touching this area of the code, the "if (strcmp" line is
indented with spaces-only, while it should be tabs-and-spaces
(a PIA, if you ask me).
Thank you!
--
Joel