Re: [RFA take 3] Allow setting breakpoints on inline functions (PR 10738) [repost]

[Jan Kratochvil <jan.kratochvil@redhat.com>]

[ repost - the mail is missing at gdb-patches ]

Hi Gary,

On Fri, 27 Jan 2012 16:10:34 +0100, Gary Benson wrote:
> How does it look?  Hopefully there are not a load more places the
> symbols have leaked into now!

I guess Tom is fine with my review, except for some details I am fine with it,
thanks for the fix.

FYI this comment by me looks fixed with current FSF GDB HEAD and your patch:
	http://sourceware.org/bugzilla/show_bug.cgi?id=10738#c11


[...]
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -321,6 +321,33 @@ cplusplus_error (const char *name, const char *fmt, ...)
>    throw_error (NOT_FOUND_ERROR, "%s", message);
>  }
>  
> +/* A callback function and the data to pass to it.  */
> +
> +struct callback_and_data
> +{
> +  /* The callback to use.  */
> +  int (*callback) (struct symbol *, void *);
> +
> +  /* Data to be passed to the callback.  */
> +  void *data;
> +};

I would prefer renaming "callback_and_data" so that the name somehow contains
it is related to "symbol".


> +
> +/* A helper for iterate_over_all_matching_symtabs that is used
> +   to restrict calls to another callback to symbols representing
> +   the inlined instances of functions only.  */
> +
> +static int
> +iterate_inline_only (struct symbol *sym, void *d)
> +{
> +  if (SYMBOL_INLINED (sym))
> +    {
> +      struct callback_and_data *cad = (struct callback_and_data *) d;

Excessive cast.


> +
> +      return cad->callback (sym, cad->data);
> +    }
> +  return 0;

Could you make a comment why is here 0?  I would think here should be 1 but
I may be wrong.


> +}
> +
>  /* Some data for the expand_symtabs_matching callback.  */
>  
>  struct symbol_matcher_data
> @@ -348,14 +375,16 @@ iterate_name_matcher (const char *name, void *d)
>  /* A helper that walks over all matching symtabs in all objfiles and
>     calls CALLBACK for each symbol matching NAME.  If SEARCH_PSPACE is
>     not NULL, then the search is restricted to just that program
> -   space.  */
> +   space.  If INCLUDE_INLINE is nonzero then symbols representing
> +   inlined instances of functions will be included in the result.  */
>  
>  static void
>  iterate_over_all_matching_symtabs (const char *name,
>  				   const domain_enum domain,
>  				   int (*callback) (struct symbol *, void *),

Could you provide a pre-requisite patch turning this CALLBACK argument type
into symbol_found_callback_ftype typedef where its result would be described
(which is described in la_iterate_over_symbols but that is a bit far away).
And possibly commenting all the 0 and 1 return values in the implementations.
I would do it otherwise.


>  				   void *data,
> -				   struct program_space *search_pspace)
> +				   struct program_space *search_pspace,
> +				   int include_inline)
>  {
>    struct objfile *objfile;
>    struct program_space *pspace;
[...]
> @@ -2245,7 +2288,8 @@ find_function_symbols (char **argptr, char *p, int is_quote_enclosed,
>      copy[p - *argptr] = 0;
>  
>    iterate_over_all_matching_symtabs (copy, VAR_DOMAIN,
> -				     collect_function_symbols, &result, NULL);
> +				     collect_function_symbols, &result, NULL,
> +				     0);

I see 1 here to support `break inlinedfunc:labelname' does not work, just such
a statement, fine with me, it can be an incremental improvement one day in the
future.


>  
>    if (VEC_empty (symbolp, result))
>      VEC_free (symbolp, result);
[...]
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-inline-break.S
> @@ -0,0 +1,1663 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2011 Free Software Foundation, Inc.

2011++.  Four times.


> --- /dev/null
> +++ b/gdb/testsuite/gdb.opt/inline-break.c
[...]
> +/* The file ../gdb.dwarf2/inline-break.S was generated manually from
> +   this file, and should be regenerated if this file is modified.  */
> +
> +#ifdef __GNUC__
> +#define ATTR __attribute__((always_inline))
> +#else
> +#define ATTR
> +#endif

Indentation (sure a nitpick):

#ifdef __GNUC__
# define ATTR __attribute__((always_inline))
#else
# define ATTR
#endif



Thanks,
Jan