Re: [patch] Do not add partial_symbol again and again to the list

[Aleksandar Ristovski <aristovski@qnx.com>]

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

Daniel Jacobowitz wrote:
> 
> I think we should finish up this patch, and then proceed from there.
> 
> Should we use this optimization for all global psymtabs?  If so, then
> we ought to do it in common code so that all the other symbol readers,
> like stabs, benefit.
> 

Ok, here is simplified, but with broader consequences patch.

The patch now affects all readers that use add_psymbol_to_list by 
not allowing duplicate partial symbols in the global psymbol list 
(for a given objfile).

Tested on linux dwarf2 format, no regression. I did
not test other debug formats.


Thanks,

Aleksandar


ChangeLog

	* bcache.c (bcache_data): Call bcache_added function.
	(bcache_added): New function name. Body of function bcache_data
	is used here with the addition of 'added' argument. 
	* bcache.h (bcache_added): New function.
	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
	work from add_psymbol_to_list - initializes partial symbol and stashes
	it in objfile's cache.
	(append_psymbol_to_list): New helper function, takes other part of 
	work from add_psymbol_to_list - adds partial symbol to the given list.
	(add_psymbol_to_list): Call helper functions instead of doing work 
	here. If adding to global list, do not duplicate partial symbols in the
	partial symtab.



