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

[Aleksandar Ristovski <aristovski at qnx dot com>]

Daniel Jacobowitz wrote:
> > 	* 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. The argument
> > 	is optional and if specified, bcache_added returns 1 if the data
> > 	was added and 0 if it was found in the hash.
>
> Whenever you find yourself writing this sort of explanation in the
> changelog, it's a sign that you should have a shorter changelog entry
> and a long comment in the source code.  Please move the explanation to
> bcache.h.

Comments added.

> > +const void *
> > +bcache (const void *addr, int length, struct bcache *bcache)
> > +{
> > +  return bcache_data (addr, length, bcache);
> > +}
> > +
> > +void *
> > +bcache_added (const void *addr, int length, struct bcache *bcache, 
> > +		   int *added)
>
> It should return a const pointer, like bcache.  Also the indentation
> on the second line is too deep.

bcache_added is more like bcache_data which returns void*. It would make sense to return void const* 
but then I would have to change const-ness in many places (too many: I would leave that for some 
other patch).

> >      case DW_TAG_namespace:
> > -      add_psymbol_to_list (actual_name, strlen (actual_name),
> > +      add_psymbol_to_global_list (actual_name, strlen (actual_name),
> >  			   VAR_DOMAIN, LOC_TYPEDEF,
> > -			   &objfile->global_psymbols,
> >  			   0, (CORE_ADDR) 0, cu->language, objfile);
>
> Please adjust indentation on the subsequent lines, since the open
> paren moved over.

Done.

> >      case DW_TAG_enumerator:
> > -j      add_psymbol_to_list (actual_name, strlen (actual_name),
> > +      if (cu->language == language_cplus
> > +	  || cu->language == language_java)
> > +	add_psymbol_to_global_list (actual_name, strlen (actual_name),
> > +				    VAR_DOMAIN, LOC_CONST,
> > +				    0, (CORE_ADDR) 0, cu->language, objfile);
>
> This part I don't understand.  Why does the behavior of enumerators
> depend on the language?  The logic for finding them during lookup is
> the same.

I must admit that I am confused a bit too. I followed the existing logic 
but now that you mentioned it, I realized I do not have a good explanation. 
I am inclined a bit to store them to the static list for C++, but for now 
I would just leave it as-is.

> In fact, if lookup_partial_symbol will never find a global symbol
> with an identical name why do we need them for any duplicated symbol,
> even legitimately duplicated things like static functions?  

I don't think static functions are a good example since they would not 
go into the global list anyway (see dwarf2read.c, case DW_TAG_subprogram).

The attached is revised patch with the changes above; additionally, it 
calls new add_partial_symbol_to_global_list for all partial symbols that 
are being added to the global list. 


Thanks,
Aleksandar Ristovski
QNX Software Systems



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.
	* dwarf2read.c (add_partial_symbol): Make call to 
	new function add_psymbol_to_global_list for partial symbols that
	are being added to the global list.
	(load_partial_dies): Likewise.
	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
	work from add_psymbol_to_list - initialises 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. Functionally, the function hasn't changed.
	(add_psymbol_to_global_list): New function, adds partial symbol to
	global list and does it only once per objfile.
	* symfile.h (add_psymbol_to_global_list): New function.
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	5 May 2008 18:49:56 -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);
-}
 
 /* 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	5 May 2008 18:49:56 -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/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.263
diff -u -p -r1.263 dwarf2read.c
--- gdb/dwarf2read.c	5 May 2008 14:37:32 -0000	1.263
+++ gdb/dwarf2read.c	5 May 2008 18:49:57 -0000
@@ -1964,11 +1964,10 @@ add_partial_symbol (struct partial_die_i
              in the global scope.  */
 	  /*prim_record_minimal_symbol (actual_name, pdi->lowpc + baseaddr,
 	     mst_text, objfile); */
