combine bfd_lookup_symbol in solib-*.c

combine bfd_lookup_symbol in solib-*.c

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

>> > +static CORE_ADDR
>> > +bfd_lookup_symbol (bfd *abfd, char *symname)
> I now notice that we already have copies of this
> function in solib-svr4.c, solib-frv.c and solib-pa64.c
> could could be combined.  Given the precedent, it's okay
> to leave that for a follow up.
>

During the tic6x patches review, Pedro pointed out the duplication of
bfd_lookup_symbol cross solib-svr4.c, solib-frv.c and solib-pa64.c.
This patch is to remove the duplication.

Four instances of bfd_lookup_symbol is not exactly the same, and can be
grouped into three 1) solib-svr4.c 2) solib-frv.c and solib-dsbt.c, 3)
solib-pa64.  In this patch, I split original version into two functions
bfd_lookup_symbol_from_symtab and bfd_lookup_symbol_from_dyn_symtab, and
move them to solib.c, so that they can be reused easily.  A helper
function, as a parameter, is introduced to hide the difference on
comparing symbol name and checking section flag.

There is still minor duplications in this new patch, which is helper
function (cmp_name) defined in each solib-{frv,pa64,dsbt}.c
respectively.  Since I don't want helper_function be visible out of
file, so this duplication is acceptable to me.

Regression tested x86_64-pc-linux-gnu.  OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: 0007-combine-bfd_lookup_symbol.patch --]
[-- Type: text/x-patch, Size: 16436 bytes --]


        gdb/
        * solib-dsbt.c (bfd_lookup_symbol): Removed.
        (cmp_name): New.
        (enable_break2): Update caller.
        * solib-frv.c (bfd_lookup_symbol): Removed.
        (cmp_name): New.
        (enable_break2): Update caller.
        * solib-pa64.c (bfd_lookup_symbol): Removed.
        (cmp_name): New.
        * solib-svr4.c (bfd_lookup_symbol): Removed.
        (cmp_name_and_sec_flags): New.
        (enable_break): Update caller.
        * solib.c (bfd_lookup_symbol_from_symtab): New.
        (bfd_lookup_symbol_from_dyn_symtab): New.
        (bfd_lookup_symbol): New.
        * solib.h: Functions declarations.
---
 gdb/solib-dsbt.c |   80 +++++-----------------------------------------
 gdb/solib-frv.c  |   82 ++++++-----------------------------------------
 gdb/solib-pa64.c |   48 +++------------------------
 gdb/solib-svr4.c |   91 ++++++-----------------------------------------------
 gdb/solib.c      |   93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib.h      |   12 +++++++
 6 files changed, 140 insertions(+), 266 deletions(-)

diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 2569395..2922731 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -508,76 +508,6 @@ scan_dyntag (int dyntag, bfd *abfd, CORE_ADDR *ptr)
   return 0;
 }
 
-/* An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   interface structures in the shared library.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, char *symname)
-{
-  long storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative. */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  if (symaddr)
-    return symaddr;
-
-  /* Look for the symbol in the dynamic string table too.  */
-
-  storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative. */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  return symaddr;
-}
-
-
 /* If no open symbol file, attempt to locate and open the main symbol
    file.
 
@@ -852,6 +782,14 @@ enable_break_failure_warning (void)
 	   "and track explicitly loaded dynamic code."));
 }
 
+/* Helper function for bfd_lookup_symbol.  */
+
+static int
+cmp_name (asymbol *sym, const char *name)
+{
+  return (strcmp (sym->name, symname) == 0);
+}
+
 /* The dynamic linkers has, as part of its debugger interface, support
    for arranging for the inferior to hit a breakpoint after mapping in
    the shared libraries.  This function enables that breakpoint.
@@ -957,7 +895,7 @@ enable_break2 (void)
 	    info->interp_plt_sect_low + bfd_section_size (tmp_bfd, interp_sect);
 	}
 
-      addr = bfd_lookup_symbol (tmp_bfd, "_dl_debug_addr");
+      addr = bfd_lookup_symbol (tmp_bfd, "_dl_debug_addr", cmp_name);
       if (addr == 0)
 	{
 	  warning (_("Could not find symbol _dl_debug_addr in dynamic linker"));
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index e1c16d9..d412b61 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -239,77 +239,6 @@ static void frv_relocate_main_executable (void);
 static CORE_ADDR main_got (void);
 static int enable_break2 (void);
 
-/* Lookup the value for a specific symbol.
-
-   An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   interface structures in the shared library.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, char *symname)
-{
-  long storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  if (symaddr)
-    return symaddr;
-
-  /* Look for the symbol in the dynamic string table too.  */
-
-  storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  return symaddr;
-}
-
 /* Implement the "open_symbol_file_object" target_so_ops method.  */
 
 static int
