From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25546 invoked by alias); 29 Aug 2011 11:41:19 -0000 Received: (qmail 25535 invoked by uid 22791); 29 Aug 2011 11:41:18 -0000 X-SWARE-Spam-Status: No, hits=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 29 Aug 2011 11:41:05 +0000 Received: (qmail 20188 invoked from network); 29 Aug 2011 11:41:04 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 29 Aug 2011 11:41:04 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: combine bfd_lookup_symbol in solib-*.c Date: Mon, 29 Aug 2011 11:41:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-11-generic; KDE/4.7.0; x86_64; ; ) Cc: Yao Qi References: <4E4D318E.704@codesourcery.com> In-Reply-To: <4E4D318E.704@codesourcery.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201108291241.02099.pedro@codesourcery.com> 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: 2011-08/txt/msg00559.txt.bz2 Hi Yao, thanks for doing this. Looks mostly good. A couple cosmetic comments below. On Thursday 18 August 2011 16:36:46, Yao Qi wrote: > + > +CORE_ADDR > +bfd_lookup_symbol (bfd *abfd, const char *symname, > + int (*is_right_sym) (asymbol *, const char *)) All the function does with the passed in SYMNAME is passing it down to the callback. If the interface has been generalized to pass in a "match" callback, then make the symname parameter generic too, like: CORE_ADDR bfd_lookup_symbol (bfd *abfd, int (*is_right_sym) (asymbol *, void *), void *data); Please document the parameters. I think it's best to reserve extern symbols that start with "bfd_" to libbfd proper. Maybe gdb_bfd_lookup_symbol would be a good enough alternative. (Or maybe propose the function for bfd proper, though I'd suggest fixing its interface to not assume 0 return is error then) > +/* Lookup the value for a specific symbol. > + > + An expensive way to lookup the value of a single symbol for > + bfd's that are only temporary anyway. This is used by the > + shared library support to find the address of the debugger > + notification routine in the shared library. > + > + The returned symbol may be in a code or data section; functions > + will normally be in a code section, but may be in a data section > + if this architecture uses function descriptors. The way you've generalized the function, since it's the callback's responsibility to pick the the symbol, this comment no longer appears to be in the right place. -- Pedro Alves