This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PR 13897


PR 13897 is regarding a 100x slowdown in objdump -dS output for an old
dot-sym PowerpC64 binary, after a fix I made for bfd_find_nearest_line
on new PowerPC64 binaries.  Nick added some caching in opd_entry_value
that certainly sped things up, but only because a bug made the cache
return the single cached function entry for all opd lookups.  I fixed
the cache but then noticed that it was completely ineffective.  In
fact, the cache further slowed down objdump -dS by a few percent.

My first attempt at fixing the problem removed the ineffective cache
and changed the interface to maybe_function_sym to only return true
for functions in the section of interest.  That saved a loop over
sections in opd_entry_value, giving a 4x speedup.  Not good enough.
We're still 25x slower than the original code which worked fine with
dot-sym binaries.

So I decided to add a cache in elf_find_function.  objdump -dS calls
bfd_find_nearest_line on every instruction, and elf_find_function will
return the same function info for every instruction of a given
function.  This gave around a 50x speedup, so now objdump -dS is
approximately 2x faster than it was before 2012-01-23.

	PR binutils/13897
	* elf.c (elf_find_function): Cache last function sym info.
	(_bfd_elf_maybe_function_sym): Return function size, pass in
	section of interest.
	* elf-bfd.h (struct elf_backend_data <maybe_function_sym>): Likewise.
	(_bfd_elf_maybe_function_sym): Likewise.
	* elf64-ppc.c (ppc64_elf_maybe_function_sym): Likewise.
	(opd_entry_value): Add in_code_sec param.  Revert caching code.
	Return -1 if in_code_sec and function found in wrong section.
	Update all calls.

Index: bfd/elf-bfd.h
===================================================================
RCS file: /cvs/src/src/bfd/elf-bfd.h,v
retrieving revision 1.339
diff -u -p -r1.339 elf-bfd.h
--- bfd/elf-bfd.h	25 May 2012 01:12:19 -0000	1.339
+++ bfd/elf-bfd.h	2 Jun 2012 23:51:12 -0000
@@ -1222,10 +1222,11 @@ struct elf_backend_data
   /* Return TRUE if type is a function symbol type.  */
   bfd_boolean (*is_function_type) (unsigned int type);
 
-  /* Return TRUE if symbol may be a function.  Set *CODE_SEC and *CODE_VAL
-     to the function's entry point.  */
-  bfd_boolean (*maybe_function_sym) (const asymbol *sym,
-				     asection **code_sec, bfd_vma *code_off);
+  /* If the ELF symbol SYM might be a function in SEC, return the
+     function size and set *CODE_OFF to the function's entry point,
+     otherwise return zero.  */
+  bfd_size_type (*maybe_function_sym) (const asymbol *sym, asection *sec,
+				       bfd_vma *code_off);
 
   /* Used to handle bad SHF_LINK_ORDER input.  */
   bfd_error_handler_type link_order_error_handler;
@@ -2207,8 +2208,8 @@ extern bfd_boolean _bfd_elf_map_sections
 
 extern bfd_boolean _bfd_elf_is_function_type (unsigned int);
 
-extern bfd_boolean _bfd_elf_maybe_function_sym (const asymbol *,
-						asection **, bfd_vma *);
+extern bfd_size_type _bfd_elf_maybe_function_sym (const asymbol *, asection *,
+						  bfd_vma *);
 
 extern int bfd_elf_get_default_section_type (flagword);
 
