This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Factor out the common code in lookup_{static,global}_symbol
On Mon, Aug 5, 2019 at 10:32 AM Christian Biesinger
<cbiesinger@google.com> wrote:
>
> On Sat, Aug 3, 2019 at 6:41 PM Simon Marchi <simark@simark.ca> 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 <cbiesinger@google.com>
> > >
> > > * 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