From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32374 invoked by alias); 3 May 2008 20:54:41 -0000 Received: (qmail 32364 invoked by uid 22791); 3 May 2008 20:54:40 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 03 May 2008 20:54:16 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 58BB0983D9; Sat, 3 May 2008 20:54:14 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id 27DAC98150; Sat, 3 May 2008 20:54:14 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1JsOkH-0006I6-4u; Sat, 03 May 2008 16:54:13 -0400 Date: Sat, 03 May 2008 21:32:00 -0000 From: Daniel Jacobowitz To: Aleksandar Ristovski Cc: gdb-patches@sourceware.org Subject: Re: [patch] Do not add partial_symbol again and again to the list Message-ID: <20080503205413.GA22704@caradoc.them.org> Mail-Followup-To: Aleksandar Ristovski , gdb-patches@sourceware.org References: <20080211203809.GA29560@caradoc.them.org> <47B0B56F.4010607@qnx.com> <20080211210935.GA31767@caradoc.them.org> <47B0C0F4.4090302@qnx.com> <20080211214750.GA1953@caradoc.them.org> <47B0C7C6.9090605@qnx.com> <20080211223056.GA3833@caradoc.them.org> <47B0CF8A.6080306@qnx.com> <20080211225314.GA5832@caradoc.them.org> <47B27EB1.6030606@qnx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <47B27EB1.6030606@qnx.com> User-Agent: Mutt/1.5.17 (2007-12-11) 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: 2008-05/txt/msg00142.txt.bz2 On Wed, Feb 13, 2008 at 12:22:57AM -0500, Aleksandar Ristovski wrote: > 3. lookup_partial_symbol > Function looks up global or static list of objfile, but only subset of it > determined by partial symtab's offset and number of symbols. Returns partial > symbol found. This function is always called from a loop through all partial > symtabs (this fact is important). Ah! That's the part I didn't understand before. I was worried about the construct tested in scope.exp, "print 'scope0.c'::filelocal". But it uses full symtabs, not partial symtabs. > 2008-02-12 Aleksandar Ristovski > > * bcache.c (bcache_data): Call bcache_added function. > (bcache_added): New function name. Body of function bcache_data > is used here with the addition of 'added' argument. The argument > is optional and if specified, bcache_added returns 1 if the data > was added and 0 if it was found in the hash. Whenever you find yourself writing this sort of explanation in the changelog, it's a sign that you should have a shorter changelog entry and a long comment in the source code. Please move the explanation to bcache.h. > * symfile.c (add_psymbol_to_bcache): New helper function, takes part of > work from add_psymbol_to_list - initialises partial symbol and stashes > it in objfile's cache. > (append_psymbol_to_list): New helper function, takes other part of > work from add_psymbol_to_list - adds partial symbol to the given list. > (add_psymbol_to_list): Call helper functions instead of doing work > here. Functionally, the function hasn't changed. > (add_psymbol_to_global_list): New function, adds partial symbol to > global list and does it only once per objfile. Same thing for these. > +const void * > +bcache (const void *addr, int length, struct bcache *bcache) > +{ > + return bcache_data (addr, length, bcache); > +} > + > +void * > +bcache_added (const void *addr, int length, struct bcache *bcache, > + int *added) It should return a const pointer, like bcache. Also the indentation on the second line is too deep. > case DW_TAG_namespace: > - add_psymbol_to_list (actual_name, strlen (actual_name), > + add_psymbol_to_global_list (actual_name, strlen (actual_name), > VAR_DOMAIN, LOC_TYPEDEF, > - &objfile->global_psymbols, > 0, (CORE_ADDR) 0, cu->language, objfile); Please adjust indentation on the subsequent lines, since the open paren moved over. > if (cu->language == language_cplus > || cu->language == language_java > || cu->language == language_ada) > { > /* For C++ and Java, these implicitly act as typedefs as well. */ > - add_psymbol_to_list (actual_name, strlen (actual_name), > - VAR_DOMAIN, LOC_TYPEDEF, > - &objfile->global_psymbols, > - 0, (CORE_ADDR) 0, cu->language, objfile); > + add_psymbol_to_global_list (actual_name, strlen (actual_name), > + VAR_DOMAIN, LOC_TYPEDEF, > + 0, (CORE_ADDR) 0, cu->language, objfile); > } You've deleted this bit in your other patch fortunately :-) > case DW_TAG_enumerator: > - add_psymbol_to_list (actual_name, strlen (actual_name), > + if (cu->language == language_cplus > + || cu->language == language_java) > + add_psymbol_to_global_list (actual_name, strlen (actual_name), > + VAR_DOMAIN, LOC_CONST, > + 0, (CORE_ADDR) 0, cu->language, objfile); This part I don't understand. Why does the behavior of enumerators depend on the language? The logic for finding them during lookup is the same. In fact, if lookup_partial_symbol will never find a global symbol with an identical name why do we need them for any duplicated symbol, even legitimately duplicated things like static functions? Well, the check in cp-support.c is one reason for functions. And I can imagine other valid reasons in the future. So functions are special. But for things other than functions. -- Daniel Jacobowitz CodeSourcery