Index: bfd/elf.c
===================================================================
RCS file: /cvs/src/src/bfd/elf.c,v
retrieving revision 1.555
diff -u -p -r1.555 elf.c
--- bfd/elf.c	17 May 2012 06:29:02 -0000	1.555
+++ bfd/elf.c	2 Jun 2012 23:51:13 -0000
@@ -7407,59 +7407,74 @@ elf_find_function (bfd *abfd,
 		   const char **filename_ptr,
 		   const char **functionname_ptr)
 {
-  const char *filename;
-  asymbol *func, *file;
-  bfd_vma low_func;
-  asymbol **p;
-  /* ??? Given multiple file symbols, it is impossible to reliably
-     choose the right file name for global symbols.  File symbols are
-     local symbols, and thus all file symbols must sort before any
-     global symbols.  The ELF spec may be interpreted to say that a
-     file symbol must sort before other local symbols, but currently
-     ld -r doesn't do this.  So, for ld -r output, it is possible to
-     make a better choice of file name for local symbols by ignoring
-     file symbols appearing after a given local symbol.  */
-  enum { nothing_seen, symbol_seen, file_after_symbol_seen } state;
-  const struct elf_backend_data *bed = get_elf_backend_data (abfd);
+  static asection *last_section;
+  static asymbol *func;
+  static const char *filename;
+  static bfd_size_type func_size;
 
   if (symbols == NULL)
     return FALSE;
 
-  filename = NULL;
-  func = NULL;
-  file = NULL;
-  low_func = 0;
-  state = nothing_seen;
-
-  for (p = symbols; *p != NULL; p++)
-    {
-      asymbol *sym = *p;
-      asection *code_sec;
-      bfd_vma code_off;
+  if (last_section != section
+      || func == NULL
+      || offset < func->value
+      || offset >= func->value + func_size)
+    {
+      asymbol *file;
+      bfd_vma low_func;
+      asymbol **p;
+      /* ??? Given multiple file symbols, it is impossible to reliably
+	 choose the right file name for global symbols.  File symbols are
+	 local symbols, and thus all file symbols must sort before any
+	 global symbols.  The ELF spec may be interpreted to say that a
+	 file symbol must sort before other local symbols, but currently
+	 ld -r doesn't do this.  So, for ld -r output, it is possible to
+	 make a better choice of file name for local symbols by ignoring
+	 file symbols appearing after a given local symbol.  */
+      enum { nothing_seen, symbol_seen, file_after_symbol_seen } state;
+      const struct elf_backend_data *bed = get_elf_backend_data (abfd);
+
+      filename = NULL;
+      func = NULL;
+      file = NULL;
+      low_func = 0;
+      state = nothing_seen;
+      func_size = 0;
+      last_section = section;
 
-      if ((sym->flags & BSF_FILE) != 0)
+      for (p = symbols; *p != NULL; p++)
 	{
-	  file = sym;
-	  if (state == symbol_seen)
-	    state = file_after_symbol_seen;
-	  continue;
-	}
+	  asymbol *sym = *p;
+	  bfd_vma code_off;
+	  bfd_size_type size;
 
-      if (bed->maybe_function_sym (sym, &code_sec, &code_off)
-	  && code_sec == section
-	  && code_off >= low_func
-	  && code_off <= offset)
-	{
-	  func = sym;
-	  low_func = code_off;
-	  filename = NULL;
-	  if (file != NULL
-	      && ((sym->flags & BSF_LOCAL) != 0
-		  || state != file_after_symbol_seen))
-	    filename = bfd_asymbol_name (file);
+	  if ((sym->flags & BSF_FILE) != 0)
+	    {
+	      file = sym;
+	      if (state == symbol_seen)
+		state = file_after_symbol_seen;
+	      continue;
+	    }
+
+	  size = bed->maybe_function_sym (sym, section, &code_off);
+	  if (size != 0
+	      && code_off <= offset
+	      && (code_off > low_func
+		  || (code_off == low_func
+		      && size > func_size)))
+	    {
+	      func = sym;
+	      func_size = size;
+	      low_func = code_off;
+	      filename = NULL;
+	      if (file != NULL
+		  && ((sym->flags & BSF_LOCAL) != 0
+		      || state != file_after_symbol_seen))
+		filename = bfd_asymbol_name (file);
+	    }
+	  if (state == nothing_seen)
+	    state = symbol_seen;
 	}
-      if (state == nothing_seen)
-	state = symbol_seen;
     }
 
   if (func == NULL)
@@ -9714,18 +9729,26 @@ _bfd_elf_is_function_type (unsigned int 
 	  || type == STT_GNU_IFUNC);
 }
 
-/* Return TRUE iff the ELF symbol SYM might be a function.  Set *CODE_SEC
-   and *CODE_OFF to the function's entry point.  */
-
-bfd_boolean
-_bfd_elf_maybe_function_sym (const asymbol *sym,
-			     asection **code_sec, bfd_vma *code_off)
+/* If the ELF symbol SYM might be a function in SEC, return the
+   function size and set *CODE_OFF to the function's entry point,
+   otherwise return zero.  */
+
+bfd_size_type
+_bfd_elf_maybe_function_sym (const asymbol *sym, asection *sec,
+			     bfd_vma *code_off)
 {
+  bfd_size_type size;
+
   if ((sym->flags & (BSF_SECTION_SYM | BSF_FILE | BSF_OBJECT
-		     | BSF_THREAD_LOCAL | BSF_RELC | BSF_SRELC)) != 0)
-    return FALSE;
+		     | BSF_THREAD_LOCAL | BSF_RELC | BSF_SRELC)) != 0
+      || sym->section != sec)
+    return 0;
 