@@ -554,6 +483,14 @@ enable_break_failure_warning (void)
 	   "and track explicitly loaded dynamic code."));
 }
 
+/* Helper function for bfd_lookup_symbol.  */
+
+static int
+cmp_name (asymbol *sym, const char *name)
+{
+  return (strcmp (sym->name, name) == 0);
+}
+
 /* Arrange for dynamic linker to hit breakpoint.
 
    The dynamic linkers has, as part of its debugger interface, support
@@ -680,7 +617,8 @@ enable_break2 (void)
 	    interp_plt_sect_low + bfd_section_size (tmp_bfd, interp_sect);
 	}
 
-      addr = bfd_lookup_symbol (tmp_bfd, "_dl_debug_addr");
+      addr = bfd_lookup_symbol (tmp_bfd, "_dl_debug_addr", cmp_name);
+
       if (addr == 0)
 	{
 	  warning (_("Could not find symbol _dl_debug_addr "
diff --git a/gdb/solib-pa64.c b/gdb/solib-pa64.c
index cd38728..83e179d 100644
--- a/gdb/solib-pa64.c
+++ b/gdb/solib-pa64.c
@@ -269,51 +269,14 @@ read_dynamic_info (asection *dyninfo_sect, dld_cache_t *dld_cache_p)
   return 1;
 }
 
-/* bfd_lookup_symbol -- lookup the value for a specific symbol
+/* Helper function for bfd_lookup_symbol_from_symtab.  */
 
-   An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   interface structures in the shared library.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, char *symname)
+static int
+cmp_name (asymbol *sym, const char *name)
 {
-  unsigned int storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-  return (symaddr);
+  return (strcmp (sym->name, symname) == 0);
 }
 
-
 /* This hook gets called just before the first instruction in the
    inferior process is executed.
 
@@ -421,7 +384,8 @@ manpage for methods to privately map shared library text."));
 	 routine.  */
       load_addr = regcache_read_pc (get_current_regcache ())
 		  - tmp_bfd->start_address;
-      sym_addr = bfd_lookup_symbol (tmp_bfd, "__dld_break");
+      sym_addr = bfd_lookup_symbol_from_symtab (tmp_bfd, "__dld_break",
+						cmp_name);
       sym_addr = load_addr + sym_addr + 4;
       
       /* Create the shared library breakpoint.  */
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index f643a24..d199af4 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -370,86 +370,6 @@ get_svr4_info (void)
 
 static int match_main (const char *);
 
-/* Lookup the value for a specific symbol.
-
-   An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   notification routine in the shared library.
-
-   The returned symbol may be in a code or data section; functions
-   will normally be in a code section, but may be in a data section
-   if this architecture uses function descriptors.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, const char *symname)
-{
-  long storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0
-              && (sym->section->flags & (SEC_CODE | SEC_DATA)) != 0)
-	    {
-	      /* BFD symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  if (symaddr)
-    return symaddr;
-
-  /* On FreeBSD, the dynamic linker is stripped by default.  So we'll
-     have to check the dynamic string table too.  */
-
-  storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-
-	  if (strcmp (sym->name, symname) == 0
-              && (sym->section->flags & (SEC_CODE | SEC_DATA)) != 0)
-	    {
-	      /* BFD symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  return symaddr;
-}
-
-
 /* Read program header TYPE from inferior memory.  The header is found
    by scanning the OS auxillary vector.
 
@@ -1253,6 +1173,14 @@ exec_entry_point (struct bfd *abfd, struct target_ops *targ)
 					     targ);
 }
 
+/* Helper function for bfd_lookup_symbol.  */
+
+static int
+cmp_name_and_sec_flags (asymbol *sym, const char *name)
+{
+  return (strcmp (sym->name, name) == 0
+	  && (sym->section->flags & (SEC_CODE | SEC_DATA)) != 0);
+}
 /* Arrange for dynamic linker to hit breakpoint.
 
    Both the SunOS and the SVR4 dynamic linkers have, as part of their
@@ -1501,7 +1429,8 @@ enable_break (struct svr4_info *info, int from_tty)
       /* Now try to set a breakpoint in the dynamic linker.  */
       for (bkpt_namep = solib_break_names; *bkpt_namep != NULL; bkpt_namep++)
 	{
-	  sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep);
+	  sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep,
+					cmp_name_and_sec_flags);
 	  if (sym_addr != 0)
 	    break;
 	}
