From patchwork Fri Nov 7 09:07:19 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 3607 Received: (qmail 12404 invoked by alias); 7 Nov 2014 09:08:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 12386 invoked by uid 89); 7 Nov 2014 09:08:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pa0-f42.google.com Received: from mail-pa0-f42.google.com (HELO mail-pa0-f42.google.com) (209.85.220.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 07 Nov 2014 09:08:13 +0000 Received: by mail-pa0-f42.google.com with SMTP id bj1so3182265pad.1 for ; Fri, 07 Nov 2014 01:08:11 -0800 (PST) X-Received: by 10.70.135.196 with SMTP id pu4mr10675472pdb.43.1415351291435; Fri, 07 Nov 2014 01:08:11 -0800 (PST) Received: from seba.sebabeach.org.gmail.com (173-13-178-53-sfba.hfc.comcastbusiness.net. [173.13.178.53]) by mx.google.com with ESMTPSA id ej7sm8049516pdb.88.2014.11.07.01.08.10 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 07 Nov 2014 01:08:10 -0800 (PST) From: Doug Evans To: Yao Qi Cc: Subject: Re: [PATCH 7/9] Rewrite lookup_static_symbol to use gdbarch routine References: <87d29cvqfu.fsf@codesourcery.com> Date: Fri, 07 Nov 2014 01:07:19 -0800 In-Reply-To: (Doug Evans's message of "Wed, 05 Nov 2014 12:24:58 -0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Doug Evans writes: > Yao Qi writes: >> Doug Evans writes: >> >>> struct symbol * >>> lookup_static_symbol_aux (const char *name, const domain_enum domain) >>> { >>> struct objfile *objfile; >>> struct symbol *sym; >>> >>> sym = lookup_symbol_aux_symtabs (STATIC_BLOCK, name, domain); >>> if (sym != NULL) >>> return sym; >>> >>> ALL_OBJFILES (objfile) >>> { >>> sym = lookup_symbol_aux_quick (objfile, STATIC_BLOCK, name, domain); >>> if (sym != NULL) >>> return sym; >>> } >>> >>> return NULL; >>> } >>> >>> Note what we're doing here. >>> First we're searching over all expanded symtabs in all objfiles, >>> and then we search partial/gdb_index tables in all objfiles. Eh? >>> >>> Normally when looking up a symbol in a particular objfile >>> we first list in expanded symtabs and then look in partial/gdb_index >>> tables. And *then* we try the next objfile. >>> >>> I can't think of any justification for the current behaviour. >>> >>> This patch changes things to be consistent, >>> but it is a behavioural change. >> >> Yes, it changes the behavior. Here is an example in my mind, but not >> sure it is correct or not, say, we have a static int foo defined in two >> objfiles respectively (1.c and 2.c), foo is in the partial table of two >> objfiles, but is only expanded to the full symtab of *one* objfile (2.c). >> >> 1.c 2.c >> partial foo foo >> full foo > > Also note that the context is searching across objfiles, > so 1.c and 2.c are also 1.so and 2.so. > >> before your patch, GDB gets foo from 2.c full table, and after it, GDB >> gets foo from 1.c partial table. Is it possible? > > The question isn't whether it is possible, > the question is whether this is a behavioural change > on which we make some kind of guarantee. > For the task at hand, we should be making a guarantee > related to library search order before making > any kind of guarantee related to internal > implementation details (partial vs full syms). > [I'm not saying we can or should make search order > guarantees, per se. Rather, such things should > at least be coherent.] > With this patch we now perform a proper full search > of libraries in a partcular order > (however that order is defined which is a separate discussion). > Whereas today we could find foo in the last library in the search order, > even if every library has foo, just because someone accessed > some other symbol in the last library and caused the > symtab with foo to be expanded. > >> Regarding the change, we also need to update the comments to >> iterate_over_objfiles_in_search_order in gdbarch.sh, >> >> # Iterate over all objfiles in the order that makes the most sense >> # for the architecture to make global symbol searches. >> ^^^^^^^^^^^^^ > > Ah, righto. > >>> The testsuite passes, and any noticeable difference >>> by this change would be dependent on the order in which >>> symtabs got expanded. Thus I can't think of a reason to not >>> apply this change. >> >> If read this >> >> correctly, Joel expressed the same intention there. > > Righto. > >> Since gdbarch method iterate_over_objfiles_in_search_order was added for >> windows target and this patch uses it, we need to test it on windows target. >> If you don't have mingw testing env, let me know, I'll see if I can do >> the test. > > I'm going to be making more symtab changes so I might as well > get mingw testing going here. > Testing in progress. Hi. I was looking at the callers of lookup_static_symbol, and I think there is more cleanup possible here, but I'd rather not take that on at the moment. So I'm going with a simplified version of my previous patch. Regression tested on amd64-linux. 2014-11-07 Doug Evans * symtab.c (lookup_symbol_in_all_objfiles): Delete. (lookup_static_symbol): Move definition to new location and rewrite. (lookup_symbol_in_objfile): New function. (lookup_symbol_global_iterator_cb): Call it. diff --git a/gdb/symtab.c b/gdb/symtab.c index 2aae04c..ec1c033 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -80,11 +80,6 @@ struct symbol *lookup_local_symbol (const char *name, enum language language); static -struct symbol *lookup_symbol_in_all_objfiles (int block_index, - const char *name, - const domain_enum domain); - -static struct symbol *lookup_symbol_via_quick_fns (struct objfile *objfile, int block_index, const char *name, @@ -1474,28 +1469,6 @@ lookup_symbol_aux (const char *name, const struct block *block, return lookup_static_symbol (name, domain); } -/* See symtab.h. */ - -struct symbol * -lookup_static_symbol (const char *name, const domain_enum domain) -{ - struct objfile *objfile; - struct symbol *sym; - - sym = lookup_symbol_in_all_objfiles (STATIC_BLOCK, name, domain); - if (sym != NULL) - return sym; - - ALL_OBJFILES (objfile) - { - sym = lookup_symbol_via_quick_fns (objfile, STATIC_BLOCK, name, domain); - if (sym != NULL) - return sym; - } - - return NULL; -} - /* Check to see if the symbol is defined in BLOCK or its superiors. Don't search STATIC_BLOCK or GLOBAL_BLOCK. */ @@ -1650,27 +1623,6 @@ lookup_symbol_in_objfile_symtabs (struct objfile *objfile, int block_index, return NULL; } -/* Wrapper around lookup_symbol_in_objfile_symtabs to search all objfiles. - Returns the first match found. */ - -static struct symbol * -lookup_symbol_in_all_objfiles (int block_index, const char *name, - const domain_enum domain) -{ - struct symbol *sym; - struct objfile *objfile; - - ALL_OBJFILES (objfile) - { - sym = lookup_symbol_in_objfile_symtabs (objfile, block_index, name, - domain); - if (sym) - return sym; - } - - return NULL; -} - /* Wrapper around lookup_symbol_in_objfile_symtabs for search_symbols. Look up LINKAGE_NAME in DOMAIN in the global and static blocks of OBJFILE and all related objfiles. */ @@ -1774,7 +1726,7 @@ basic_lookup_symbol_nonlocal (const char *name, not it would be appropriate to search the current global block here as well. (That's what this code used to do before the is_a_field_of_this check was moved up.) On the one hand, it's - redundant with the lookup_symbol_in_all_objfiles search that happens + redundant with the lookup in all objfiles search that happens next. On the other hand, if decode_line_1 is passed an argument like filename:var, then the user presumably wants 'var' to be searched for in filename. On the third hand, there shouldn't be @@ -1818,6 +1770,46 @@ lookup_symbol_in_static_block (const char *name, return NULL; } +/* Perform the standard symbol lookup of NAME in OBJFILE: + 1) First search expanded symtabs, and if not found + 2) Search the "quick" symtabs (partial or .gdb_index). + BLOCK_INDEX is one of GLOBAL_BLOCK or STATIC_BLOCK. */ + +static struct symbol * +lookup_symbol_in_objfile (struct objfile *objfile, int block_index, + const char *name, const domain_enum domain) +{ + struct symbol *result; + + result = lookup_symbol_in_objfile_symtabs (objfile, block_index, + name, domain); + if (result == NULL) + { + result = lookup_symbol_via_quick_fns (objfile, block_index, + name, domain); + } + + return result; +} + +/* See symtab.h. */ + +struct symbol * +lookup_static_symbol (const char *name, const domain_enum domain) +{ + struct objfile *objfile; + struct symbol *result; + + ALL_OBJFILES (objfile) + { + result = lookup_symbol_in_objfile (objfile, STATIC_BLOCK, name, domain); + if (result != NULL) + return result; + } + + return NULL; +} + /* Private data to be used with lookup_symbol_global_iterator_cb. */ struct global_sym_lookup_data @@ -1847,11 +1839,8 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile, gdb_assert (data->result == NULL); - data->result = lookup_symbol_in_objfile_symtabs (objfile, GLOBAL_BLOCK, - data->name, data->domain); - if (data->result == NULL) - data->result = lookup_symbol_via_quick_fns (objfile, GLOBAL_BLOCK, - data->name, data->domain); + data->result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, + data->name, data->domain); /* If we found a match, tell the iterator to stop. Otherwise, keep going. */