[patch 5/7] STT_GNU_IFUNC symbols reader

[patch 5/7] STT_GNU_IFUNC symbols reader

[Jan Kratochvil]

Hi,

it is questionable if it should not reside in gdb/elf-ifunc.c (like
bfd/elf-ifunc.c).  With the other patches it is over 400 LoC which would make
sense.  Unfortunately it needs some internal functions of elfread.c which
would need to be exposed, probably via new elfread.h etc., I have no problem
with the split but I tried it (one also needs to modify configure.ac for the
ELF exception) and the split was not too clean.

The elf_objfile_gnu_ifunc_cache_data hash table was implemented as minimal
symbols before.  But as these entries are found only during the inferior
runtime install_minimal_symbols has already finished and there is no clean way
how to add new minimal symbols, moreover to make them visible for
<tab>-completion etc.  Therefore it is only an internal GDB cache now.

In the previous post in 2010 were consider the inferior calls very expensive.
GDB now tries the very every method it can find to avoid the inferior call.
Still it will do an inferior call if there is no other possibility.  This also
means one usually can resolve the STT_GNU_IFUNC functions even from core files
now.

I was considering whether to delay the SYMBOL_GOT_PLT_SUFFIX minimal symbols
(those from .got.plt) reading only for the case first STT_GNU_IFUNC is needed.
As the .got.plt section is typically not big and there is rather needed an
unrelated optimization to make the symbols (incl. minimal symbols) reading
lazy and not to touch 150+ symbol files for LibreOffice when only <5 of such
files GDB needs to know I do not consider such optimization relevant now.
With the lazy symbol files reading even the .got.plt reading will get
optimized along.  And for the few really needed files it should not harm.

Another issue is if it should not be rather located in OSABI.  But this is an
ELF feature, which matches neither OS nor ARCH.  Also OSABI has no inheritance
which makes implementation of such global features a bit tedious.
I am interested in the opinion on the OSABI way.


Thanks,
Jan


gdb/
2011-03-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	STT_GNU_IFUNC reader implementation.
	* elfread.c: Include gdbtypes.h, value.h and infcall.h.
	(SYMBOL_GOT_PLT_SUFFIX, elf_rel_plt_read)
	(elf_objfile_gnu_ifunc_cache_data, struct elf_gnu_ifunc_cache)
	(elf_gnu_ifunc_cache_hash, elf_gnu_ifunc_cache_eq)
	(elf_gnu_ifunc_record_cache, elf_gnu_ifunc_resolve_by_cache)
	(elf_gnu_ifunc_resolve_by_got, elf_gnu_ifunc_resolve_name)
	(elf_gnu_ifunc_resolve_addr): New.
	(elf_symfile_read): Call elf_rel_plt_read.
	(elf_gnu_ifunc_fns): New.
	(_initialize_elfread): Initialize elf_objfile_gnu_ifunc_cache_data.
	Install elf_gnu_ifunc_fns.
	* infcall.c (find_function_return_type): New function.
	(find_function_addr): Resolve TYPE_GNU_IFUNC functions, if possible.
	* minsyms.c (stub_gnu_ifunc_resolve_addr)
	(stub_gnu_ifunc_resolve_name): New functions.
	(stub_gnu_ifunc_fns, gnu_ifunc_fns_p): New variables.
	* symtab.h (struct gnu_ifunc_fns, gnu_ifunc_resolve_addr)
	(gnu_ifunc_resolve_name, gnu_ifunc_fns_p): New.

--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -37,6 +37,9 @@
 #include "complaints.h"
 #include "demangle.h"
 #include "psympriv.h"
+#include "gdbtypes.h"
+#include "value.h"
+#include "infcall.h"
 
 extern void _initialize_elfread (void);
 
@@ -57,6 +60,13 @@ struct elfinfo
 
 static void free_elfinfo (void *);
 
+/* Minimal symbols located at the GOT entries for .plt - that is the real
+   pointer where the given entry will jump to.  It gets updated by the real
+   function address during lazy ld.so resolving in the inferior.  These
+   minimal symbols are indexed for <tab>-completion.  */
+
+#define SYMBOL_GOT_PLT_SUFFIX "@got.plt"
+
 /* Locate the segments in ABFD.  */
 
 static struct symfile_segment_data *