diff --git a/gdb/solib.c b/gdb/solib.c
index f843723..9fffa49 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1336,6 +1336,99 @@ solib_global_lookup (const struct objfile *objfile,
   return NULL;
 }
 
+CORE_ADDR
+bfd_lookup_symbol_from_symtab (bfd *abfd, const char *symname,
+			       int (*is_right_sym) (asymbol *, const char *))
+{
+  long storage_needed = bfd_get_symtab_upper_bound (abfd);
+  CORE_ADDR symaddr = 0;
+
+  if (storage_needed > 0)
+    {
+      unsigned int i;
+
+      asymbol **symbol_table = (asymbol **) xmalloc (storage_needed);
+      struct cleanup *back_to = make_cleanup (xfree, symbol_table);
+      unsigned int number_of_symbols =
+	bfd_canonicalize_symtab (abfd, symbol_table);
+
+      for (i = 0; i < number_of_symbols; i++)
+	{
+	  asymbol *sym  = *symbol_table++;
+
+	  if (is_right_sym (sym, symname))
+	    {
+	      /* BFD symbols are section relative.  */
+	      symaddr = sym->value + sym->section->vma;
+	      break;
+	    }
+	}
+      do_cleanups (back_to);
+    }
+
+  return symaddr;
+}
+
+static CORE_ADDR
+bfd_lookup_symbol_from_dyn_symtab (bfd *abfd, const char *symname,
+				   int (*is_right_sym) (asymbol *,
+							const char *))
+{
+  long storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
+  CORE_ADDR symaddr = 0;
+
+  if (storage_needed > 0)
+    {
+      unsigned int i;
+      asymbol **symbol_table = (asymbol **) xmalloc (storage_needed);
+      struct cleanup *back_to = make_cleanup (xfree, symbol_table);
+      unsigned int number_of_symbols =
+	bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
+
+      for (i = 0; i < number_of_symbols; i++)
+	{
+	  asymbol *sym = *symbol_table++;
+
+	  if (is_right_sym (sym, symname))
+	    {
+	      /* BFD symbols are section relative.  */
+	      symaddr = sym->value + sym->section->vma;
+	      break;
+	    }
+	}
+      do_cleanups (back_to);
+    }
+  return symaddr;
+}
+
+/* Lookup the value for a specific symbol.
+
+   An expensive way to lookup the value of a single symbol for
+   bfd's that are only temporary anyway.  This is used by the
+   shared library support to find the address of the debugger
+   notification routine in the shared library.
+
+   The returned symbol may be in a code or data section; functions
+   will normally be in a code section, but may be in a data section
+   if this architecture uses function descriptors.
+
+   Note that 0 is specifically allowed as an error return (no
+   such symbol).  */
+
+CORE_ADDR
+bfd_lookup_symbol (bfd *abfd, const char *symname,
+		   int (*is_right_sym) (asymbol *, const char *))
+{
+  CORE_ADDR symaddr = bfd_lookup_symbol_from_symtab (abfd, symname,
+						     is_right_sym);
+
+  /* On FreeBSD, the dynamic linker is stripped by default.  So we'll
+     have to check the dynamic string table too.  */
+  if (symaddr == 0)
+    symaddr = bfd_lookup_symbol_from_dyn_symtab (abfd, symname, is_right_sym);
+
+  return symaddr;
+}
 
 extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
 
