This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA take 3] Allow setting breakpoints on inline functions (PR 10738) [repost]
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: gdb-patches at sourceware dot org
- Cc: Gary Benson <gbenson at redhat dot com>
- Date: Sat, 28 Jan 2012 10:07:24 +0100
- Subject: Re: [RFA take 3] Allow setting breakpoints on inline functions (PR 10738) [repost]
- References: <20120127151034.GA22606@redhat.com> <20120128001638.GA4448@host2.jankratochvil.net>
[ 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