@@ -581,6 +591,362 @@ elf_symtab_read (struct objfile *objfile, int type,
   do_cleanups (back_to);
 }
 
+/* Build minimal symbols named `function@got.plt' (see SYMBOL_GOT_PLT_SUFFIX)
+   for later look ups of which function to call when user requests
+   a STT_GNU_IFUNC function.  As the STT_GNU_IFUNC type is found at the target
+   library defining `function' we cannot yet know while reading OBJFILE which
+   of the SYMBOL_GOT_PLT_SUFFIX entries will be needed and later
+   DYN_SYMBOL_TABLE is no longer easily available for OBJFILE.  */
+
+static void
+elf_rel_plt_read (struct objfile *objfile, asymbol **dyn_symbol_table)
+{
+  bfd *obfd = objfile->obfd;
+  const struct elf_backend_data *bed = get_elf_backend_data (obfd);
+  asection *plt, *relplt, *got_plt;
+  unsigned u;
+  int plt_elf_idx;
+  bfd_size_type reloc_count, reloc;
+  char *string_buffer = NULL;
+  size_t string_buffer_size = 0;
+  struct cleanup *back_to;
+  struct gdbarch *gdbarch = objfile->gdbarch;
+  struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
+  size_t ptr_size = TYPE_LENGTH (ptr_type);
+
+  if (objfile->separate_debug_objfile_backlink)
+    return;
+
+  plt = bfd_get_section_by_name (obfd, ".plt");
+  if (plt == NULL)
+    return;
+  plt_elf_idx = elf_section_data (plt)->this_idx;
+
+  got_plt = bfd_get_section_by_name (obfd, ".got.plt");
+  if (got_plt == NULL)
+    return;
+
+  /* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc.  */
+  for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
+    if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
+	&& (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
+	    || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
+      break;
+  if (relplt == NULL)
+    return;
+
+  if (! bed->s->slurp_reloc_table (obfd, relplt, dyn_symbol_table, TRUE))
+    return;
+
+  back_to = make_cleanup (free_current_contents, &string_buffer);
+
+  reloc_count = relplt->size / elf_section_data (relplt)->this_hdr.sh_entsize;
+  for (reloc = 0; reloc < reloc_count; reloc++)
+    {
+      const char *name, *name_got_plt;
+      struct minimal_symbol *msym;
+      CORE_ADDR address;
+      const size_t got_suffix_len = strlen (SYMBOL_GOT_PLT_SUFFIX);
+      size_t name_len;
+
+      name = bfd_asymbol_name (*relplt->relocation[reloc].sym_ptr_ptr);
+      name_len = strlen (name);
+      address = relplt->relocation[reloc].address;
+
+      /* Does the pointer reside in the .got.plt section?  */
+      if (!(bfd_get_section_vma (obfd, got_plt) <= address
+            && address < bfd_get_section_vma (obfd, got_plt)
+			 + bfd_get_section_size (got_plt)))
+	continue;
+
+      /* We cannot check if NAME is a reference to mst_text_gnu_ifunc as in
+	 OBJFILE the symbol is undefined and the objfile having NAME defined
+	 may not yet have been loaded.  */
+
+      if (string_buffer_size < name_len + got_suffix_len)
+	{
+	  string_buffer_size = 2 * (name_len + got_suffix_len);
+	  string_buffer = xrealloc (string_buffer, string_buffer_size);
+	}
+      memcpy (string_buffer, name, name_len);
+      memcpy (&string_buffer[name_len], SYMBOL_GOT_PLT_SUFFIX,
+	      got_suffix_len);
+
+      msym = record_minimal_symbol (string_buffer, name_len + got_suffix_len,
+                                    1, address, mst_slot_got_plt, got_plt,
+				    objfile);
+      if (msym)
+	MSYMBOL_SIZE (msym) = ptr_size;
+    }
+
+  do_cleanups (back_to);
+}
+
+/* The data pointer is htab_t for gnu_ifunc_record_cache_unchecked.  */
+
+static const struct objfile_data *elf_objfile_gnu_ifunc_cache_data;
+
+/* Map function names to CORE_ADDR in elf_objfile_gnu_ifunc_cache_data.  */
+
+struct elf_gnu_ifunc_cache
+{
+  /* This is always a function entry address, not a function descriptor.  */
+  CORE_ADDR addr;
+
+  char name[1];
+};
+
+/* htab_hash for elf_objfile_gnu_ifunc_cache_data.  */
+
+static hashval_t
+elf_gnu_ifunc_cache_hash (const void *a_voidp)
+{
+  const struct elf_gnu_ifunc_cache *a = a_voidp;
+
+  return htab_hash_string (a->name);
+}
+
+/* htab_eq for elf_objfile_gnu_ifunc_cache_data.  */
+
+static int
+elf_gnu_ifunc_cache_eq (const void *a_voidp, const void *b_voidp)
+{
+  const struct elf_gnu_ifunc_cache *a = a_voidp;
+  const struct elf_gnu_ifunc_cache *b = b_voidp;
+
+  return strcmp (a->name, b->name) == 0;
+}
+
+/* Record the target function address of a STT_GNU_IFUNC function NAME is the
+   function entry address ADDR.  Return 1 if NAME and ADDR are considered as
+   valid and therefore they were successfully recorded, return 0 otherwise.
+
+   Function does not expect a duplicate entry.  Use
+   elf_gnu_ifunc_resolve_by_cache first to check if the entry for NAME already
+   exists.  */
+
+static int
+elf_gnu_ifunc_record_cache (const char *name, CORE_ADDR addr)
+{
+  struct minimal_symbol *msym;
+  asection *sect;
+  struct objfile *objfile;
+  htab_t htab;
+  struct elf_gnu_ifunc_cache entry_local, *entry_p;
+  void **slot;
+
+  msym = lookup_minimal_symbol_by_pc (addr);
+  if (msym == NULL)
+    return 0;
+  if (SYMBOL_VALUE_ADDRESS (msym) != addr)
+    return 0;
+  /* minimal symbols have always SYMBOL_OBJ_SECTION non-NULL.  */
+  sect = SYMBOL_OBJ_SECTION (msym)->the_bfd_section;
+  objfile = SYMBOL_OBJ_SECTION (msym)->objfile;
+
+  /* If .plt jumps back to .plt the symbol is still deferred for later
+     resolution and it has no use for GDB.  Besides ".text" this symbol can
+     reside also in ".opd" for ppc64 function descriptor.  */
+  if (strcmp (bfd_get_section_name (objfile->obfd, sect), ".plt") == 0)
+    return 0;
+
+  htab = objfile_data (objfile, elf_objfile_gnu_ifunc_cache_data);
+  if (htab == NULL)
+    {
+      htab = htab_create_alloc_ex (1, elf_gnu_ifunc_cache_hash,
+				   elf_gnu_ifunc_cache_eq,
+				   NULL, &objfile->objfile_obstack,
+				   hashtab_obstack_allocate,
+				   dummy_obstack_deallocate);
+      set_objfile_data (objfile, elf_objfile_gnu_ifunc_cache_data, htab);
+    }
+
+  entry_local.addr = addr;
+  obstack_grow (&objfile->objfile_obstack, &entry_local,
+		offsetof (struct elf_gnu_ifunc_cache, name));
+  obstack_grow_str0 (&objfile->objfile_obstack, name);
+  entry_p = obstack_finish (&objfile->objfile_obstack);
+
+  slot = htab_find_slot (htab, entry_p, INSERT);
+  if (*slot != NULL)
+    {
+      struct elf_gnu_ifunc_cache *entry_found_p = *slot;
+      struct gdbarch *gdbarch = objfile->gdbarch;
+
+      if (entry_found_p->addr != addr)
+	{
+	  /* This case indicates buggy inferior program, the resolved address
+	     should never change.  */
+
+	    warning (_("gnu-indirect-function \"%s\" has changed its resolved "
+		       "function_address from %s to %s"),
+		     name, paddress (gdbarch, entry_found_p->addr),
+		     paddress (gdbarch, addr));
+	}
+
+      /* New ENTRY_P is here leaked/duplicate in the OBJFILE obstack.  */
+    }
+  *slot = entry_p;
+
+  return 1;
+}
+
+/* Try to find the target resolved function entry address of a STT_GNU_IFUNC
+   function NAME.  If the address is found it is stored to *ADDR_P (if ADDR_P
+   is not NULL) and the function returns 1.  It returns 0 otherwise.
+
+   Only the elf_objfile_gnu_ifunc_cache_data hash table is searched by this
+   function.  */
+
+static int
+elf_gnu_ifunc_resolve_by_cache (const char *name, CORE_ADDR *addr_p)
+{
+  struct objfile *objfile;
+
+  ALL_PSPACE_OBJFILES (current_program_space, objfile)
+    {
+      htab_t htab;
+      struct elf_gnu_ifunc_cache *entry_p;
+      void **slot;
+
+      htab = objfile_data (objfile, elf_objfile_gnu_ifunc_cache_data);
+      if (htab == NULL)
+	continue;
+
+      entry_p = alloca (sizeof (*entry_p) + strlen (name));
+      strcpy (entry_p->name, name);
+
+      slot = htab_find_slot (htab, entry_p, NO_INSERT);
+      if (slot == NULL)
+	continue;
+      entry_p = *slot;
+      gdb_assert (entry_p != NULL);
+
+      if (addr_p)
+	*addr_p = entry_p->addr;
+      return 1;
+    }
+
+  return 0;
+}
+
+/* Try to find the target resolved function entry address of a STT_GNU_IFUNC
+   function NAME.  If the address is found it is stored to *ADDR_P (if ADDR_P
+   is not NULL) and the function returns 1.  It returns 0 otherwise.
+
+   Only the SYMBOL_GOT_PLT_SUFFIX locations are searched by this function.
+   elf_gnu_ifunc_resolve_by_cache must have been already called for NAME to
+   prevent cache entries duplicates.  */
+
+static int
+elf_gnu_ifunc_resolve_by_got (const char *name, CORE_ADDR *addr_p)
+{
+  char *name_got_plt;
+  struct objfile *objfile;
+  const size_t got_suffix_len = strlen (SYMBOL_GOT_PLT_SUFFIX);
+
+  name_got_plt = alloca (strlen (name) + got_suffix_len + 1);
+  sprintf (name_got_plt, "%s" SYMBOL_GOT_PLT_SUFFIX, name);
+
+  ALL_PSPACE_OBJFILES (current_program_space, objfile)
+    {
+      bfd *obfd = objfile->obfd;
+      struct gdbarch *gdbarch = objfile->gdbarch;
+      struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
+      size_t ptr_size = TYPE_LENGTH (ptr_type);
+      CORE_ADDR pointer_address, addr;
+      asection *plt;
+      gdb_byte *buf = alloca (ptr_size);
+      struct minimal_symbol *msym;
+
+      msym = lookup_minimal_symbol (name_got_plt, NULL, objfile);
+      if (msym == NULL)
+	continue;
+      if (MSYMBOL_TYPE (msym) != mst_slot_got_plt)
+	continue;
+      pointer_address = SYMBOL_VALUE_ADDRESS (msym);
+
+      plt = bfd_get_section_by_name (obfd, ".plt");
+      if (plt == NULL)
+	continue;
+
+      if (MSYMBOL_SIZE (msym) != ptr_size)
+	continue;
+      if (target_read_memory (pointer_address, buf, ptr_size) != 0)
+	continue;
+      addr = extract_typed_address (buf, ptr_type);
+      addr = gdbarch_convert_from_func_ptr_addr (gdbarch, addr,
+						 &current_target);
+
+      if (addr_p)
+	*addr_p = addr;
+      if (elf_gnu_ifunc_record_cache (name, addr))
+	return 1;
+    }
+
+  return 0;
+}
+
+/* Try to find the target resolved function entry address of a STT_GNU_IFUNC
+   function NAME.  If the address is found it is stored to *ADDR_P (if ADDR_P
+   is not NULL) and the function returns 1.  It returns 0 otherwise.
+
+   Both the elf_objfile_gnu_ifunc_cache_data hash table and
+   SYMBOL_GOT_PLT_SUFFIX locations are searched by this function.  */
+
+static int
+elf_gnu_ifunc_resolve_name (const char *name, CORE_ADDR *addr_p)
+{
+  if (elf_gnu_ifunc_resolve_by_cache (name, addr_p))
+    return 1;
+  
+  if (elf_gnu_ifunc_resolve_by_got (name, addr_p))
+    return 1;
+
+  return 0;
+}
+
+/* Call STT_GNU_IFUNC - a function returning addresss of a real function to
+   call.  PC is theSTT_GNU_IFUNC resolving function entry.  The value returned
+   is the entry point of the resolved STT_GNU_IFUNC target function to call.
+   */
+
+static CORE_ADDR
+elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  char *name_at_pc;
+  CORE_ADDR start_at_pc, address;
+  struct type *func_func_type = builtin_type (gdbarch)->builtin_func_func;
+  struct value *function, *address_val;
+
+  /* Try first any non-intrusive methods without an inferior call.  */
+
+  if (find_pc_partial_function (pc, &name_at_pc, &start_at_pc, NULL)
+      && start_at_pc == pc)
+    {
+      if (elf_gnu_ifunc_resolve_name (name_at_pc, &address))
+	return address;
+    }
+  else
+    name_at_pc = NULL;
+
+  function = allocate_value (func_func_type);
+  set_value_address (function, pc);
+
+  /* STT_GNU_IFUNC resolver functions have no parameters.  FUNCTION is the
+     function entry address.  ADDRESS may be a function descriptor.  */
+
+  address_val = call_function_by_hand (function, 0, NULL);
+  address = value_as_address (address_val);
+  address = gdbarch_convert_from_func_ptr_addr (gdbarch, address,
+						&current_target);
+
+  if (name_at_pc)
+    elf_gnu_ifunc_record_cache (name_at_pc, address);
+
+  return address;
+}
+
 struct build_id
   {
     size_t size;
@@ -821,6 +1187,8 @@ elf_symfile_read (struct objfile *objfile, int symfile_flags)
 	       bfd_errmsg (bfd_get_error ()));
 
       elf_symtab_read (objfile, ST_DYNAMIC, dynsymcount, dyn_symbol_table, 0);
