From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5879 invoked by alias); 28 Jan 2012 09:07:57 -0000 Received: (qmail 5869 invoked by uid 22791); 28 Jan 2012 09:07:55 -0000 X-SWARE-Spam-Status: No, hits=-4.5 required=5.0 tests=AWL,BAYES_50,KAM_STOCKGEN,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_BJ,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 28 Jan 2012 09:07:31 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0S97VMk028600 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 28 Jan 2012 04:07:31 -0500 Received: from host2.jankratochvil.net (ovpn-116-47.ams2.redhat.com [10.36.116.47]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q0S97P52025416 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Sat, 28 Jan 2012 04:07:28 -0500 Date: Sat, 28 Jan 2012 18:16:00 -0000 From: Jan Kratochvil To: gdb-patches@sourceware.org Cc: Gary Benson Subject: Re: [RFA take 3] Allow setting breakpoints on inline functions (PR 10738) [repost] Message-ID: <20120128090724.GA30170@host2.jankratochvil.net> References: <20120127151034.GA22606@redhat.com> <20120128001638.GA4448@host2.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120128001638.GA4448@host2.jankratochvil.net> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-01/txt/msg00972.txt.bz2 [ 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