-  *code_sec = sym->section;
   *code_off = sym->value;
-  return TRUE;
+  size = 0;
+  if (!(sym->flags & BSF_SYNTHETIC))
+    size = ((elf_symbol_type *) sym)->internal_elf_sym.st_size;
+  if (size == 0)
+    size = 1;
+  return size;
 }
Index: bfd/elf64-ppc.c
===================================================================
RCS file: /cvs/src/src/bfd/elf64-ppc.c,v
retrieving revision 1.386
diff -u -p -r1.386 elf64-ppc.c
--- bfd/elf64-ppc.c	1 Jun 2012 12:26:55 -0000	1.386
+++ bfd/elf64-ppc.c	3 Jun 2012 03:39:12 -0000
@@ -55,7 +55,7 @@ static bfd_reloc_status_type ppc64_elf_t
 static bfd_reloc_status_type ppc64_elf_unhandled_reloc
   (bfd *, arelent *, asymbol *, void *, asection *, bfd *, char **);
 static bfd_vma opd_entry_value
-  (asection *, bfd_vma, asection **, bfd_vma *);
+  (asection *, bfd_vma, asection **, bfd_vma *, bfd_boolean);
 
 #define TARGET_LITTLE_SYM	bfd_elf64_powerpcle_vec
 #define TARGET_LITTLE_NAME	"elf64-powerpcle"