+
+      elf_rel_plt_read (objfile, dyn_symbol_table);
     }
 
   /* Add synthetic symbols - for instance, names for any PLT entries.  */
@@ -1128,8 +1496,19 @@ static const struct sym_fns elf_sym_fns_gdb_index =
   &dwarf2_gdb_index_functions
 };
 
+/* STT_GNU_IFUNC resolver vector to be installed to gnu_ifunc_fns_p.  */
+
+static const struct gnu_ifunc_fns elf_gnu_ifunc_fns =
+{
+  elf_gnu_ifunc_resolve_addr,
+  elf_gnu_ifunc_resolve_name,
+};
+
 void
 _initialize_elfread (void)
 {
   add_symtab_fns (&elf_sym_fns);
+
+  elf_objfile_gnu_ifunc_cache_data = register_objfile_data ();
+  gnu_ifunc_fns_p = &elf_gnu_ifunc_fns;
 }
--- a/gdb/infcall.c
+++ b/gdb/infcall.c
@@ -228,6 +228,21 @@ value_arg_coerce (struct gdbarch *gdbarch, struct value *arg,
   return value_cast (type, arg);
 }
 
+/* Return the return type of a function with its first instruction exactly at
+   the PC address.  Return NULL otherwise.  */
+
+static struct type *
+find_function_return_type (CORE_ADDR pc)
+{
+  struct symbol *sym = find_pc_function (pc);
+
+  if (sym != NULL && BLOCK_START (SYMBOL_BLOCK_VALUE (sym)) == pc
+      && SYMBOL_TYPE (sym) != NULL)
+    return TYPE_TARGET_TYPE (SYMBOL_TYPE (sym));
+
+  return NULL;
+}
+
 /* Determine a function's address and its return type from its value.
    Calls error() if the function is not valid for calling.  */
 