diff --git a/gdb/solib.h b/gdb/solib.h
index c473d85..4e788f8 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -78,4 +78,16 @@ extern void set_solib_ops (struct gdbarch *gdbarch,
 
 extern int libpthread_name_p (const char *name);
 
+/* Look up symbol from both symbol table and dynamic string table.  */
+
+extern CORE_ADDR bfd_lookup_symbol (bfd *abfd, const char *symname,
+				    int (*is_right_sym) (asymbol *,
+							 const char *));
+
+/* Look up symbol from symbol table.  */
+
+extern CORE_ADDR bfd_lookup_symbol_from_symtab (bfd *abfd, const char *symname,
+						int (*is_right_sym) (asymbol *,
+								     const char *));
+
 #endif /* SOLIB_H */
-- 
1.7.0.4

[ping] combine bfd_lookup_symbol in solib-*.c

[Yao Qi]

On 08/18/2011 11:36 PM, Yao Qi wrote:
>>>> +static CORE_ADDR
>>>> +bfd_lookup_symbol (bfd *abfd, char *symname)
>> I now notice that we already have copies of this
>> function in solib-svr4.c, solib-frv.c and solib-pa64.c
>> could could be combined.  Given the precedent, it's okay
>> to leave that for a follow up.
>>
>
> During the tic6x patches review, Pedro pointed out the duplication of
> bfd_lookup_symbol cross solib-svr4.c, solib-frv.c and solib-pa64.c.
> This patch is to remove the duplication.
>
> Four instances of bfd_lookup_symbol is not exactly the same, and can be
> grouped into three 1) solib-svr4.c 2) solib-frv.c and solib-dsbt.c, 3)
> solib-pa64.  In this patch, I split original version into two functions
> bfd_lookup_symbol_from_symtab and bfd_lookup_symbol_from_dyn_symtab, and
> move them to solib.c, so that they can be reused easily.  A helper
> function, as a parameter, is introduced to hide the difference on
> comparing symbol name and checking section flag.
>
> There is still minor duplications in this new patch, which is helper
> function (cmp_name) defined in each solib-{frv,pa64,dsbt}.c
> respectively.  Since I don't want helper_function be visible out of
> file, so this duplication is acceptable to me.
>
> Regression tested x86_64-pc-linux-gnu.  OK for mainline?
>

Ping.  http://sourceware.org/ml/gdb-patches/2011-08/msg00356.html

-- 
Yao (齐尧)

Re: combine bfd_lookup_symbol in solib-*.c

[Pedro Alves]

Hi Yao, thanks for doing this.  Looks mostly good.
A couple cosmetic comments below.

On Thursday 18 August 2011 16:36:46, Yao Qi wrote:

> +
> +CORE_ADDR
> +bfd_lookup_symbol (bfd *abfd, const char *symname,
> +                  int (*is_right_sym) (asymbol *, const char *))

All the function does with the passed in SYMNAME is passing
it down to the callback.  If the interface has been generalized to
pass in a "match" callback, then make the symname parameter
generic too, like:

  CORE_ADDR
  bfd_lookup_symbol (bfd *abfd,
                  int (*is_right_sym) (asymbol *, void *),
                  void *data);

Please document the parameters.

I think it's best to reserve extern symbols that start with
"bfd_" to libbfd proper.  Maybe gdb_bfd_lookup_symbol would
be a good enough alternative.  (Or maybe propose the function
for bfd proper, though I'd suggest fixing its interface
to not assume 0 return is error then)