@@ -2347,7 +2347,7 @@ ppc64_elf_branch_reloc (bfd *abfd, arele
     {
       bfd_vma dest = opd_entry_value (symbol->section,
 				      symbol->value + reloc_entry->addend,
-				      NULL, NULL);
+				      NULL, NULL, FALSE);
       if (dest != (bfd_vma) -1)
 	reloc_entry->addend = dest - (symbol->value
 				      + symbol->section->output_section->vma
@@ -5522,7 +5522,8 @@ static bfd_vma
 opd_entry_value (asection *opd_sec,
 		 bfd_vma offset,
 		 asection **code_sec,
-		 bfd_vma *code_off)
+		 bfd_vma *code_off,
+		 bfd_boolean in_code_sec)
 {
   bfd *opd_bfd = opd_sec->owner;
   Elf_Internal_Rela *relocs;
@@ -5533,43 +5534,39 @@ opd_entry_value (asection *opd_sec,
      at a final linked executable with addr2line or somesuch.  */
   if (opd_sec->reloc_count == 0)
     {
-      static asection *last_opd_sec, *last_code_sec;
-      static bfd_vma last_opd_off, last_entry_vma;
-      static bfd_boolean sec_search_done;
-
-      if (last_opd_sec != opd_sec
-	  || last_opd_off != offset
-	  || (code_sec != NULL && !sec_search_done))
-	{
-	  char buf[8];
-
-	  if (!bfd_get_section_contents (opd_bfd, opd_sec, buf, offset, 8))
-	    return (bfd_vma) -1;
-
-	  last_opd_sec = opd_sec;
-	  last_opd_off = offset;
-	  last_entry_vma = bfd_get_64 (opd_bfd, buf);
-	  sec_search_done = FALSE;
-	  if (code_sec != NULL)
-	    {
-	      asection *sec;
+      char buf[8];
+
+      if (!bfd_get_section_contents (opd_bfd, opd_sec, buf, offset, 8))
+	return (bfd_vma) -1;
 
-	      sec_search_done = TRUE;
-	      last_code_sec = NULL;
-	      for (sec = opd_bfd->sections; sec != NULL; sec = sec->next)
-		if (sec->vma <= last_entry_vma
-		    && (sec->flags & SEC_LOAD) != 0
-		    && (sec->flags & SEC_ALLOC) != 0)
-		  last_code_sec = sec;
-	    }
-	}
-      if (code_sec != NULL && last_code_sec != NULL)
-	{
-	  *code_sec = last_code_sec;
-	  if (code_off != NULL)
-	    *code_off = last_entry_vma - last_code_sec->vma;
+      val = bfd_get_64 (opd_bfd, buf);
+      if (code_sec != NULL)
+	{
+	  asection *sec, *likely = NULL;
+
+	  if (in_code_sec)
+	    {
+	      sec = *code_sec;
+	      if (sec->vma <= val
+		  && val < sec->vma + sec->size)
+		likely = sec;
+	      else
+		val = -1;
+	    }
+	  else
+	    for (sec = opd_bfd->sections; sec != NULL; sec = sec->next)
+	      if (sec->vma <= val
+		  && (sec->flags & SEC_LOAD) != 0
+		  && (sec->flags & SEC_ALLOC) != 0)
+		likely = sec;
+	  if (likely != NULL)
+	    {
+	      *code_sec = likely;
+	      if (code_off != NULL)
+		*code_off = val - likely->vma;
+	    }
 	}
-      return last_entry_vma;
+      return val;
     }
 
   BFD_ASSERT (is_ppc64_elf (opd_bfd));
@@ -5640,7 +5637,12 @@ opd_entry_value (asection *opd_sec,
 	      if (code_off != NULL)
 		*code_off = val;
 	      if (code_sec != NULL)
-		*code_sec = sec;
+		{
+		  if (in_code_sec && *code_sec != sec)
+		    return -1;
+		  else
+		    *code_sec = sec;
+		}
 	      if (sec != NULL && sec->output_section != NULL)
 		val += sec->output_section->vma + sec->output_offset;
 	    }
@@ -5651,20 +5653,53 @@ opd_entry_value (asection *opd_sec,
   return val;
 }
 
-/* Return TRUE iff the ELF symbol SYM might be a function.  Set *CODE_SEC
-   and *CODE_OFF to the function's entry point.  */
-
-static bfd_boolean
-ppc64_elf_maybe_function_sym (const asymbol *sym,
-			      asection **code_sec, bfd_vma *code_off)
+/* If the ELF symbol SYM might be a function in SEC, return the
+   function size and set *CODE_OFF to the function's entry point,
+   otherwise return zero.  */
+
+static bfd_size_type
+ppc64_elf_maybe_function_sym (const asymbol *sym, asection *sec,
+			      bfd_vma *code_off)
 {
-  if (_bfd_elf_maybe_function_sym (sym, code_sec, code_off))
+  bfd_size_type size;
+
+  if ((sym->flags & (BSF_SECTION_SYM | BSF_FILE | BSF_OBJECT
+		     | BSF_THREAD_LOCAL | BSF_RELC | BSF_SRELC)) != 0)
+    return 0;
+
+  size = 0;
+  if (!(sym->flags & BSF_SYNTHETIC))
+    size = ((elf_symbol_type *) sym)->internal_elf_sym.st_size;
+
+  if (strcmp (sym->section->name, ".opd") == 0)
     {
-      if (strcmp (sym->section->name, ".opd") == 0)
-	opd_entry_value (sym->section, sym->value, code_sec, code_off);
-      return TRUE;
+      if (opd_entry_value (sym->section, sym->value,
+			   &sec, code_off, TRUE) == (bfd_vma) -1)
+	return 0;
+      /* An old ABI binary with dot-syms has a size of 24 on the .opd
+	 symbol.  This size has nothing to do with the code size of the
+	 function, which is what we're supposed to return, but the
+	 code size isn't available without looking up the dot-sym.
+	 However, doing that would be a waste of time particularly
+	 since elf_find_function will look at the dot-sym anyway.
+	 Now, elf_find_function will keep the largest size of any
+	 function sym found at the code address of interest, so return
+	 1 here to avoid it incorrectly caching a larger function size
+	 for a small function.  This does mean we return the wrong
+	 size for a new-ABI function of size 24, but all that does is
+	 disable caching for such functions.  */
+      if (size == 24)
+	size = 1;
     }
-  return FALSE;
+  else
+    {
+      if (sym->section != sec)
+	return 0;
+      *code_off = sym->value;
+    }
+  if (size == 0)
+    size = 1;
+  return size;
 }
 
 /* Return true if symbol is defined in a regular object file.  */
@@ -5744,7 +5779,7 @@ ppc64_elf_gc_keep (struct bfd_link_info 
       else if (get_opd_info (eh->elf.root.u.def.section) != NULL
 	       && opd_entry_value (eh->elf.root.u.def.section,
 				   eh->elf.root.u.def.value,
-				   &sec, NULL) != (bfd_vma) -1)
+				   &sec, NULL, FALSE) != (bfd_vma) -1)
 	sec->flags |= SEC_KEEP;
 
       sec = eh->elf.root.u.def.section;
@@ -5795,7 +5830,7 @@ ppc64_elf_gc_mark_dynamic_ref (struct el
       else if (get_opd_info (eh->elf.root.u.def.section) != NULL
 	       && opd_entry_value (eh->elf.root.u.def.section,
 				   eh->elf.root.u.def.value,
-				   &code_sec, NULL) != (bfd_vma) -1)
+				   &code_sec, NULL, FALSE) != (bfd_vma) -1)
 	code_sec->flags |= SEC_KEEP;
     }
 
@@ -5855,7 +5890,7 @@ ppc64_elf_gc_mark_hook (asection *sec,
 	      else if (get_opd_info (eh->elf.root.u.def.section) != NULL
 		       && opd_entry_value (eh->elf.root.u.def.section,
 					   eh->elf.root.u.def.value,
-					   &rsec, NULL) != (bfd_vma) -1)
+					   &rsec, NULL, FALSE) != (bfd_vma) -1)
 		eh->elf.root.u.def.section->gc_mark = 1;
 	      else
 		rsec = h->root.u.def.section;