@@ -257,7 +272,19 @@ find_function_addr (struct value *function, struct type **retval_type)
     }
   if (TYPE_CODE (ftype) == TYPE_CODE_FUNC
       || TYPE_CODE (ftype) == TYPE_CODE_METHOD)
-    value_type = TYPE_TARGET_TYPE (ftype);
+    {
+      value_type = TYPE_TARGET_TYPE (ftype);
+
+      if (TYPE_GNU_IFUNC (ftype))
+	{
+	  funaddr = gnu_ifunc_resolve_addr (gdbarch, funaddr);
+
+	  /* Skip querying the function symbol if no RETVAL_TYPE has been
+	     asked for.  */
+	  if (retval_type)
+	    value_type = find_function_return_type (funaddr);
+	}
+    }
   else if (TYPE_CODE (ftype) == TYPE_CODE_INT)
     {
       /* Handle the case of functions lacking debugging info.
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -707,6 +707,39 @@ in_gnu_ifunc_stub (CORE_ADDR pc)
   return msymbol && MSYMBOL_TYPE (msymbol) == mst_text_gnu_ifunc;
 }
 
+/* See elf_gnu_ifunc_resolve_addr for its real implementation.  */
+
+static CORE_ADDR
+stub_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  error (_("GDB cannot resolve STT_GNU_IFUNC symbol at address %s without "
+	   "the ELF support compiled in."),
+	 paddress (gdbarch, pc));
+}
+
+/* See elf_gnu_ifunc_resolve_name for its real implementation.  */
+
+static int
+stub_gnu_ifunc_resolve_name (const char *function_name,
+			     CORE_ADDR *function_address_p)
+{
+  error (_("GDB cannot resolve STT_GNU_IFUNC symbol \"%s\" without "
+	   "the ELF support compiled in."),
+	 function_name);
+}
+
+/* See elf_gnu_ifunc_fns for its real implementation.  */
+
+static const struct gnu_ifunc_fns stub_gnu_ifunc_fns =
+{
+  stub_gnu_ifunc_resolve_addr,
+  stub_gnu_ifunc_resolve_name,
+};
+
+/* A placeholder for &elf_gnu_ifunc_fns.  */
+
+const struct gnu_ifunc_fns *gnu_ifunc_fns_p = &stub_gnu_ifunc_fns;
+
 /* Find the minimal symbol named NAME, and return both the minsym
    struct and its objfile.  This only checks the linkage name.  Sets
    *OBJFILE_P and returns the minimal symbol, if it is found.  If it
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1044,6 +1044,24 @@ extern struct minimal_symbol *lookup_minimal_symbol_by_pc (CORE_ADDR);
 
 extern int in_gnu_ifunc_stub (CORE_ADDR pc);
 
+/* Functions for resolving STT_GNU_IFUNC symbols which are implemented only
+   for ELF symbol files.  */
+
+struct gnu_ifunc_fns
+{
+  /* See elf_gnu_ifunc_resolve_addr for its real implementation.  */
+  CORE_ADDR (*gnu_ifunc_resolve_addr) (struct gdbarch *gdbarch, CORE_ADDR pc);
+
+  /* See elf_gnu_ifunc_resolve_name for its real implementation.  */
+  int (*gnu_ifunc_resolve_name) (const char *function_name,
+				 CORE_ADDR *function_address_p);
+};
+
+#define gnu_ifunc_resolve_addr gnu_ifunc_fns_p->gnu_ifunc_resolve_addr
+#define gnu_ifunc_resolve_name gnu_ifunc_fns_p->gnu_ifunc_resolve_name
+
+extern const struct gnu_ifunc_fns *gnu_ifunc_fns_p;
+
 extern struct minimal_symbol *
     lookup_minimal_symbol_and_objfile (const char *,
 				       struct objfile **);