> +/* Lookup the value for a specific symbol.
> +
> +   An expensive way to lookup the value of a single symbol for
> +   bfd's that are only temporary anyway.  This is used by the
> +   shared library support to find the address of the debugger
> +   notification routine in the shared library.
> +
> +   The returned symbol may be in a code or data section; functions
> +   will normally be in a code section, but may be in a data section
> +   if this architecture uses function descriptors.

The way you've generalized the function, since it's the
callback's responsibility to pick the the symbol, this
comment no longer appears to be in the right place.

-- 
Pedro Alves

Re: combine bfd_lookup_symbol in solib-*.c

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On 08/29/2011 07:41 PM, Pedro Alves wrote:

> On Thursday 18 August 2011 16:36:46, Yao Qi wrote:
>
>> +
>> +CORE_ADDR
>> +bfd_lookup_symbol (bfd *abfd, const char *symname,
>> +                  int (*is_right_sym) (asymbol *, const char *))
>
> All the function does with the passed in SYMNAME is passing
> it down to the callback.  If the interface has been generalized to
> pass in a "match" callback, then make the symname parameter
> generic too, like:
>
>    CORE_ADDR
>    bfd_lookup_symbol (bfd *abfd,
>                    int (*is_right_sym) (asymbol *, void *),
>                    void *data);
>
> Please document the parameters.

OK, parameters are documented.

>
> I think it's best to reserve extern symbols that start with
> "bfd_" to libbfd proper.  Maybe gdb_bfd_lookup_symbol would
> be a good enough alternative.  (Or maybe propose the function
> for bfd proper, though I'd suggest fixing its interface
> to not assume 0 return is error then)
>

I agree.  Renamed bfd_* functions to gdb_bfd_*.

>> +/* Lookup the value for a specific symbol.
>> +
>> +   An expensive way to lookup the value of a single symbol for
>> +   bfd's that are only temporary anyway.  This is used by the
>> +   shared library support to find the address of the debugger
>> +   notification routine in the shared library.
>> +
>> +   The returned symbol may be in a code or data section; functions
>> +   will normally be in a code section, but may be in a data section
>> +   if this architecture uses function descriptors.
>
> The way you've generalized the function, since it's the
> callback's responsibility to pick the the symbol, this
> comment no longer appears to be in the right place.
>

OK, removed this comment here.

-- 
Yao (齐尧)

[-- Attachment #2: 0001-combine-bfd_lookup_symbol.patch --]
[-- Type: text/x-patch, Size: 16798 bytes --]

        gdb/
        * solib-dsbt.c (bfd_lookup_symbol): Removed.
        (cmp_name): New.
        (enable_break2): Update caller.
        * solib-frv.c (bfd_lookup_symbol): Removed.
        (cmp_name): New.
        (enable_break2): Update caller.
        * solib-pa64.c (bfd_lookup_symbol): Removed.
        (cmp_name): New.
        * solib-svr4.c (bfd_lookup_symbol): Removed.
        (cmp_name_and_sec_flags): New.
        (enable_break): Update caller.
        * solib.c (gdb_bfd_lookup_symbol_from_symtab): New.
        (gdb_bfd_lookup_symbol_from_dyn_symtab): New.
        (gdb_bfd_lookup_symbol): New.
        * solib.h: Functions declarations.
---
 gdb/solib-dsbt.c |   80 +++++---------------------------------------
 gdb/solib-frv.c  |   82 +++++----------------------------------------
 gdb/solib-pa64.c |   48 +++-----------------------
 gdb/solib-svr4.c |   91 +++++---------------------------------------------
 gdb/solib.c      |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/solib.h      |   14 ++++++++
 6 files changed, 146 insertions(+), 266 deletions(-)

diff --git a/gdb/solib-dsbt.c b/gdb/solib-dsbt.c
index 2569395..1db4537 100644
--- a/gdb/solib-dsbt.c
+++ b/gdb/solib-dsbt.c
@@ -508,76 +508,6 @@ scan_dyntag (int dyntag, bfd *abfd, CORE_ADDR *ptr)
   return 0;
 }
 
-/* An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   interface structures in the shared library.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, char *symname)
-{
-  long storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative. */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  if (symaddr)
-    return symaddr;
-
-  /* Look for the symbol in the dynamic string table too.  */
-
-  storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative. */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  return symaddr;
-}
-
-
 /* If no open symbol file, attempt to locate and open the main symbol
    file.
 
@@ -852,6 +782,14 @@ enable_break_failure_warning (void)
 	   "and track explicitly loaded dynamic code."));
 }
 
+/* Helper function for gdb_bfd_lookup_symbol.  */
+
+static int
+cmp_name (asymbol *sym, const void *data)
+{
+  return (strcmp (sym->name, (const char *) data) == 0);
+}
+
 /* The dynamic linkers has, as part of its debugger interface, support
    for arranging for the inferior to hit a breakpoint after mapping in
    the shared libraries.  This function enables that breakpoint.
@@ -957,7 +895,7 @@ enable_break2 (void)
 	    info->interp_plt_sect_low + bfd_section_size (tmp_bfd, interp_sect);
 	}
 
-      addr = bfd_lookup_symbol (tmp_bfd, "_dl_debug_addr");
+      addr = gdb_bfd_lookup_symbol (tmp_bfd, cmp_name, "_dl_debug_addr");
       if (addr == 0)
 	{
 	  warning (_("Could not find symbol _dl_debug_addr in dynamic linker"));
diff --git a/gdb/solib-frv.c b/gdb/solib-frv.c
index e1c16d9..c70dffb 100644
--- a/gdb/solib-frv.c
+++ b/gdb/solib-frv.c
@@ -239,77 +239,6 @@ static void frv_relocate_main_executable (void);
 static CORE_ADDR main_got (void);
 static int enable_break2 (void);
 
-/* Lookup the value for a specific symbol.
-
-   An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   interface structures in the shared library.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, char *symname)
-{
-  long storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  if (symaddr)
-    return symaddr;
-
-  /* Look for the symbol in the dynamic string table too.  */
-
-  storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  return symaddr;
-}
-
 /* Implement the "open_symbol_file_object" target_so_ops method.  */
 
 static int
