Re: [PING] [rfc]: Framework for looking up multiply defined global symbols in shared libraries

[Daniel Jacobowitz <drow at false dot org>]

On Thu, May 10, 2007 at 02:32:10PM +0200, Markus Deuling wrote:
> @@ -945,6 +946,32 @@
>  }
>  
>  
> +/* Handler for library-specific lookup of global symbol
> +   NAME in BLOCK.  Detect objfile corresponding to BLOCK
> +   and call the library-specific handler if it is installed
> +   for the current target.  */
> +
> +struct symbol *
> +solib_global_lookup (const struct block *block,
> +		     const char *name,
> +		     const char *linkage_name,
> +		     const domain_enum domain,
> +		     struct symtab **symtab)

Since all this does with the block is convert it to an objfile, it
should probably be passed an objfile instead of a block.

> diff -urN src/gdb/solib-svr4.c dev/gdb/solib-svr4.c
> --- src/gdb/solib-svr4.c	2007-04-16 13:51:29.000000000 +0200
> +++ dev/gdb/solib-svr4.c	2007-04-26 07:18:50.000000000 +0200
> @@ -378,6 +378,72 @@
>  
>   */
>  
> +/* Scan OBFD for a SYMBOLIC flag
> +   in the .dynamic section. If it is set then
> +   OBFD is a dso with a special rule for symbol
> +   lookup. The lookup then starts in the dso before
> +   searching in the executable.  */

Could you wrap comments around 72 columns (or 79 if you prefer)
instead of this short?  It's harder to read in my humble opinion.

Also, DSO is an acronym so it should be capitalized, and please use
two spaces between each sentence.

> @@ -125,4 +133,13 @@
>  #define TARGET_SO_IN_DYNSYM_RESOLVE_CODE \
>    (current_target_so_ops->in_dynsym_resolve_code)
>  
> +/* Handler for library-specific global symbol lookup
> +   in solib.c.  */
> +struct symbol *
> +solib_global_lookup (const struct block *block,
> +		     const char *name,
> +		     const char *linkage_name,
> +		     const domain_enum domain,
> +		     struct symtab **symtab);
> +

Since this is a prototype, not a definition, the function name should
not be on its own line; grep "^solib_global_lookup" should find only
the definition.

> @@ -1265,6 +1266,44 @@
>    return NULL;
>  }
>  
> +/* Look up objfile to BLOCK.  */
> +
> +struct objfile *
> +lookup_objfile_from_block (const struct block *block)
> +{
> +  struct objfile *obj = NULL;
> +  struct blockvector *bv;
> +  struct block *b;
> +  struct symtab *s;
> +  struct partial_symtab *ps;
> +
> +  if (block == NULL)
> +    return NULL;
> +
> +  /* Go through SYMTABS.  */
> +  ALL_SYMTABS (obj, s)
> +  {
> +    bv = BLOCKVECTOR (s);
> +    b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
> +    if (BLOCK_START (b) <= BLOCK_START (block)
> +	&& BLOCK_END (b) > BLOCK_START (block))
> +      return obj;
> +  }
> +
> +  /* Go through PSYMTABS.  */
> +  ALL_PSYMTABS (obj, ps)
> +  {
> +    s = PSYMTAB_TO_SYMTAB (ps);
> +    bv = BLOCKVECTOR (s);
> +    b = BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK);
> +    if (BLOCK_START (b) <= BLOCK_START (block)
> +	&& BLOCK_END (b) > BLOCK_START (block))
> +      return obj;
> +  }
> +
> +  return NULL;
> +}

There's a much easier way to do this.  Use "block = block_global_block
(block);" at the top, and then compare for equality with
BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK).  You don't need to check the
psymtabs; if it's not already read in, then we wouldn't have read in
BLOCK either, which is impossible.