Re: [patch 5/7] STT_GNU_IFUNC symbols reader

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> it is questionable if it should not reside in gdb/elf-ifunc.c (like
Jan> bfd/elf-ifunc.c).  With the other patches it is over 400 LoC which
Jan> would make sense.  Unfortunately it needs some internal functions
Jan> of elfread.c which would need to be exposed, probably via new
Jan> elfread.h etc., I have no problem with the split but I tried it
Jan> (one also needs to modify configure.ac for the ELF exception) and
Jan> the split was not too clean.

What you did seems fine to me.

Jan> The elf_objfile_gnu_ifunc_cache_data hash table was implemented as
Jan> minimal symbols before.  But as these entries are found only during
Jan> the inferior runtime install_minimal_symbols has already finished
Jan> and there is no clean way how to add new minimal symbols, moreover
Jan> to make them visible for <tab>-completion etc.  Therefore it is
Jan> only an internal GDB cache now.

Also ok by me.

Jan> I was considering whether to delay the SYMBOL_GOT_PLT_SUFFIX
Jan> minimal symbols (those from .got.plt) reading only for the case
Jan> first STT_GNU_IFUNC is needed.  As the .got.plt section is
Jan> typically not big and there is rather needed an unrelated
Jan> optimization to make the symbols (incl. minimal symbols) reading
Jan> lazy and not to touch 150+ symbol files for LibreOffice when only
Jan> <5 of such files GDB needs to know I do not consider such
Jan> optimization relevant now.  With the lazy symbol files reading even
Jan> the .got.plt reading will get optimized along.  And for the few
Jan> really needed files it should not harm.