@@ -554,6 +483,14 @@ enable_break_failure_warning (void)
 	   "and track explicitly loaded dynamic code."));
 }
 
+/* Helper function for gdb_bfd_lookup_symbol.  */
+
+static int
+cmp_name (asymbol *sym, const void *data)
+{
+  return (strcmp (sym->name, (const char *) data) == 0);
+}
+
 /* Arrange for dynamic linker to hit breakpoint.
 
    The dynamic linkers has, as part of its debugger interface, support
@@ -680,7 +617,8 @@ enable_break2 (void)
 	    interp_plt_sect_low + bfd_section_size (tmp_bfd, interp_sect);
 	}
 
-      addr = bfd_lookup_symbol (tmp_bfd, "_dl_debug_addr");
+      addr = gdb_bfd_lookup_symbol (tmp_bfd, cmp_name, "_dl_debug_addr");
+
       if (addr == 0)
 	{
 	  warning (_("Could not find symbol _dl_debug_addr "
diff --git a/gdb/solib-pa64.c b/gdb/solib-pa64.c
index cd38728..dae99ba 100644
--- a/gdb/solib-pa64.c
+++ b/gdb/solib-pa64.c
@@ -269,51 +269,14 @@ read_dynamic_info (asection *dyninfo_sect, dld_cache_t *dld_cache_p)
   return 1;
 }
 
-/* bfd_lookup_symbol -- lookup the value for a specific symbol
+/* Helper function for gdb_bfd_lookup_symbol_from_symtab.  */
 