> @@ -1599,18 +1689,28 @@
>  struct symbol *
>  lookup_symbol_global (const char *name,
>  		      const char *linkage_name,
> +		      const struct block *block,
>  		      const domain_enum domain,
>  		      struct symtab **symtab)
>  {
>    struct symbol *sym;
> +  struct objfile *objfile;
>  
> -  sym = lookup_symbol_aux_symtabs (GLOBAL_BLOCK, name, linkage_name,
> -				   domain, symtab);
> +  /* Call library-specific lookup procedure.  */
> +  sym = solib_global_lookup (block, name, linkage_name, domain, symtab);
>    if (sym != NULL)
>      return sym;
>  
> -  return lookup_symbol_aux_psymtabs (GLOBAL_BLOCK, name, linkage_name,
> -				     domain, symtab);
> +  /* Lookup global symbol in all objfiles in symtab and psymtab.  */
> +  ALL_OBJFILES (objfile)
> +  {
> +    sym = lookup_global_symbol_from_objfile (objfile, name, linkage_name,
> +					     domain, symtab);
> +    if (sym != NULL)
> +      return sym;
> +  }
> +
> +  return NULL;
>  }
>  
>  /* Look, in partial_symtab PST, for symbol whose natural name is NAME.

Why change the search order?  If you do this, you'll find a static
symbol in the second objfile when you should have found a global
symbol in the third objfile first.

> +/* Lookup objfile to a block.  */
> +extern struct objfile *
> +lookup_objfile_from_block (const struct block *block);
> +
> +/* Check global symbols in objfile.  */
> +struct symbol *
> +lookup_global_symbol_from_objfile (struct objfile *objfile,
> +				   const char *name,
> +				   const char *linkage_name,
> +				   const domain_enum domain,
> +				   struct symtab **symtab);

Same comment about formatting.

> diff -urN src/gdb/testsuite/gdb.base/libmd.c dev/gdb/testsuite/gdb.base/libmd.c
> --- src/gdb/testsuite/gdb.base/libmd.c	1970-01-01 01:00:00.000000000 +0100
> +++ dev/gdb/testsuite/gdb.base/libmd.c	2007-04-26 07:18:50.000000000 +0200
> @@ -0,0 +1,18 @@
> +#include <stdio.h>
> +#include "libmd.h"

All test files should get copyright notices, please, unless it would
invalidate whatever is being tested.

> +set prms_id 0
> +set bug_id 0

I think it's safe to omit these.

> +set timeout 5

And please don't adjust the timeout :-)

> +if { [istarget "spu-*"] } then {
> +  unsupported "Skipping shared libraries testcase."
> +  return -1
> +}

Please use skip_shlib_tests instead.  You will probably also need to
exclude a list of targets where -Bsymbolic won't work.

(I realize skip_shlib_tests doesn't exist yet.  Gimme a day or so and
it will, I'll be working on those patches next.)

> +# Build a solib with -Bsymbolic.
> +set compile_flags {debug}
> +set compile_flags "$compile_flags additional_flags=-fPIC"
> +set compile_flags "$compile_flags additional_flags=-Wl,-Bsymbolic"
> +set compile_flags "$compile_flags additional_flags=-shared"
> +set sources ${srcdir}/${subdir}/${srcfile_lib}
> +if { [gdb_compile $sources ${binfile_lib} executable $compile_flags] != "" } {
> +  return -1
> +}

There's a gdb_compile_shlib; if you use that, you won't need to handle
-fPIC or -shared.

> +# Build binary.
> +set compile_flags {debug}
> +set compile_flags "$compile_flags additional_flags=-L${objdir}/${subdir}/"
> +set compile_flags "$compile_flags additional_flags=-Wl,-rpath=${objdir}/${subdir}/"
> +set compile_flags "$compile_flags additional_flags=-lmd"
> +set sources ${srcdir}/${subdir}/${srcfile}

Using shlib= will make this simpler.

> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

Once I check in the other patch, gdb_load_shlibs here.  That doesn't
exist yet either :-)  Just warning you.

> +# Delete all breakpoints.
> +send_gdb "delete breakpoints\n"
> +gdb_expect {
> +  -re "Delete.*all.*breakpoints.*\(y or n\).*" {
> +    send_gdb "y\n"
> +    gdb_expect {
> +      -re "$gdb_prompt.*" { pass "Delete all breakpoints" }
> +      timeout             { fail "Delete all breakpoints (timeout)" }
> +    }
> +  }
> +  -re "$gdb_prompt"       { fail "Delete all breakpoints" }
> +  timeout                 { fail "Delete all breakpoints (timeout)" }
> +}

This is just delete_breakpoints.

-- 
Daniel Jacobowitz
CodeSourcery