From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17964 invoked by alias); 20 Feb 2003 20:04:18 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 17931 invoked from network); 20 Feb 2003 20:04:16 -0000 Received: from unknown (HELO mx1.redhat.com) (172.16.49.200) by 172.16.49.205 with SMTP; 20 Feb 2003 20:04:16 -0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.11.6/8.11.6) with ESMTP id h1KK4GN13372 for ; Thu, 20 Feb 2003 15:04:16 -0500 Received: from pobox.corp.redhat.com (pobox.corp.redhat.com [172.16.52.156]) by int-mx1.corp.redhat.com (8.11.6/8.11.6) with ESMTP id h1KK4Gf03882 for ; Thu, 20 Feb 2003 15:04:16 -0500 Received: from localhost.redhat.com (romulus-int.sfbay.redhat.com [172.16.27.46]) by pobox.corp.redhat.com (8.11.6/8.11.6) with ESMTP id h1KK4Ft21515 for ; Thu, 20 Feb 2003 15:04:15 -0500 Received: by localhost.redhat.com (Postfix, from userid 469) id CE188FF79; Thu, 20 Feb 2003 15:08:20 -0500 (EST) From: Elena Zannoni MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <15957.13747.748204.408563@localhost.redhat.com> Date: Thu, 20 Feb 2003 20:04:00 -0000 To: David Carlton , gdb-patches@sources.redhat.com Subject: Re: [rfa] more lookup_symbol_aux_minsyms futzing In-Reply-To: References: X-SW-Source: 2003-02/txt/msg00490.txt.bz2 A few comments David Carlton writes: > Here's another patch involving lookup_symbol_aux_minsyms. It reverts > the search_symbols part of my last patch, and deletes the last "else > if" part of lookup_symbol_aux_minsyms. (And it deletes the > 'is_a_field_of_this' argument, since that's was only used in that > "else if" part.) > > I'd been ascribing more power to lookup_symbol_aux_minsyms than it > actually had: I had assumed that it did a reasonable job of finding a > symbol corresponding to a minimal symbol whenever there was one, but > now I think that it only does that in the function case. Furthermore, > it seems to me that whatever it tries to do in the non-function case > is more likely to be hazardous than helpful. (Or at least was more > likely before 'force_return' got deleted.) > > This has two implications: > > 1) Don't count on lookup_symbol_aux_minsyms doing anything useful in > the non-function case. In particular, my search_symbols patch from > before was bad. > > 2) Get rid of the non-function case of lookup_symbol_aux_minsyms: it > just confuses the issue and slows down symbol lookup. > > Let me analyze lookup_symbol_aux_minsyms in a bit more detail, to back > this up. It looks like this, stripped down: > > if (namespace == VAR_NAMESPACE) > { > msymbol = lookup_minimal_symbol (name, NULL, NULL); > > if (msymbol != NULL) > { > s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol), > SYMBOL_BFD_SECTION (msymbol)); > if (s != NULL) > { > /* Find the correct symbol, and return it. */ > } > else if (MSYMBOL_TYPE (msymbol) != mst_text > && MSYMBOL_TYPE (msymbol) != mst_file_text > && !STREQ (name, SYMBOL_NAME (msymbol))) > { > /* This is a mangled variable, look it up by its > mangled name. */ > return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, > NULL, namespace, is_a_field_of_this, > symtab); > } > } > } > > return NULL; > > Now, find_pc_sect_symtab only works if the symbol in question > corresponds to a function, loosely speaking: it finds the right > minimal symbol (which will be 'msymbol' above) and then immediately > returns NULL if that symbol doesn't have type mst_text or > mst_file_text. So the whole bit that I've marked by "Find the correct > symbol, and return it" only happens if we're looking for a function. > I think this deserves a clearer comment. It took me a while to refigure this out. Also, I suggest changing the name of the function to reflect this. lookup_symbol_aux_func? > So what happens in the "else if" case? This checks that we're in the > non-function case, and that the name we were passed isn't the > SYMBOL_NAME of msymbol, which means that msymbol has a mangled name > while we were passed the demangled name. In this case, it calles > lookup_symbol_aux with a mangled name as the 'name' argument. So what > happens in this nested call to lookup_symbol_aux? > > 1) lookup_symbol_aux_local. This doesn't happen because 'block' is > NULL. Micro optimization: should we conditionalize the call on 'block', and initialize 'static_block' to NULL. Just like the call to lookup_symbol_aux_block is. > > 2) The 'is_a_field_of_this' check. It makes no sense to do this now: > we already did it once! So we should have passed NULL in place of > 'is_a_field_of_this': oops. > however, because of the oddities in this recursive call, in the first invocation the is_a_field_of_this check was done using 'name' (ie the demangled name), while now we redo it using SYMBOL_NAME(msymbol) which would be the mangled one. does it matter? > 3) The static block check; again, 'block' is NULL, so this doesn't > happen. > ok > 4) The 'lookup_symbol_aux_symtabs' check. The symtab search > eventually comes down to calling lookup_block_symbol a bunch of > times, with the mangled name as its 'name' argument. But, as Jim > Blandy noted in > , > doing that makes no sense: it won't find a match in the hash > table. So that's not useful. > what happened to that patch? did it ever get committed? I wonder if it would have helped in Jason's case. > 5) The 'lookup_symbol_aux_minsyms' check. That's not going to uncover > anything the second time that it didn't uncover the first time it > was called. > > 6) The 'lookup_symbol_aux_psymtabs' check. This tries to find the > right partial symbol table, read in the corresponding symbol table, > and then search it with lookup_block_symbol. But, again, calling > lookup_block_symbol with the mangled name as the 'name' argument > makes no sense. > right, in this invocation of lookup_symbol_aux the two parameters name and mangled_name are the same. > So in no case is it correct for lookup_symbol_aux_minsyms to call > lookup_symbol_aux with the demangled name as the 'name' argument. So > I think we should just delete that call. > > There is one thing that bothers me: right now, partial symbols don't > include demangled names. So that call to lookup_symbol_aux_psymtabs > with the demangled name might actually cause the correct symtab to be > read in, even if the lookup_block_symbol fails. The correct thing to > do here is to store demangled names in partial symbols, however, not > to depend on an undocumented side effect of an incorrect psymtab > search. I'll submit a patch for that next. > > David Carlton > carlton@math.stanford.edu > > 2002-12-23 David Carlton > > * symtab.c (search_symbols): Change call to > lookup_symbol_aux_minsyms back to cal to lookup_symbol, reverting > previous patch. > (lookup_symbol_aux_minsyms): Don't search on mangled names; delete > 'is_a_field_of_this' argument. > (lookup_symbol_aux): Don't pass 'is_a_field_of_this' to > lookup_symbol_aux_minsyms. > > Index: symtab.c > =================================================================== > RCS file: /cvs/src/src/gdb/symtab.c,v > retrieving revision 1.83 > diff -u -p -r1.83 symtab.c > --- symtab.c 23 Dec 2002 16:43:18 -0000 1.83 > +++ symtab.c 23 Dec 2002 18:03:03 -0000 > @@ -116,7 +116,6 @@ static > struct symbol *lookup_symbol_aux_minsyms (const char *name, > const char *mangled_name, > const namespace_enum namespace, > - int *is_a_field_of_this, > struct symtab **symtab); > > static struct symbol *find_active_alias (struct symbol *sym, CORE_ADDR addr); > @@ -923,8 +922,7 @@ lookup_symbol_aux (const char *name, con > Eventually, all global symbols might be resolved in this way. */ > > sym = lookup_symbol_aux_minsyms (name, mangled_name, > - namespace, is_a_field_of_this, > - symtab); > + namespace, symtab); > > if (sym != NULL) > return sym; > @@ -971,8 +969,7 @@ lookup_symbol_aux (const char *name, con > > > sym = lookup_symbol_aux_minsyms (name, mangled_name, > - namespace, is_a_field_of_this, > - symtab); > + namespace, symtab); > > if (sym != NULL) > return sym; > @@ -1154,24 +1151,20 @@ lookup_symbol_aux_psymtabs (int block_in > return NULL; > } > > -/* Check for the possibility of the symbol being a function or a > - mangled variable that is stored in one of the minimal symbol > - tables. Eventually, all global symbols might be resolved in this > - way. */ > - > -/* NOTE: carlton/2002-12-05: At one point, this function was part of > - lookup_symbol_aux, and what are now 'return' statements within > - lookup_symbol_aux_minsyms returned from lookup_symbol_aux, even if > - sym was NULL. As far as I can tell, this was basically accidental; > - it didn't happen every time that msymbol was non-NULL, but only if > - some additional conditions held as well, and it caused problems > - with HP-generated symbol tables. */ > +/* Check for the possibility of the symbol being a function that is > + stored in one of the minimal symbol tables. */ > + > +/* NOTE: carlton/2002-12-23: At one point, when this function was part > + of lookup_symbol_aux, its behavior was different, for reasons that > + are unclear: it forced a return from lookup_symbol_aux at times > + even without checking the partial symbol tables, and it tried to do > + something strange involving mangled names in the non-function > + case. */ > > static struct symbol * > lookup_symbol_aux_minsyms (const char *name, > const char *mangled_name, > const namespace_enum namespace, > - int *is_a_field_of_this, > struct symtab **symtab) > { > struct symbol *sym; > @@ -1186,20 +1179,15 @@ lookup_symbol_aux_minsyms (const char *n > > if (msymbol != NULL) > { > - /* OK, we found a minimal symbol in spite of not finding any > - symbol. There are various possible explanations for > - this. One possibility is the symbol exists in code not > - compiled -g. Another possibility is that the 'psymtab' > - isn't doing its job. A third possibility, related to #2, > - is that we were confused by name-mangling. For instance, > - maybe the psymtab isn't doing its job because it only > - know about demangled names, but we were given a mangled > - name... */ > - > - /* We first use the address in the msymbol to try to locate > - the appropriate symtab. Note that find_pc_sect_symtab() > - has a side-effect of doing psymtab-to-symtab expansion, > - for the found symtab. */ > + /* We found a minimal symbol in spite of not finding a > + symbol. This probably means that the symbol in question > + is in a partial symbol table that hasn't been loaded, but > + it may mean that it's from code compiled without -g. > + Partial symbol table searches are kind of expensive; if > + the symbol corresponds to a function, we can find the > + appropriate symtab more quickly by calling > + find_pc_sect_symtab, so let's try that. */ > + > s = find_pc_sect_symtab (SYMBOL_VALUE_ADDRESS (msymbol), > SYMBOL_BFD_SECTION (msymbol)); > if (s != NULL) > @@ -1267,16 +1255,6 @@ lookup_symbol_aux_minsyms (const char *n > *symtab = s; > return fixup_symbol_section (sym, s->objfile); > } > - else if (MSYMBOL_TYPE (msymbol) != mst_text > - && MSYMBOL_TYPE (msymbol) != mst_file_text > - && !STREQ (name, SYMBOL_NAME (msymbol))) > - { > - /* This is a mangled variable, look it up by its > - mangled name. */ > - return lookup_symbol_aux (SYMBOL_NAME (msymbol), mangled_name, > - NULL, namespace, is_a_field_of_this, > - symtab); > - } > } > } > > @@ -2896,31 +2874,12 @@ search_symbols (char *regexp, namespace_ > { > if (0 == find_pc_symtab (SYMBOL_VALUE_ADDRESS (msymbol))) > { > - if (kind == FUNCTIONS_NAMESPACE) > - { > - found_misc = 1; > - } > - else > - { > - struct symbol *sym; > - > - if (SYMBOL_DEMANGLED_NAME (msymbol) != NULL) > - sym > - = lookup_symbol_aux_minsyms (SYMBOL_DEMANGLED_NAME > - (msymbol), > - SYMBOL_NAME (msymbol), > - VAR_NAMESPACE, > - NULL, NULL); > - else > - sym > - = lookup_symbol_aux_minsyms (SYMBOL_NAME (msymbol), > - NULL, > - VAR_NAMESPACE, > - NULL, NULL); > - > - if (sym == NULL) > - found_misc = 1; > - } > + if (kind == FUNCTIONS_NAMESPACE > + || lookup_symbol (SYMBOL_NAME (msymbol), > + (struct block *) NULL, > + VAR_NAMESPACE, > + 0, (struct symtab **) NULL) == NULL) > + found_misc = 1; > } > } > }