@@ -6324,7 +6359,7 @@ func_desc_adjust (struct elf_link_hash_e
       && opd_entry_value (fdh->elf.root.u.def.section,
 			  fdh->elf.root.u.def.value,
 			  &fh->elf.root.u.def.section,
-			  &fh->elf.root.u.def.value) != (bfd_vma) -1)
+			  &fh->elf.root.u.def.value, FALSE) != (bfd_vma) -1)
     {
       fh->elf.root.type = fdh->elf.root.type;
       fh->elf.forced_local = 1;
@@ -10915,7 +10950,8 @@ toc_adjusting_stub_needed (struct bfd_li
 		  sym_value += adjust;
 		}
 
-	      dest = opd_entry_value (sym_sec, sym_value, &sym_sec, NULL);
+	      dest = opd_entry_value (sym_sec, sym_value,
+				      &sym_sec, NULL, FALSE);
 	      if (dest == (bfd_vma) -1)
 		continue;
 	    }
@@ -11490,7 +11526,7 @@ ppc64_elf_size_stubs (struct bfd_link_in
 			  sym_value += adjust;
 			}
 		      dest = opd_entry_value (sym_sec, sym_value,
-					      &code_sec, &code_value);
+					      &code_sec, &code_value, FALSE);
 		      if (dest != (bfd_vma) -1)
 			{
 			  destination = dest;
@@ -12904,7 +12940,7 @@ ppc64_elf_relocate_section (bfd *output_
 	      bfd_vma off = (relocation + addend
 			     - sec->output_section->vma
 			     - sec->output_offset);
-	      bfd_vma dest = opd_entry_value (sec, off, NULL, NULL);
+	      bfd_vma dest = opd_entry_value (sec, off, NULL, NULL, FALSE);
 	      if (dest != (bfd_vma) -1)
 		{
 		  relocation = dest;

-- 
Alan Modra
Australia Development Lab, IBM


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]