I agree.

Jan> Another issue is if it should not be rather located in OSABI.  But
Jan> this is an ELF feature, which matches neither OS nor ARCH.  Also
Jan> OSABI has no inheritance which makes implementation of such global
Jan> features a bit tedious.  I am interested in the opinion on the
Jan> OSABI way.

It seems harmless to do it this way.

Jan> +      htab = htab_create_alloc_ex (1, elf_gnu_ifunc_cache_hash,
Jan> +				   elf_gnu_ifunc_cache_eq,
Jan> +				   NULL, &objfile->objfile_obstack,
Jan> +				   hashtab_obstack_allocate,
Jan> +				   dummy_obstack_deallocate);

It seems just as easy to allocate the hash table so that rehashing
doesn't waste memory.

Tom

Re: [patch 5/7] STT_GNU_IFUNC symbols reader

[Jan Kratochvil]

On Mon, 21 Mar 2011 21:45:29 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> Jan> +      htab = htab_create_alloc_ex (1, elf_gnu_ifunc_cache_hash,
> Jan> +				   elf_gnu_ifunc_cache_eq,
> Jan> +				   NULL, &objfile->objfile_obstack,
> Jan> +				   hashtab_obstack_allocate,
> Jan> +				   dummy_obstack_deallocate);
> 
> It seems just as easy to allocate the hash table so that rehashing
> doesn't waste memory.