-	  psym = add_psymbol_to_list (actual_name, strlen (actual_name),
-				      VAR_DOMAIN, LOC_BLOCK,
-				      &objfile->global_psymbols,
-				      0, pdi->lowpc + baseaddr,
-				      cu->language, objfile);
+	  psym = add_psymbol_to_global_list (actual_name, strlen (actual_name),
+					     VAR_DOMAIN, LOC_BLOCK,
+					     0, pdi->lowpc + baseaddr,
+					     cu->language, objfile);
 	}
       else
 	{
@@ -2000,11 +1999,11 @@ add_partial_symbol (struct partial_die_i
 	  if (pdi->locdesc)
 	    addr = decode_locdesc (pdi->locdesc, cu);
 	  if (pdi->locdesc || pdi->has_type)
-	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
-					VAR_DOMAIN, LOC_STATIC,
-					&objfile->global_psymbols,
-					0, addr + baseaddr,
-					cu->language, objfile);
+	    psym = add_psymbol_to_global_list (actual_name, 
+					       strlen (actual_name),
+					       VAR_DOMAIN, LOC_STATIC,
+					       0, addr + baseaddr,
+					       cu->language, objfile);
 	}
       else
 	{
@@ -2034,10 +2033,9 @@ add_partial_symbol (struct partial_die_i
 			   0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_namespace:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   VAR_DOMAIN, LOC_TYPEDEF,
-			   &objfile->global_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				  VAR_DOMAIN, LOC_TYPEDEF,
+				  0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_class_type:
     case DW_TAG_interface_type:
@@ -2058,23 +2056,28 @@ add_partial_symbol (struct partial_die_i
 
       /* NOTE: carlton/2003-10-07: See comment in new_symbol about
 	 static vs. global.  */
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   STRUCT_DOMAIN, LOC_TYPEDEF,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
-
+      if (cu->language == language_cplus 
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    STRUCT_DOMAIN, LOC_TYPEDEF,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     STRUCT_DOMAIN, LOC_TYPEDEF,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_enumerator:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   VAR_DOMAIN, LOC_CONST,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      if (cu->language == language_cplus 
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    VAR_DOMAIN, LOC_CONST,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else 
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     VAR_DOMAIN, LOC_CONST,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     default:
       break;
@@ -5638,13 +5641,21 @@ load_partial_dies (bfd *abfd, gdb_byte *
 	  if (part_die->name == NULL)
 	    complaint (&symfile_complaints, _("malformed enumerator DIE ignored"));
 	  else if (building_psymtab)
-	    add_psymbol_to_list (part_die->name, strlen (part_die->name),
-				 VAR_DOMAIN, LOC_CONST,
-				 (cu->language == language_cplus
-				  || cu->language == language_java)
-				 ? &cu->objfile->global_psymbols
-				 : &cu->objfile->static_psymbols,
-				 0, (CORE_ADDR) 0, cu->language, cu->objfile);
+	    {
+	      if (cu->language == language_cplus
+		  || cu->language == language_java)
+		add_psymbol_to_global_list (part_die->name, 
+					    strlen (part_die->name),
+					    VAR_DOMAIN, LOC_CONST,
+					    0, (CORE_ADDR) 0, 
+					    cu->language, cu->objfile);
+	      else
+		add_psymbol_to_list (part_die->name, strlen (part_die->name),
+				     VAR_DOMAIN, LOC_CONST,
+				     &cu->objfile->static_psymbols,
+				     0, (CORE_ADDR) 0, 
+				     cu->language, cu->objfile);
+	    }
 
 	  info_ptr = locate_pdi_sibling (part_die, info_ptr, abfd, cu);
 	  continue;
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	5 May 2008 18:49:57 -0000
@@ -3082,6 +3082,68 @@ start_psymtab_common (struct objfile *ob
   return (psymtab);
 }
 
+/* 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)
+{
+  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;
+  
+  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)
+    {
+      SYMBOL_VALUE (&psymbol) = val;
+    }
+  else
+    {
+      SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
+    }
+  SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_LANGUAGE (&psymbol) = language;
+  PSYMBOL_DOMAIN (&psymbol) = domain;
+  PSYMBOL_CLASS (&psymbol) = class;
+
+  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+
+  /* Stash the partial symbol away in the cache */
+  return bcache_added (&psymbol, sizeof (struct partial_symbol),
+		       objfile->psymbol_cache, added);
+}
+
+/* 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);
+  *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.  */
@@ -3105,43 +3167,37 @@ add_psymbol_to_list (char *name, int nam
 		     enum language language, struct objfile *objfile)
 {
   struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
-  /* 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;
+  /* Stash the partial symbol away in the cache */
+  psym = add_psymbol_to_bcache (name, namelength, domain, class,
+				val, coreaddr, language, objfile, NULL);
 
-  /* 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)
-    {
-      SYMBOL_VALUE (&psymbol) = val;
-    }
-  else
-    {
-      SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
-    }
-  SYMBOL_SECTION (&psymbol) = 0;
-  SYMBOL_LANGUAGE (&psymbol) = language;
-  PSYMBOL_DOMAIN (&psymbol) = domain;
-  PSYMBOL_CLASS (&psymbol) = class;
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (list, psym, objfile);
+  return psym;
+}
 
-  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+/* Add partial symbol to OBJFILE's global list of partial symbols. 
+   Make sure we do not duplicate this information.  See add_psymbol_to_bcache
+   for more details.  */
 
-  /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+const struct partial_symbol *
+add_psymbol_to_global_list (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)
+{
+  struct partial_symbol *psym;
+  int added;
+  
+  psym = add_psymbol_to_bcache (name, namelength, domain, class, val,
+			        coreaddr, language, objfile, &added);
 
-  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
-  if (list->next >= list->list + list->size)
-    {
-      extend_psymbol_list (list, objfile);
-    }
-  *list->next++ = psym;
-  OBJSTAT (objfile, n_psyms++);
+  if (!added)
+    return psym;
 
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (&objfile->global_psymbols, psym, objfile);
   return psym;
 }
 
Index: gdb/symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.46
diff -u -p -r1.46 symfile.h
--- gdb/symfile.h	3 Feb 2008 22:13:29 -0000	1.46
+++ gdb/symfile.h	5 May 2008 18:49:57 -0000
@@ -199,6 +199,13 @@ struct partial_symbol *add_psymbol_to_li
 					    long, CORE_ADDR,
 					    enum language, struct objfile *);
 
+extern const
+struct partial_symbol *add_psymbol_to_global_list (char *, int, domain_enum,
+						   enum address_class,
+						   long, CORE_ADDR,
+						   enum language,
+						   struct objfile *);
+
 extern void init_psymbol_list (struct objfile *, int);
 
 extern void sort_pst_symbols (struct partial_symtab *);