-   An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   interface structures in the shared library.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, char *symname)
+static int
+cmp_name (asymbol *sym, const void *data)
 {
-  unsigned int storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0)
-	    {
-	      /* Bfd symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-  return (symaddr);
+  return (strcmp (sym->name, (const char *) data) == 0);
 }
 
-
 /* This hook gets called just before the first instruction in the
    inferior process is executed.
 
@@ -421,7 +384,8 @@ manpage for methods to privately map shared library text."));
 	 routine.  */
       load_addr = regcache_read_pc (get_current_regcache ())
 		  - tmp_bfd->start_address;
-      sym_addr = bfd_lookup_symbol (tmp_bfd, "__dld_break");
+      sym_addr = bfd_lookup_symbol_from_symtab (tmp_bfd, cmp_name,
+						"__dld_break");
       sym_addr = load_addr + sym_addr + 4;
       
       /* Create the shared library breakpoint.  */
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index f643a24..13cad46 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -370,86 +370,6 @@ get_svr4_info (void)
 
 static int match_main (const char *);
 
-/* Lookup the value for a specific symbol.
-
-   An expensive way to lookup the value of a single symbol for
-   bfd's that are only temporary anyway.  This is used by the
-   shared library support to find the address of the debugger
-   notification routine in the shared library.
-
-   The returned symbol may be in a code or data section; functions
-   will normally be in a code section, but may be in a data section
-   if this architecture uses function descriptors.
-
-   Note that 0 is specifically allowed as an error return (no
-   such symbol).  */
-
-static CORE_ADDR
-bfd_lookup_symbol (bfd *abfd, const char *symname)
-{
-  long storage_needed;
-  asymbol *sym;
-  asymbol **symbol_table;
-  unsigned int number_of_symbols;
-  unsigned int i;
-  struct cleanup *back_to;
-  CORE_ADDR symaddr = 0;
-
-  storage_needed = bfd_get_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-	  if (strcmp (sym->name, symname) == 0
-              && (sym->section->flags & (SEC_CODE | SEC_DATA)) != 0)
-	    {
-	      /* BFD symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  if (symaddr)
-    return symaddr;
-
-  /* On FreeBSD, the dynamic linker is stripped by default.  So we'll
-     have to check the dynamic string table too.  */
-
-  storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
-
-  if (storage_needed > 0)
-    {
-      symbol_table = (asymbol **) xmalloc (storage_needed);
-      back_to = make_cleanup (xfree, symbol_table);
-      number_of_symbols = bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
-
-      for (i = 0; i < number_of_symbols; i++)
-	{
-	  sym = *symbol_table++;
-
-	  if (strcmp (sym->name, symname) == 0
-              && (sym->section->flags & (SEC_CODE | SEC_DATA)) != 0)
-	    {
-	      /* BFD symbols are section relative.  */
-	      symaddr = sym->value + sym->section->vma;
-	      break;
-	    }
-	}
-      do_cleanups (back_to);
-    }
-
-  return symaddr;
-}
-
-
 /* Read program header TYPE from inferior memory.  The header is found
    by scanning the OS auxillary vector.
 
@@ -1253,6 +1173,14 @@ exec_entry_point (struct bfd *abfd, struct target_ops *targ)
 					     targ);
 }
 
+/* Helper function for gdb_bfd_lookup_symbol.  */
+
+static int
+cmp_name_and_sec_flags (asymbol *sym, const void *data)
+{
+  return (strcmp (sym->name, (const char *) data) == 0
+	  && (sym->section->flags & (SEC_CODE | SEC_DATA)) != 0);
+}
 /* Arrange for dynamic linker to hit breakpoint.
 
    Both the SunOS and the SVR4 dynamic linkers have, as part of their
@@ -1501,7 +1429,8 @@ enable_break (struct svr4_info *info, int from_tty)
       /* Now try to set a breakpoint in the dynamic linker.  */
       for (bkpt_namep = solib_break_names; *bkpt_namep != NULL; bkpt_namep++)
 	{
-	  sym_addr = bfd_lookup_symbol (tmp_bfd, *bkpt_namep);
+	  sym_addr = gdb_bfd_lookup_symbol (tmp_bfd, cmp_name_and_sec_flags,
+					    (const void *) *bkpt_namep);
 	  if (sym_addr != 0)
 	    break;
 	}
diff --git a/gdb/solib.c b/gdb/solib.c
index f843723..2c0548e 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1336,6 +1336,103 @@ solib_global_lookup (const struct objfile *objfile,
   return NULL;
 }
 
+/* Lookup the value for a specific symbol from dynamic symbol table.  Look
+   up symbol from ABFD.  MATCH_SYM is a callback function to determine
+   whether to pick up a symbol.  DATA is the input of this callback
+   function.  Return NULL if symbol is not found.  */
+
+CORE_ADDR
+gdb_bfd_lookup_symbol_from_symtab (bfd *abfd,
+				   int (*match_sym) (asymbol *, const void *),
+				   const void *data)
+{
+  long storage_needed = bfd_get_symtab_upper_bound (abfd);
+  CORE_ADDR symaddr = 0;
+
+  if (storage_needed > 0)
+    {
+      unsigned int i;
+
+      asymbol **symbol_table = (asymbol **) xmalloc (storage_needed);
+      struct cleanup *back_to = make_cleanup (xfree, symbol_table);
+      unsigned int number_of_symbols =
+	bfd_canonicalize_symtab (abfd, symbol_table);
+
+      for (i = 0; i < number_of_symbols; i++)
+	{
+	  asymbol *sym  = *symbol_table++;
+
+	  if (match_sym (sym, data))
+	    {
+	      /* BFD symbols are section relative.  */
+	      symaddr = sym->value + sym->section->vma;
+	      break;
+	    }
+	}
+      do_cleanups (back_to);
+    }
+
+  return symaddr;
+}
+
+/* Lookup the value for a specific symbol from symbol table.  Look up symbol
+   from ABFD.  MATCH_SYM is a callback function to determine whether to pick
+   up a symbol.  DATA is the input of this callback function.  Return NULL
+   if symbol is not found.  */
+
+static CORE_ADDR
+bfd_lookup_symbol_from_dyn_symtab (bfd *abfd,
+				   int (*match_sym) (asymbol *,
+							const void *),
+				   const void *data)
+{
+  long storage_needed = bfd_get_dynamic_symtab_upper_bound (abfd);
+  CORE_ADDR symaddr = 0;
+
+  if (storage_needed > 0)
+    {
+      unsigned int i;
+      asymbol **symbol_table = (asymbol **) xmalloc (storage_needed);
+      struct cleanup *back_to = make_cleanup (xfree, symbol_table);
+      unsigned int number_of_symbols =
+	bfd_canonicalize_dynamic_symtab (abfd, symbol_table);
+
+      for (i = 0; i < number_of_symbols; i++)
+	{
+	  asymbol *sym = *symbol_table++;
+
+	  if (match_sym (sym, data))
+	    {
+	      /* BFD symbols are section relative.  */
+	      symaddr = sym->value + sym->section->vma;
+	      break;
+	    }
+	}
+      do_cleanups (back_to);
+    }
+  return symaddr;
+}
+
+/* Lookup the value for a specific symbol from symbol table and dynamic
+   symbol table.  Look up symbol from ABFD.  MATCH_SYM is a callback
+   function to determine whether to pick up a symbol.  DATA is the
+   input of this callback function.  Return NULL if symbol is not
+   found.  */
+
+CORE_ADDR
+gdb_bfd_lookup_symbol (bfd *abfd,
+		       int (*match_sym) (asymbol *, const void *),
+		       const void *data)
+{
+  CORE_ADDR symaddr = gdb_bfd_lookup_symbol_from_symtab (abfd, match_sym, data);
+
+  /* On FreeBSD, the dynamic linker is stripped by default.  So we'll
+     have to check the dynamic string table too.  */
+  if (symaddr == 0)
+    symaddr = bfd_lookup_symbol_from_dyn_symtab (abfd, match_sym, data);
+
+  return symaddr;
+}
 
 extern initialize_file_ftype _initialize_solib; /* -Wmissing-prototypes */
 
diff --git a/gdb/solib.h b/gdb/solib.h
index c473d85..54640b2 100644
--- a/gdb/solib.h
+++ b/gdb/solib.h
@@ -78,4 +78,18 @@ extern void set_solib_ops (struct gdbarch *gdbarch,
 
 extern int libpthread_name_p (const char *name);
 
+/* Look up symbol from both symbol table and dynamic string table.  */
+
+extern CORE_ADDR gdb_bfd_lookup_symbol (bfd *abfd,
+					int (*match_sym) (asymbol *,
+							  const void *),
+					const void *data);
+
+/* Look up symbol from symbol table.  */
+
+extern CORE_ADDR bfd_lookup_symbol_from_symtab (bfd *abfd,
+						int (*match_sym) (asymbol *,
+								  const void *),
+						const void *data);
+
 #endif /* SOLIB_H */
-- 
1.7.0.4

Re: combine bfd_lookup_symbol in solib-*.c

[Pedro Alves]

Thanks.

On Monday 29 August 2011 17:22:49, Yao Qi wrote:
> +CORE_ADDR
> +gdb_bfd_lookup_symbol (bfd *abfd,
> +                      int (*match_sym) (asymbol *, const void *),
> +                      const void *data)
> +{

The patch is okay if you remove the "const"s.  It's common to
want to increment a counter in `data' or something like that.

(Try `$ grep iterate_over_ *.h' to see other examples of the pattern).

-- 
Pedro Alves

[committed]: combine bfd_lookup_symbol in solib-*.c

[Yao Qi]

On 08/30/2011 12:33 AM, Pedro Alves wrote:
> Thanks.
>
> On Monday 29 August 2011 17:22:49, Yao Qi wrote:
>> +CORE_ADDR
>> +gdb_bfd_lookup_symbol (bfd *abfd,
>> +                      int (*match_sym) (asymbol *, const void *),
>> +                      const void *data)
>> +{
>
> The patch is okay if you remove the "const"s.  It's common to
> want to increment a counter in `data' or something like that.
>
> (Try `$ grep iterate_over_ *.h' to see other examples of the pattern).
>

OK, "const"s are removed, and committed.
http://sourceware.org/ml/gdb-cvs/2011-08/msg00135.html

-- 
Yao (齐尧)