To clarify the comment:

This statement is in elf_gnu_ifunc_record_cache, HTAB is used only for the
cache of STT_GNU_IFUNC resolved addresses where GDB explicitly had to figure
out the address due to a user request for breakpoint on that function.

That is usually only few of the functions will ever get stored by
elf_gnu_ifunc_record_cache into that hash even if the objfile has many
STT_GNU_IFUNC symbols.

Did you mean that GDB should make the hash size its maximum possible one, by
counting the STT_GNU_IFUNC symbols in that objfile?  I understand even an
occasional rehashing is more expensive than a larger than needed initial
allocation each time.

BTW the rehashing using non-deallocating hashtab_obstack_allocate already
commonly happens in current FSF GDB code.

While I do not want to oppose your constructive comment still I have to note
the value 1 may be IMO the optimal one during most of the GDB use cases. :-)


Thanks,
Jan

Re: [patch 5/7] STT_GNU_IFUNC symbols reader

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> Did you mean that GDB should make the hash size its maximum
Jan> possible one, by counting the STT_GNU_IFUNC symbols in that
Jan> objfile?

Nope, I was referring to allocating the hash on an obstack.

Jan> BTW the rehashing using non-deallocating hashtab_obstack_allocate
Jan> already commonly happens in current FSF GDB code.

Yeah, but I don't like that.  It isn't a big issue if you want to keep
it as-is.

Tom

[commit] Re: [patch 5/7] STT_GNU_IFUNC symbols reader

[Jan Kratochvil]

On Mon, 28 Mar 2011 20:41:47 +0200, Tom Tromey wrote:
> Yeah, but I don't like that.  It isn't a big issue if you want to keep
> it as-is.

Left it as-is.  All the uses of hashtab_obstack_allocate could be reviewed
although it is also no concern in this case IMO.

The hash size 1 gets mapped to the next highest prime 7 which is also high
enough so I have left there 1, also other GDB code is using size 1 there.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-03/msg00313.html


Thanks,
Jan