[RFA] dangling cleanup in find_frame_funname

Joel Brobecker brobecker@adacore.com
Tue May 21 05:33:00 GMT 2013


> 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



More information about the Gdb-patches mailing list