From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1631 invoked by alias); 5 Aug 2019 15:33:03 -0000 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 Received: (qmail 1623 invoked by uid 89); 5 Aug 2019 15:33:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-30.1 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,RCVD_IN_JMF_BL,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=biesinger, Biesinger, bsc, HX-Envelope-From:sk:cbiesin X-HELO: mail-oi1-f193.google.com Received: from mail-oi1-f193.google.com (HELO mail-oi1-f193.google.com) (209.85.167.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 05 Aug 2019 15:33:00 +0000 Received: by mail-oi1-f193.google.com with SMTP id w196so40995908oie.7 for ; Mon, 05 Aug 2019 08:33:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=lT5anrnfTfyqxXHu4E2f0NbNnzpsrI1zuqaQ1Knt8KE=; b=LkADxa4AQnBi7BguBtnWfgd/5x4Pv5DEMR91gt5NKUCIduUk6KWEYWAMNYPqTtYPmU W2sRM7ae6qrGa7/ZNEB5LveBYHUPsg4muBZ3J10rwcs8zxzBz6jGo35a2z/kgD4/1HGu rmSVtO8JzaBWLnqTfnj0/0FC+mUw64mpLlulC4uA9qe0P8b8gOQlOJrA/AxI1mROXgml u79jq+FUmYHZzodGVApG7xXoIFd0fHJOCoMJuSVDisaB052dtzMvae/HWrUSoFQ63zNc jkvmYM5So54wb0axl01rUmSiziPY9sRfgkBm50AaoHL9CHlLAqge1zs98a4OfEuwWw/h T06g== MIME-Version: 1.0 References: <20190803010414.34872-1-cbiesinger@google.com> <7200a7d3-7e20-4e2c-7213-7f0b2f88b8f0@simark.ca> In-Reply-To: <7200a7d3-7e20-4e2c-7213-7f0b2f88b8f0@simark.ca> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Mon, 05 Aug 2019 15:33:00 -0000 Message-ID: Subject: Re: [PATCH] Factor out the common code in lookup_{static,global}_symbol To: Simon Marchi Cc: Christian Biesinger via gdb-patches Content-Type: text/plain; charset="UTF-8" X-SW-Source: 2019-08/txt/msg00112.txt.bz2 On Sat, Aug 3, 2019 at 6:41 PM Simon Marchi wrote: > > On 2019-08-02 9:04 p.m., Christian Biesinger via gdb-patches wrote: > > Thanks for the suggestion, that does seem better. > > > > The two functions are extremely similar; this factors out their code into > > a shared _internal function. > > I am reasonably convinced that this is correct. lookup_static_symbol currently > iterates objfiles by simply iterating the linked list. Now it will use > gdbarch_iterate_over_objfiles_in_search_order, with current_objfile == NULL. The > only gdbarch that implements it is Windows (windows_iterate_over_objfiles_in_search_order) > and with current_objfile == NULL, it goes back to iterating the objfiles linked list directly. > > So there shouldn't be any functional change. Yes, that was my thinking too. > > gdb/ChangeLog: > > > > 2019-08-02 Christian Biesinger > > > > * symtab.c (lookup_static_symbol): Call the new function (and move > > it down to be next to lookup_global_symbol). > > (struct global_sym_lookup_data): Add block_enum member. > > (lookup_symbol_global_iterator_cb): Pass block_index instead of > > GLOBAL_BLOCK to lookup_symbol_in_objfile. > > (lookup_global_symbol_internal): New function. > > (lookup_global_symbol): Call new function. > > --- > > gdb/symtab.c | 81 +++++++++++++++++++++++----------------------------- > > 1 file changed, 36 insertions(+), 45 deletions(-) > > > > diff --git a/gdb/symtab.c b/gdb/symtab.c > > index 87a0c8e4da..55df92f3e0 100644 > > --- a/gdb/symtab.c > > +++ b/gdb/symtab.c > > @@ -2566,44 +2566,6 @@ lookup_symbol_in_objfile (struct objfile *objfile, enum block_enum block_index, > > return result; > > } > > > > -/* See symtab.h. */ > > - > > -struct block_symbol > > -lookup_static_symbol (const char *name, const domain_enum domain) > > -{ > > - struct symbol_cache *cache = get_symbol_cache (current_program_space); > > - struct block_symbol result; > > - struct block_symbol_cache *bsc; > > - struct symbol_cache_slot *slot; > > - > > - /* Lookup in STATIC_BLOCK is not current-objfile-dependent, so just pass > > - NULL for OBJFILE_CONTEXT. */ > > - result = symbol_cache_lookup (cache, NULL, STATIC_BLOCK, name, domain, > > - &bsc, &slot); > > - if (result.symbol != NULL) > > - { > > - if (SYMBOL_LOOKUP_FAILED_P (result)) > > - return {}; > > - return result; > > - } > > - > > - for (objfile *objfile : current_program_space->objfiles ()) > > - { > > - result = lookup_symbol_in_objfile (objfile, STATIC_BLOCK, name, domain); > > - if (result.symbol != NULL) > > - { > > - /* Still pass NULL for OBJFILE_CONTEXT here. */ > > - symbol_cache_mark_found (bsc, slot, NULL, result.symbol, > > - result.block); > > - return result; > > - } > > - } > > - > > - /* Still pass NULL for OBJFILE_CONTEXT here. */ > > - symbol_cache_mark_not_found (bsc, slot, NULL, name, domain); > > - return {}; > > -} > > - > > /* Private data to be used with lookup_symbol_global_iterator_cb. */ > > > > struct global_sym_lookup_data > > @@ -2614,6 +2576,9 @@ struct global_sym_lookup_data > > /* The domain to use for our search. */ > > domain_enum domain; > > > > + /* The block index in which to search */ > > + enum block_enum block_index; > > + > > /* The field where the callback should store the symbol if found. > > It should be initialized to {NULL, NULL} before the search is started. */ > > struct block_symbol result; > > @@ -2634,7 +2599,7 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile, > > gdb_assert (data->result.symbol == NULL > > && data->result.block == NULL); > > > > - data->result = lookup_symbol_in_objfile (objfile, GLOBAL_BLOCK, > > + data->result = lookup_symbol_in_objfile (objfile, data->block_index, > > data->name, data->domain); > > > > /* If we found a match, tell the iterator to stop. Otherwise, > > @@ -2642,12 +2607,14 @@ lookup_symbol_global_iterator_cb (struct objfile *objfile, > > return (data->result.symbol != NULL); > > } > > > > -/* See symtab.h. */ > > - > > +/* This function contains the common code of lookup_{global,static}_symbol. > > + BLOCK is only used if BLOCK_INDEX is GLOBAL_SCOPE, in which case it is > > + used to retrieve the objfile to start the lookup in. */ > > struct block_symbol > > -lookup_global_symbol (const char *name, > > - const struct block *block, > > - const domain_enum domain) > > +lookup_global_symbol_internal (const char *name, > > + enum block_enum block_index, > > + const struct block *block, > > + const domain_enum domain) > > Make this function static. And to be pedant, add a newline between the doc and function name. Oops, thanks. Will send a new patch in a moment. > > { > > struct symbol_cache *cache = get_symbol_cache (current_program_space); > > struct block_symbol result; > > @@ -2656,11 +2623,14 @@ lookup_global_symbol (const char *name, > > struct block_symbol_cache *bsc; > > struct symbol_cache_slot *slot; > > > > + gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK); > > + gdb_assert (block == nullptr || block_index == GLOBAL_BLOCK); > > + > > objfile = lookup_objfile_from_block (block); > > > > /* First see if we can find the symbol in the cache. > > This works because we use the current objfile to qualify the lookup. */ > > - result = symbol_cache_lookup (cache, objfile, GLOBAL_BLOCK, name, domain, > > + result = symbol_cache_lookup (cache, objfile, block_index, name, domain, > > &bsc, &slot); > > if (result.symbol != NULL) > > { > > @@ -2678,6 +2648,7 @@ lookup_global_symbol (const char *name, > > { > > memset (&lookup_data, 0, sizeof (lookup_data)); > > lookup_data.name = name; > > + lookup_data.block_index = block_index; > > lookup_data.domain = domain; > > gdbarch_iterate_over_objfiles_in_search_order > > (objfile != NULL ? get_objfile_arch (objfile) : target_gdbarch (), > > @@ -2693,6 +2664,26 @@ lookup_global_symbol (const char *name, > > return result; > > } > > > > +/* See symtab.h. */ > > + > > +struct block_symbol > > +lookup_static_symbol (const char *name, const domain_enum domain) > > +{ > > + /* TODO: Should static symbol lookup start with a block as well, so we can > > + prefer a static symbol in the current CU? */ > > That's a good question. So one of the things using lookup_static_symbol is the > gdb.lookup_static_symbol Python function you recently added. Right now, it is not > context sensitive. For example, here I have two files with each a static variable > `allo`, one with value 22 and the other with value 33: > > (gdb) python print(gdb.lookup_static_symbol('allo').value()) > 22 > (gdb) print allo > $1 = 22 > (gdb) s > fonction () at test2.c:6 > 6 printf("allo = %d\n", allo); > (gdb) python print(gdb.lookup_static_symbol('allo').value()) > 22 > (gdb) print allo > $2 = 33 > > As you can see, gdb.lookup_static_symbol will always find the same symbol, > regardless of the current context, unlike `print`, which ends up using lookup_symbol_aux. > Should functions like gdb.lookup_static_symbol implicitly prioritize the current context > (e.g. current CU)? I think something like that makes sense for the command line of GDB, > used interactively. But for an API, is it strange to get different results when calling > gdb.lookup_static_symbol with the same parameters at different times? Yeah, I agree that the python one should not be context-sensitive. But take, for example, the call to lookup_static_symbol at the end of lookup_symbol_aux -- shouldn't that take the context into account? For the one in cp_lookup_nested_symbol_1 I can't quite tell if it makes a difference, though I think the code there only looks in the current CU and probably should prefer the current objfile for static symbols as well. Etc. Christian