From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 93638 invoked by alias); 9 Aug 2019 15:28:46 -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 93630 invoked by uid 89); 9 Aug 2019 15:28:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-30.3 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,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=U*cbiesinger, sk:cbiesin, cbiesinger@google.com, biesinger X-HELO: mail-ot1-f67.google.com Received: from mail-ot1-f67.google.com (HELO mail-ot1-f67.google.com) (209.85.210.67) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 Aug 2019 15:28:44 +0000 Received: by mail-ot1-f67.google.com with SMTP id q20so133942507otl.0 for ; Fri, 09 Aug 2019 08:28:44 -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=qpu2+rLjLITyPN+MRw9DfcSz1+ZSw7Sg2pxauu/l9Ik=; b=W+wDgwc+GrjDDL+hB9OJMtlVs+tRJI9OERfCN9OX7Mmz4tqyFATIdW2ZqBEiyjLIdW N5iRrP5Bwd+WhkZdxQYPCT0CabuxO2HBQXkhwQLCwoWmd0T+jBFNSfrBtVvzB85cUOpp YUnbE3uoSxaz27FiqmxvrKD5DtSescCgX7T4TkXkj2nTdOYjvoRfdM22107KBkW8ZY+1 6QkKVXadht6uMjcc3Wp6hpDp6a+Rc0eeAJs+6PFbJKeFMlYYbVRIcpNTmo95WIURkXmR 9LMxBTUvvQq+TJ0uJ98znQS0Um0iSWtN+ATCFwABibKCP8hm/HxZssVwzfTl0KgGV2pg 1KpQ== MIME-Version: 1.0 References: <20190803010414.34872-1-cbiesinger@google.com> <7200a7d3-7e20-4e2c-7213-7f0b2f88b8f0@simark.ca> In-Reply-To: From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Fri, 09 Aug 2019 15:28: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-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00228.txt.bz2 On Mon, Aug 5, 2019 at 10:32 AM Christian Biesinger wrote: > > 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. BTW, if that's the only thing holding up this patch, I'm happy to remove the comment. Christian