[-- Attachment #2: bcache_added_donotduplicatepsyms20080506.diff --]
[-- Type: text/plain, Size: 8082 bytes --]

Index: gdb/bcache.c
===================================================================
RCS file: /cvs/src/src/gdb/bcache.c,v
retrieving revision 1.20
diff -u -p -r1.20 bcache.c
--- gdb/bcache.c	1 Jan 2008 22:53:09 -0000	1.20
+++ gdb/bcache.c	6 May 2008 18:31:17 -0000
@@ -197,11 +197,40 @@ expand_hash_table (struct bcache *bcache
 static void *
 bcache_data (const void *addr, int length, struct bcache *bcache)
 {
+  return bcache_added (addr, length, bcache, NULL);
+}
+
+
+void *
+deprecated_bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+const void *
+bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+/* Find a copy of the LENGTH bytes at ADDR in BCACHE.  If BCACHE has
+   never seen those bytes before, add a copy of them to BCACHE.  In
+   either case, return a pointer to BCACHE's copy of that string.  If
+   optional ADDED is not NULL, return 1 in case of new entry or 0 if
+   returning an old entry.  */
+
+void *
+bcache_added (const void *addr, int length, struct bcache *bcache, 
+		   int *added)
+{
   unsigned long full_hash;
   unsigned short half_hash;
   int hash_index;
   struct bstring *s;
 
+  if (added)
+    *added = 0;
+
   /* If our average chain length is too high, expand the hash table.  */
   if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
     expand_hash_table (bcache);
@@ -242,21 +271,12 @@ bcache_data (const void *addr, int lengt
     bcache->unique_size += length;
     bcache->structure_size += BSTRING_SIZE (length);
 
+    if (added)
+      *added = 1;
+
     return &new->d.data;
   }
 }
-
-void *
-deprecated_bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
-
-const void *
-bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
 \f
 /* Allocating and freeing bcaches.  */
 
Index: gdb/bcache.h
===================================================================
RCS file: /cvs/src/src/gdb/bcache.h,v
retrieving revision 1.12
diff -u -p -r1.12 bcache.h
--- gdb/bcache.h	1 Jan 2008 22:53:09 -0000	1.12
+++ gdb/bcache.h	6 May 2008 18:31:18 -0000
@@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo
 extern const void *bcache (const void *addr, int length,
 			   struct bcache *bcache);
 
+extern void *bcache_added (const void *addr, int length,
+			   struct bcache *bcache, int *added);
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
 
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.203
diff -u -p -r1.203 symfile.c
--- gdb/symfile.c	5 May 2008 16:13:49 -0000	1.203
+++ gdb/symfile.c	6 May 2008 18:31:19 -0000
@@ -3082,38 +3082,33 @@ start_psymtab_common (struct objfile *ob
   return (psymtab);
 }
 \f
-/* Add a symbol with a long value to a psymtab.
-   Since one arg is a struct, we pass in a ptr and deref it (sigh).
-   Return the partial symbol that has been added.  */
-
-/* NOTE: carlton/2003-09-11: The reason why we return the partial
-   symbol is so that callers can get access to the symbol's demangled
-   name, which they don't have any cheap way to determine otherwise.
-   (Currenly, dwarf2read.c is the only file who uses that information,
-   though it's possible that other readers might in the future.)
-   Elena wasn't thrilled about that, and I don't blame her, but we
-   couldn't come up with a better way to get that information.  If
-   it's needed in other situations, we could consider breaking up
-   SYMBOL_SET_NAMES to provide access to the demangled name lookup
-   cache.  */
-
-const struct partial_symbol *
-add_psymbol_to_list (char *name, int namelength, domain_enum domain,
-		     enum address_class class,
-		     struct psymbol_allocation_list *list, long val,	/* Value as a long */
-		     CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
-		     enum language language, struct objfile *objfile)
+/* Helper function, initialises partial symbol structure and stashes 
+   it into objfile's bcache.  Note that our caching mechanism will
+   use all fields of struct partial_symbol to determine hash value of the
+   structure.  In other words, having two symbols with the same name but
+   different domain (or address) is possible and correct.  */
+
+static struct partial_symbol *
+add_psymbol_to_bcache (char *name, int namelength, domain_enum domain,
+		       enum address_class class,
+		       long val,	/* Value as a long */
+		       CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		       enum language language, struct objfile *objfile,
+		       int *added)
 {
-  struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
+  char *buf = name;  
   /* psymbol is static so that there will be no uninitialized gaps in the
      structure which might contain random data, causing cache misses in
      bcache. */
   static struct partial_symbol psymbol;
-
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -3131,17 +3126,62 @@ add_psymbol_to_list (char *name, int nam
   SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
 
   /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+  return bcache_added (&psymbol, sizeof (struct partial_symbol),
+		       objfile->psymbol_cache, added);
+}
 
-  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+/* Helper function, adds partial symbol to the given partial symbol
+   list.  */
+
+static void
+append_psymbol_to_list (struct psymbol_allocation_list *list,
+			struct partial_symbol *psym,
+			struct objfile *objfile)
+{
   if (list->next >= list->list + list->size)
-    {
-      extend_psymbol_list (list, objfile);
-    }
+    extend_psymbol_list (list, objfile);
   *list->next++ = psym;
   OBJSTAT (objfile, n_psyms++);
+}
+
+/* Add a symbol with a long value to a psymtab.
+   Since one arg is a struct, we pass in a ptr and deref it (sigh).
+   Return the partial symbol that has been added.  */
+
+/* NOTE: carlton/2003-09-11: The reason why we return the partial
+   symbol is so that callers can get access to the symbol's demangled
+   name, which they don't have any cheap way to determine otherwise.
+   (Currenly, dwarf2read.c is the only file who uses that information,
+   though it's possible that other readers might in the future.)
+   Elena wasn't thrilled about that, and I don't blame her, but we
+   couldn't come up with a better way to get that information.  If
+   it's needed in other situations, we could consider breaking up
+   SYMBOL_SET_NAMES to provide access to the demangled name lookup
+   cache.  */
 
+const struct partial_symbol *
+add_psymbol_to_list (char *name, int namelength, domain_enum domain,
+		     enum address_class class,
+		     struct psymbol_allocation_list *list, 
+		     long val,	/* Value as a long */
+		     CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		     enum language language, struct objfile *objfile)
+{
+  struct partial_symbol *psym;
+
+  int added;
+
+  /* Stash the partial symbol away in the cache */
+  psym = add_psymbol_to_bcache (name, namelength, domain, class,
+				val, coreaddr, language, objfile, &added);
+
+  /* Do not duplicate global partial symbols.  */
+  if (list == &objfile->global_psymbols
+      && !added)
+    return psym;
+
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (list, psym, objfile);
   return psym;
 }