[RFC] Use custom hash function with bcache

[RFC] Use custom hash function with bcache

[sami wagiaalla]

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

This patch enables the use of custom hash and comparison functions when 
adding elements to a bcache. The patch also introduces hash and 
comparison functions which take into consideration only the relevant 
properties of the psymbol.

Tested by running the test suit on F13 with gcc 4.4.4 on x8664, no 
regressions.

also used 'main print statistics' to ensure that the bcache object count 
and unique object count are as expected with a small test case.

Sami

[-- Attachment #2: custom_hash.patch --]
[-- Type: text/x-patch, Size: 8708 bytes --]

2010-08-13  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (psymbol_hash): New function.
	(psymbol_compare): New function.
	(add_psymbol_to_bcache): pass psymbol_hash and psymbol_compare
	to bcache_full.
	* bcache.c (hash_continue): New.
	(hash): Use hash_continue.
	(bcache): Now takes hash_function, compare_function arguments.
	(bcache_full): Ditto.
	* bcache.c (bcache): Update prototype.
	(bcache_full): Ditto.
	* macrotab.c (macro_bcache): Updated.
	* symfile.c (allocate_symtab): Updated.

diff --git a/gdb/bcache.c b/gdb/bcache.c
index 7d9180c..bef2596 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -98,12 +98,19 @@ struct bcache
 unsigned long
 hash(const void *addr, int length)
 {
+  return hash_continue (addr, length, 0);
+}
+
+/* Continue the calculation of the hash H at the given address.  */
+
+unsigned long
+hash_continue (const void *addr, int length, unsigned long h)
+{
   const unsigned char *k, *e;
-  unsigned long h;
 
   k = (const unsigned char *)addr;
   e = k+length;
-  for (h=0; k< e;++k)
+  for (; k< e;++k)
     {
       h *=16777619;
       h ^= *k;
@@ -194,21 +201,34 @@ expand_hash_table (struct bcache *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.  */
+   either case, return a pointer to BCACHE's copy of that string.
+   If HASH_FUNCTION and COMPARE_FUNCTION are not NULL they will be
+   used for hash calculation and comparing table elements respectively.
+   Otherwise the hash is calculated using the byte string and a
+   simple byte comparison is performed.  */
 const void *
-bcache (const void *addr, int length, struct bcache *bcache)
+bcache (const void *addr, int length, struct bcache *bcache,
+        unsigned long (*hash_function)(const void*),
+        int (*compare_function)(const void*, const void*))
 {
-  return bcache_full (addr, length, bcache, NULL);
+  return bcache_full (addr, length, bcache, NULL, hash_function,
+                      compare_function);
 }
 
 /* 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.  */
+   returning an old entry.
+   If HASH_FUNCTION and COMPARE_FUNCTION are not NULL they will be
+   used for hash calculation and comparing table elements respectively.
+   Otherwise the hash is calculated using the byte string and a
+   simple byte comparison is performed.  */
 
 const void *
-bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
+bcache_full (const void *addr, int length, struct bcache *bcache, int *added,
+             unsigned long (*hash_function)(const void*),
+             int (*compare_function)(const void*, const void*))
 {
   unsigned long full_hash;
   unsigned short half_hash;
@@ -235,7 +255,11 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
   bcache->total_count++;
   bcache->total_size += length;
 
-  full_hash = hash (addr, length);
+  if (hash_function == NULL)
+    full_hash = hash (addr, length);
+  else
+    full_hash = hash_function (addr);
+
   half_hash = (full_hash >> 16);
   hash_index = full_hash % bcache->num_buckets;
 
@@ -246,9 +270,18 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
     {
       if (s->half_hash == half_hash)
 	{
-	  if (s->length == length
-	      && ! memcmp (&s->d.data, addr, length))
-	    return &s->d.data;
+	  if (s->length == length)
+	    {
+	      int equal = 0;
+
+	      if (compare_function == NULL)
+		equal = (memcmp (&s->d.data, addr, length) == 0);
+	      else
+		equal = compare_function (&s->d.data, addr);
+
+	      if (equal)
+		return &s->d.data;
+	    }
 	  else
 	    bcache->half_hash_miss_count++;
 	}
diff --git a/gdb/bcache.h b/gdb/bcache.h
index da69a2d..799c744 100644
--- a/gdb/bcache.h
+++ b/gdb/bcache.h
@@ -146,13 +146,17 @@ struct bcache;
    Since the cached value is ment to be read-only, return a const
    buffer.  */
 extern const void *bcache (const void *addr, int length,
-			   struct bcache *bcache);
+			   struct bcache *bcache,
+			   unsigned long (*hash_function)(const void*),
+			   int (*compare_function)(const void*, const void*));
 
 /* Like bcache, but if ADDED is not NULL, set *ADDED to true if the
    bytes were newly added to the cache, or to false if the bytes were
    found in the cache.  */
 extern const void *bcache_full (const void *addr, int length,
-				struct bcache *bcache, int *added);
+				struct bcache *bcache, int *added,
+				unsigned long (*hash_function)(const void*),
+				int (*compare_function)(const void*, const void*));
 
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
@@ -169,5 +173,6 @@ extern int bcache_memory_used (struct bcache *bcache);
 
 /* The hash function */
 extern unsigned long hash(const void *addr, int length);
-
+extern unsigned long hash_continue (const void *addr, int length,
+                                    unsigned long h);
 #endif /* BCACHE_H */
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 93651ab..eb3eef4 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -109,7 +109,7 @@ static const void *
 macro_bcache (struct macro_table *t, const void *addr, int len)
 {
   if (t->bcache)
-    return bcache (addr, len, t->bcache);
+    return bcache (addr, len, t->bcache, NULL, NULL);
   else
     {
       void *copy = xmalloc (len);
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index bc47681..f4cdebd 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1270,6 +1270,47 @@ start_psymtab_common (struct objfile *objfile,
   return (psymtab);
 }
 
+/* Calculate a hash code for the given partial symbol.  The hash is
+   calculated using the symbol's value, language, domain, class
+   and name. These are the values which are set by
+   add_psymbol_to_bcache.  */
+
+static unsigned long
+psymbol_hash (const void *addr)
+{
+  unsigned long h = 0;
+  struct partial_symbol *psymbol = (struct partial_symbol *) addr;
+  unsigned int lang = psymbol->ginfo.language;
+  unsigned int domain = PSYMBOL_DOMAIN (psymbol);
+  unsigned int class = PSYMBOL_CLASS (psymbol);
+
+  h = hash_continue (&psymbol->ginfo.value, sizeof (psymbol->ginfo.value), 0);
+  h = hash_continue (&lang, sizeof (unsigned int), h);
+  h = hash_continue (&domain, sizeof (unsigned int), h);
+  h = hash_continue (&class, sizeof (unsigned int), h);
+  h = hash_continue (psymbol->ginfo.name, strlen (psymbol->ginfo.name), h);
+
+  return h;
+}
+
+/* Returns true if the symbol at addr1 equals the symbol at addr2.
+   For the comparison this function uses a symbols value,
+   language, domain, class and name.  */
+
+static int
+psymbol_compare (const void *addr1, const void *addr2)
+{
+  struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
+  struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
+
+  return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
+                  sizeof (sym1->ginfo.value)) == 0
+	  && sym1->ginfo.language == sym2->ginfo.language
+          && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
+          && PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2)
+          && strcmp (sym1->ginfo.name, sym2->ginfo.name) == 0);
+}
+
 /* 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
@@ -1312,7 +1353,8 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
 
   /* Stash the partial symbol away in the cache */
   return bcache_full (&psymbol, sizeof (struct partial_symbol),
-		      objfile->psymbol_cache, added);
+		      objfile->psymbol_cache, added, psymbol_hash,
+		      psymbol_compare);
 }
 
 /* Helper function, adds partial symbol to the given partial symbol
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2cb6b7f..5e7b7c2 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2729,7 +2729,7 @@ allocate_symtab (char *filename, struct objfile *objfile)
     obstack_alloc (&objfile->objfile_obstack, sizeof (struct symtab));
   memset (symtab, 0, sizeof (*symtab));
   symtab->filename = (char *) bcache (filename, strlen (filename) + 1,
-				      objfile->filename_cache);
+				      objfile->filename_cache, NULL, NULL);
   symtab->fullname = NULL;
   symtab->language = deduce_language_from_filename (filename);
   symtab->debugformat = "unknown";

Re: [RFC] Use custom hash function with bcache

[Doug Evans]

On Mon, Aug 16, 2010 at 7:10 AM, sami wagiaalla <swagiaal@redhat.com> wrote:
> This patch enables the use of custom hash and comparison functions when
> adding elements to a bcache. The patch also introduces hash and comparison
> functions which take into consideration only the relevant properties of the
> psymbol.
>
> Tested by running the test suit on F13 with gcc 4.4.4 on x8664, no
> regressions.
>
> also used 'main print statistics' to ensure that the bcache object count and
> unique object count are as expected with a small test case.
>
> Sami
>

Hi.  Comments inline with the patch.


2010-08-13  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (psymbol_hash): New function.
	(psymbol_compare): New function.
	(add_psymbol_to_bcache): pass psymbol_hash and psymbol_compare
	to bcache_full.
	* bcache.c (hash_continue): New.
	(hash): Use hash_continue.
	(bcache): Now takes hash_function, compare_function arguments.
	(bcache_full): Ditto.
	* bcache.c (bcache): Update prototype.
	(bcache_full): Ditto.
	* macrotab.c (macro_bcache): Updated.
	* symfile.c (allocate_symtab): Updated.

diff --git a/gdb/bcache.c b/gdb/bcache.c
index 7d9180c..bef2596 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -98,12 +98,19 @@ struct bcache
 unsigned long
 hash(const void *addr, int length)
 {
+  return hash_continue (addr, length, 0);
+}
+
+/* Continue the calculation of the hash H at the given address.  */
+
+unsigned long
+hash_continue (const void *addr, int length, unsigned long h)
+{
   const unsigned char *k, *e;
-  unsigned long h;

   k = (const unsigned char *)addr;
   e = k+length;
-  for (h=0; k< e;++k)
+  for (; k< e;++k)
     {
       h *=16777619;
       h ^= *k;
@@ -194,21 +201,34 @@ expand_hash_table (struct bcache *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.  */
+   either case, return a pointer to BCACHE's copy of that string.
+   If HASH_FUNCTION and COMPARE_FUNCTION are not NULL they will be
+   used for hash calculation and comparing table elements respectively.
+   Otherwise the hash is calculated using the byte string and a
+   simple byte comparison is performed.  */

blank line between a function comment and definition

 const void *
-bcache (const void *addr, int length, struct bcache *bcache)
+bcache (const void *addr, int length, struct bcache *bcache,
+        unsigned long (*hash_function)(const void*),
+        int (*compare_function)(const void*, const void*))
 {
-  return bcache_full (addr, length, bcache, NULL);
+  return bcache_full (addr, length, bcache, NULL, hash_function,
+                      compare_function);
 }

I think it would be better to attach the hash and compare functions to
the bcache object.
E.g. add hash_function, compare_function parameters to bcache_xmalloc.

 /* 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.  */
+   returning an old entry.
+   If HASH_FUNCTION and COMPARE_FUNCTION are not NULL they will be
+   used for hash calculation and comparing table elements respectively.
+   Otherwise the hash is calculated using the byte string and a
+   simple byte comparison is performed.  */

 const void *
-bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
+bcache_full (const void *addr, int length, struct bcache *bcache, int *added,
+             unsigned long (*hash_function)(const void*),
+             int (*compare_function)(const void*, const void*))
 {
   unsigned long full_hash;
   unsigned short half_hash;
@@ -235,7 +255,11 @@ bcache_full (const void *addr, int length, struct
bcache *bcache, int *added)
   bcache->total_count++;
   bcache->total_size += length;

-  full_hash = hash (addr, length);
+  if (hash_function == NULL)
+    full_hash = hash (addr, length);
+  else
+    full_hash = hash_function (addr);

[for reference sake]
Once hash_function is in struct bcache I'd store a pointer to the
default hasher, instead of having NULL mean "use default hasher".
[see below for compare_function - I wrote that part first]

+
   half_hash = (full_hash >> 16);
   hash_index = full_hash % bcache->num_buckets;

@@ -246,9 +270,18 @@ bcache_full (const void *addr, int length, struct
bcache *bcache, int *added)
     {
       if (s->half_hash == half_hash)
 	{
-	  if (s->length == length
-	      && ! memcmp (&s->d.data, addr, length))
-	    return &s->d.data;
+	  if (s->length == length)
+	    {
+	      int equal = 0;
+
+	      if (compare_function == NULL)
+		equal = (memcmp (&s->d.data, addr, length) == 0);
+	      else
+		equal = compare_function (&s->d.data, addr);

[for reference sake]
Once compare_function lives in struct bcache, I have a slight
preference for not having compare_function == NULL mean "use memcmp".
Instead store a pointer to memcmp (or wrapper) in the field (and then
you don't have to test for NULL above).  It would mean that all
compare_functions would get a length parameter that they may not use
(except perhaps in an assert that the value is the expected size), but
I think that's ok.
A NULL value for compare_function passed to bcache_xmalloc could mean
"use default" though.

+
+	      if (equal)
+		return &s->d.data;
+	    }
 	  else
 	    bcache->half_hash_miss_count++;
 	}
diff --git a/gdb/bcache.h b/gdb/bcache.h
index da69a2d..799c744 100644
--- a/gdb/bcache.h
+++ b/gdb/bcache.h
@@ -146,13 +146,17 @@ struct bcache;
    Since the cached value is ment to be read-only, return a const
    buffer.  */
 extern const void *bcache (const void *addr, int length,
-			   struct bcache *bcache);
+			   struct bcache *bcache,
+			   unsigned long (*hash_function)(const void*),
+			   int (*compare_function)(const void*, const void*));

 /* Like bcache, but if ADDED is not NULL, set *ADDED to true if the
    bytes were newly added to the cache, or to false if the bytes were
    found in the cache.  */
 extern const void *bcache_full (const void *addr, int length,
-				struct bcache *bcache, int *added);
+				struct bcache *bcache, int *added,
+				unsigned long (*hash_function)(const void*),
+				int (*compare_function)(const void*, const void*));

 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
@@ -169,5 +173,6 @@ extern int bcache_memory_used (struct bcache *bcache);

 /* The hash function */
 extern unsigned long hash(const void *addr, int length);
-
+extern unsigned long hash_continue (const void *addr, int length,
+                                    unsigned long h);

blank line

 #endif /* BCACHE_H */
diff --git a/gdb/macrotab.c b/gdb/macrotab.c
index 93651ab..eb3eef4 100644
--- a/gdb/macrotab.c
+++ b/gdb/macrotab.c
@@ -109,7 +109,7 @@ static const void *
 macro_bcache (struct macro_table *t, const void *addr, int len)
 {
   if (t->bcache)
-    return bcache (addr, len, t->bcache);
+    return bcache (addr, len, t->bcache, NULL, NULL);
   else
     {
       void *copy = xmalloc (len);
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index bc47681..f4cdebd 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1270,6 +1270,47 @@ start_psymtab_common (struct objfile *objfile,
   return (psymtab);
 }

+/* Calculate a hash code for the given partial symbol.  The hash is
+   calculated using the symbol's value, language, domain, class
+   and name. These are the values which are set by
+   add_psymbol_to_bcache.  */
+
+static unsigned long
+psymbol_hash (const void *addr)
+{
+  unsigned long h = 0;
+  struct partial_symbol *psymbol = (struct partial_symbol *) addr;
+  unsigned int lang = psymbol->ginfo.language;
+  unsigned int domain = PSYMBOL_DOMAIN (psymbol);
+  unsigned int class = PSYMBOL_CLASS (psymbol);
+
+  h = hash_continue (&psymbol->ginfo.value, sizeof (psymbol->ginfo.value), 0);

s/0/h/

+  h = hash_continue (&lang, sizeof (unsigned int), h);
+  h = hash_continue (&domain, sizeof (unsigned int), h);
+  h = hash_continue (&class, sizeof (unsigned int), h);
+  h = hash_continue (psymbol->ginfo.name, strlen (psymbol->ginfo.name), h);
+
+  return h;
+}
+
+/* Returns true if the symbol at addr1 equals the symbol at addr2.
+   For the comparison this function uses a symbols value,
+   language, domain, class and name.  */
+
+static int
+psymbol_compare (const void *addr1, const void *addr2)
+{
+  struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
+  struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
+
+  return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
+                  sizeof (sym1->ginfo.value)) == 0
+	  && sym1->ginfo.language == sym2->ginfo.language
+          && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
+          && PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2)
+          && strcmp (sym1->ginfo.name, sym2->ginfo.name) == 0);
+}
+
 /* 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
@@ -1312,7 +1353,8 @@ add_psymbol_to_bcache (char *name, int
namelength, int copy_name,

   /* Stash the partial symbol away in the cache */
   return bcache_full (&psymbol, sizeof (struct partial_symbol),
-		      objfile->psymbol_cache, added);
+		      objfile->psymbol_cache, added, psymbol_hash,
+		      psymbol_compare);
 }

 /* Helper function, adds partial symbol to the given partial symbol
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2cb6b7f..5e7b7c2 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2729,7 +2729,7 @@ allocate_symtab (char *filename, struct objfile *objfile)
     obstack_alloc (&objfile->objfile_obstack, sizeof (struct symtab));
   memset (symtab, 0, sizeof (*symtab));
   symtab->filename = (char *) bcache (filename, strlen (filename) + 1,
-				      objfile->filename_cache);
+				      objfile->filename_cache, NULL, NULL);
   symtab->fullname = NULL;
   symtab->language = deduce_language_from_filename (filename);
   symtab->debugformat = "unknown";

Re: [RFC] Use custom hash function with bcache

[Doug Evans]

On Mon, Aug 16, 2010 at 11:28 AM, Doug Evans <dje@google.com> wrote:
> On Mon, Aug 16, 2010 at 7:10 AM, sami wagiaalla <swagiaal@redhat.com> wrote:
>> This patch enables the use of custom hash and comparison functions when
>> adding elements to a bcache. The patch also introduces hash and comparison
>> functions which take into consideration only the relevant properties of the
>> psymbol.
>>
>> Tested by running the test suit on F13 with gcc 4.4.4 on x8664, no
>> regressions.
>>
>> also used 'main print statistics' to ensure that the bcache object count and
>> unique object count are as expected with a small test case.
>>
>> Sami
>>
>
> Hi.  Comments inline with the patch.
>
>
> 2010-08-13  Sami Wagiaalla  <swagiaal@redhat.com>
>
>        * psymtab.c (psymbol_hash): New function.
>        (psymbol_compare): New function.
>        (add_psymbol_to_bcache): pass psymbol_hash and psymbol_compare
>        to bcache_full.
>        * bcache.c (hash_continue): New.
>        (hash): Use hash_continue.
>        (bcache): Now takes hash_function, compare_function arguments.
>        (bcache_full): Ditto.
>        * bcache.c (bcache): Update prototype.
>        (bcache_full): Ditto.
>        * macrotab.c (macro_bcache): Updated.
>        * symfile.c (allocate_symtab): Updated.

Blech, I forgot to add that we can get rid of the static in "static
struct partial_symbol psymbol;" in add_psymbol_to_bcache.
I think that should be part of this patch.

A couple more things come to mind.
1) We store symbol names such that we can compare them with ptr1 ==
ptr2, but your patch uses strcmp.
2) I'm wondering if there's some abstraction violation with bcache not
being aware of the extra memory that is used by
gsymbol->language_specific.cplus_specific.  Dunno, just wondering.

Re: [RFC] Use custom hash function with bcache

[Daniel Jacobowitz]

On Mon, Aug 16, 2010 at 10:10:41AM -0400, sami wagiaalla wrote:
> This patch enables the use of custom hash and comparison functions
> when adding elements to a bcache. The patch also introduces hash and
> comparison functions which take into consideration only the relevant
> properties of the psymbol.

This patch makes me nervous because it's violating the invariants of
the bcache.  Any time you get a psymbol from the bcache, it is going
to have other fields that were not hashed somewhat random.  What are
those ignored fields, and why do they not matter?  How about
lifetimes, are they as long-lived as the bcache?

You compare name, value, language, domain, and class.  The mangled
name is ignored; this assumes that there is only ever one mangled name
per demangled name, with no documentation of why that's safe to
assume.  Section is ignored; if there are two definitions of a symbol
with the same name in different sections but the same value otherwise,
we'll never find out.  That could break overlays.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Use custom hash function with bcache

[sami wagiaalla]

On 08/16/2010 03:13 PM, Daniel Jacobowitz wrote:
> On Mon, Aug 16, 2010 at 10:10:41AM -0400, sami wagiaalla wrote:
>> This patch enables the use of custom hash and comparison functions
>> when adding elements to a bcache. The patch also introduces hash and
>> comparison functions which take into consideration only the relevant
>> properties of the psymbol.
>
> This patch makes me nervous because it's violating the invariants of
> the bcache.  Any time you get a psymbol from the bcache, it is going
> to have other fields that were not hashed somewhat random.  What are
> those ignored fields, and why do they not matter?  How about
> lifetimes, are they as long-lived as the bcache?
>

I only took into consideration the values which are set by 
add_psymbol_to_bcache. The assumption is that these are the only values 
that will make a difference since they are the only values available 
when calculating the hash.

> You compare name, value, language, domain, and class.  The mangled
> name is ignored; this assumes that there is only ever one mangled name
> per demangled name, with no documentation of why that's safe to
> assume.

Hmm, the demangled name can be added if this is not a correct 
assumption. I just wanted to avoid the redundant check, and 
language_specific part of the symbol.

   Section is ignored; if there are two definitions of a symbol
> with the same name in different sections but the same value otherwise,
> we'll never find out.  That could break overlays.
>

Section is set to 0 by add_psymbol_to_bcache

Re: [RFC] Use custom hash function with bcache

[sami wagiaalla]

> 1) We store symbol names such that we can compare them with ptr1 ==
> ptr2, but your patch uses strcmp.

that explains how the old hash function worked :)

> 2) I'm wondering if there's some abstraction violation with bcache not
> being aware of the extra memory that is used by
> gsymbol->language_specific.cplus_specific.  Dunno, just wondering.

I think it is okay as long as the part of psymbol taken into 
consideration ensures that duplicate symbols are caught (Daniel had some 
comments here). Also, this is only for psymbols. Other clients still use 
the old hash function

Re: [RFC] Use custom hash function with bcache

[Daniel Jacobowitz]

On Mon, Aug 16, 2010 at 03:50:17PM -0400, sami wagiaalla wrote:
> I only took into consideration the values which are set by
> add_psymbol_to_bcache. The assumption is that these are the only
> values that will make a difference since they are the only values
> available when calculating the hash.

Interesting, I didn't remember about add_psymbol_to_bcache.  I'd
better back up - what are you accomplishing by this change?

Hashing the whole structure is probably faster than hashing just
specific fields, so that's probably not it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Use custom hash function with bcache

[sami wagiaalla]

On 08/16/2010 04:03 PM, Daniel Jacobowitz wrote:
> On Mon, Aug 16, 2010 at 03:50:17PM -0400, sami wagiaalla wrote:
>> I only took into consideration the values which are set by
>> add_psymbol_to_bcache. The assumption is that these are the only
>> values that will make a difference since they are the only values
>> available when calculating the hash.
>
> Interesting, I didn't remember about add_psymbol_to_bcache.  I'd
> better back up - what are you accomplishing by this change?
>

A previous patch of mine introduced a bcache regression :D. The patch 
made cplus_specifc a pointer to an allocated struct. This is because we 
wanted to store more information in cplus_specific without penalizing 
the other other languages. With cplus_specific being a pointer hashing 
the whole symbol didn't work anymore. This patch is an attempt to fix that.

Re: [RFC] Use custom hash function with bcache

[Daniel Jacobowitz]

On Mon, Aug 16, 2010 at 04:11:01PM -0400, sami wagiaalla wrote:
> On 08/16/2010 04:03 PM, Daniel Jacobowitz wrote:
> >On Mon, Aug 16, 2010 at 03:50:17PM -0400, sami wagiaalla wrote:
> >>I only took into consideration the values which are set by
> >>add_psymbol_to_bcache. The assumption is that these are the only
> >>values that will make a difference since they are the only values
> >>available when calculating the hash.
> >
> >Interesting, I didn't remember about add_psymbol_to_bcache.  I'd
> >better back up - what are you accomplishing by this change?
> >
> 
> A previous patch of mine introduced a bcache regression :D. The patch
> made cplus_specifc a pointer to an allocated struct. This is because
> we wanted to store more information in cplus_specific without
> penalizing the other other languages. With cplus_specific being a
> pointer hashing the whole symbol didn't work anymore. This patch is
> an attempt to fix that.

Aha!  OK, I get it now.  And the section is probably never used for
psymbols, right?

I think that the way you've got this is probably OK, but could stand
some more explanation.  It kind of pains me, though, because there's
an obvious missed optimization that I don't see how to do efficiently.
We don't need to do the work of SYMBOL_SET_NAMES again if there's already a
named version in the cache...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Use custom hash function with bcache

[sami wagiaalla]

> Aha!  OK, I get it now.  And the section is probably never used for
> psymbols, right?
>

It might be if the symbol is a C++ symbol and has a demangled name.

> I think that the way you've got this is probably OK, but could stand
> some more explanation.

I will update the comments to reflect the result our discussions here.

   It kind of pains me, though, because there's
> an obvious missed optimization that I don't see how to do efficiently.
> We don't need to do the work of SYMBOL_SET_NAMES again if there's already a
> named version in the cache...
>

Hmm... this is probably a separate patch, but how about this: 
symbol_set_names sets the mangled and demangled names we need at least 
the mangled name to find out wether there is already a name version in 
the bcache. So maybe we could only set the mangled name, check the 
bcache and only set the demangled name after we have consulted the 
bcache. This only works of course if we ignore the demangled name as is 
in this patch (Do you agree that this is OK btw ?).

Re: [RFC] Use custom hash function with bcache

[Daniel Jacobowitz]

On Tue, Aug 17, 2010 at 01:01:46PM -0400, sami wagiaalla wrote:
> Hmm... this is probably a separate patch, but how about this:
> symbol_set_names sets the mangled and demangled names we need at
> least the mangled name to find out wether there is already a name
> version in the bcache. So maybe we could only set the mangled name,
> check the bcache and only set the demangled name after we have
> consulted the bcache. This only works of course if we ignore the
> demangled name as is in this patch (Do you agree that this is OK btw
> ?).

I think it'll be OK.

As for SYMBOL_SET_NAMES, I worry that will lose the optimization we
got by doing them both at once; I don't remember how it works (it's
been five years), but it made a big difference to startup time.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] Use custom hash function with bcache

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> This patch enables the use of custom hash and comparison functions
Sami> when adding elements to a bcache. The patch also introduces hash and
Sami> comparison functions which take into consideration only the relevant
Sami> properties of the psymbol.

I don't understand how this approach can work as-is.

The bcache interns objects.  It doesn't know anything about their
semantics, just their size.

This patch means that a given bcache could include two objects of the
same size that have different hash and comparison functions.  So, it
seems like some kind of clash could (in theory) result.  This is very
bad if one object is mutable and the other is not.

I think it would be safer to either:

1. Make the hash and comparison functions part of the bcache itself, set
   only when the bcache is allocated, then audit all uses of this
   particular bcache to make sure that only psymtabs are added to it
   (this could be done by introducing a new type, so that invalid uses
   are compile-time errors),
   or

2. Do psymtab interning in an htab_t.  I suspect this will have worse
   overhead, but I really don't know.


I think you can make the psym hash and comparison functions a bit
simpler by just skipping the pointer-sized lang-specific hole.  This is
maybe more future-proof as well.

Tom

Re: [RFC] Use custom hash function with bcache

[sami wagiaalla]

> This patch means that a given bcache could include two objects of the
> same size that have different hash and comparison functions.  So, it
> seems like some kind of clash could (in theory) result.  This is very
> bad if one object is mutable and the other is not.
>
> I think it would be safer to either:

I will take door number 1 since I am already working on it :)

> 1. Make the hash and comparison functions part of the bcache itself, set
>     only when the bcache is allocated,

This is what Doug suggested. Making the functions part of the bcache 
struct and assigning them when bcache_xmalloc is called.

  then audit all uses of this
>     particular bcache to make sure that only psymtabs are added to it
>     (this could be done by introducing a new type, so that invalid uses
>     are compile-time errors),

with the above change the bcaches will be initialized like this:

objfile->psymbol_cache = bcache_xmalloc (psymbol_hash, psymbol_compare);
objfile->macro_cache = bcache_xmalloc (NULL, NULL);
objfile->filename_cache = bcache_xmalloc (NULL, NULL);

So only objects added to the psymbol_cache would be subjected to the new 
hash and compare functions. Although, you are right, there is no strict 
type checking. Are you suggesting we make psymbol_cache a new type ?

>
> I think you can make the psym hash and comparison functions a bit
> simpler by just skipping the pointer-sized lang-specific hole.  This is
> maybe more future-proof as well.
>

This patch already does that :)

Re: [RFC] Use custom hash function with bcache

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> Although, you are right, there is no strict type checking. Are you
Sami> suggesting we make psymbol_cache a new type ?

Yeah; at least, if it isn't too big.

Tom

[patch 1/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[sami wagiaalla]

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

New patch with the recommended changes attached.

[-- Attachment #2: custom_hash_1.patch --]
[-- Type: text/x-patch, Size: 10083 bytes --]

Enable custom bcache hash function.

2010-08-18  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (add_psymbol_to_bcache): Remove 'static' from
	'static partial_symbol psymbol'.
	(psymbol_hash): New function.
	(psymbol_compare): New function.
	* bcache.c (hash_continue): New.
	(hash): Use hash_continue.
	* bcache.c: Add hash_function and compare_function
	pointers to bcache struct.
	(bcache_full): Use bcache->hash_function, and
	bcache->compare_function.
	(bcache_compare): New function.
	(bcache_xmalloc): Take hash_function and
	compare_function arguments and initialize the
	bcach's pointers.
	Updated comment.
	* objfiles.c (allocate_objfile): Updated.
	* symfile.c (reread_symbols): Updated.

diff --git a/gdb/bcache.c b/gdb/bcache.c
index 7d9180c..ea408ab 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -89,6 +89,12 @@ struct bcache
      16 bits of hash values) hit, but the corresponding combined
      length/data compare missed.  */
   unsigned long half_hash_miss_count;
+
+  /* Hash function to be used for this bcache object.  */
+  unsigned long (*hash_function)(const void *addr, int length);
+
+  /* Compare function to be used for this bcache object.  */
+  int (*compare_function)(const void*, const void*, int length);
 };
 
 /* The old hash function was stolen from SDBM. This is what DB 3.0 uses now,
@@ -98,12 +104,19 @@ struct bcache
 unsigned long
 hash(const void *addr, int length)
 {
+  return hash_continue (addr, length, 0);
+}
+
+/* Continue the calculation of the hash H at the given address.  */
+
+unsigned long
+hash_continue (const void *addr, int length, unsigned long h)
+{
   const unsigned char *k, *e;
-  unsigned long h;
 
   k = (const unsigned char *)addr;
   e = k+length;
-  for (h=0; k< e;++k)
+  for (; k< e;++k)
     {
       h *=16777619;
       h ^= *k;
@@ -235,7 +248,8 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
   bcache->total_count++;
   bcache->total_size += length;
 
-  full_hash = hash (addr, length);
+  full_hash = bcache->hash_function (addr, length);
+
   half_hash = (full_hash >> 16);
   hash_index = full_hash % bcache->num_buckets;
 
@@ -247,7 +261,7 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
       if (s->half_hash == half_hash)
 	{
 	  if (s->length == length
-	      && ! memcmp (&s->d.data, addr, length))
+	      && bcache->compare_function (&s->d.data, addr, length))
 	    return &s->d.data;
 	  else
 	    bcache->half_hash_miss_count++;
@@ -276,14 +290,42 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
   }
 }
 \f
+
+/* Compare the byte string at ADDR1 of lenght LENGHT to the
+   string at ADDR2.  Return 1 if they are equal.  */
+
+static int
+bcache_compare (const void* addr1, const void* addr2, int length)
+{
+  return memcmp (addr1, addr2, length) == 0;
+}
+
+/* Forwarding function for memcmp.  */
+//bcache_memcmp ()
+
 /* Allocating and freeing bcaches.  */
 
+/* Allocated a bcache.  HASH_FUNCTION and COMPARE_FUNCTION can be used
+   to pass in custom hash, and compare functions to be used by this
+   bcache. If HASH_FUNCTION is NULL hash() is used and if COMPARE_FUNCTION
+   is NULL memcmp() is used.  */
+
 struct bcache *
-bcache_xmalloc (void)
+bcache_xmalloc (unsigned long (*hash_function)(const void*, int length),
+                int (*compare_function)(const void*, const void*, int length))
 {
   /* Allocate the bcache pre-zeroed.  */
   struct bcache *b = XCALLOC (1, struct bcache);
 
+  if (hash_function)
+    b->hash_function = hash_function;
+  else
+    b->hash_function = hash;
+
+  if (compare_function)
+    b->compare_function = compare_function;
+  else
+    b->compare_function = bcache_compare;
   return b;
 }
 
diff --git a/gdb/bcache.h b/gdb/bcache.h
index da69a2d..97fd417 100644
--- a/gdb/bcache.h
+++ b/gdb/bcache.h
@@ -158,7 +158,9 @@ extern const void *bcache_full (const void *addr, int length,
 extern void bcache_xfree (struct bcache *bcache);
 
 /* Create a new bcache object.  */
-extern struct bcache *bcache_xmalloc (void);
+extern struct bcache *bcache_xmalloc (
+    unsigned long (*hash_function)(const void*, int length),
+    int (*compare_function)(const void*, const void*, int length));
 
 /* Print statistics on BCACHE's memory usage and efficacity at
    eliminating duplication.  TYPE should be a string describing the
@@ -167,7 +169,9 @@ extern struct bcache *bcache_xmalloc (void);
 extern void print_bcache_statistics (struct bcache *bcache, char *type);
 extern int bcache_memory_used (struct bcache *bcache);
 
-/* The hash function */
+/* The hash functions */
 extern unsigned long hash(const void *addr, int length);
+extern unsigned long hash_continue (const void *addr, int length,
+                                    unsigned long h);
 
 #endif /* BCACHE_H */
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index b522189..c479d22 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -199,9 +199,9 @@ allocate_objfile (bfd *abfd, int flags)
   struct objfile *objfile;
 
   objfile = (struct objfile *) xzalloc (sizeof (struct objfile));
-  objfile->psymbol_cache = bcache_xmalloc ();
-  objfile->macro_cache = bcache_xmalloc ();
-  objfile->filename_cache = bcache_xmalloc ();
+  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash, psymbol_compare);
+  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
+  objfile->filename_cache = bcache_xmalloc (NULL, NULL);
   /* We could use obstack_specify_allocation here instead, but
      gdb_obstack.h specifies the alloc/dealloc functions.  */
   obstack_init (&objfile->objfile_obstack);
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index bc47681..0d57b5e 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1270,6 +1270,47 @@ start_psymtab_common (struct objfile *objfile,
   return (psymtab);
 }
 
+/* Calculate a hash code for the given partial symbol.  The hash is
+   calculated using the symbol's value, language, domain, class
+   and name. These are the values which are set by
+   add_psymbol_to_bcache.  */
+
+unsigned long
+psymbol_hash (const void *addr, int length)
+{
+  unsigned long h = 0;
+  struct partial_symbol *psymbol = (struct partial_symbol *) addr;
+  unsigned int lang = psymbol->ginfo.language;
+  unsigned int domain = PSYMBOL_DOMAIN (psymbol);
+  unsigned int class = PSYMBOL_CLASS (psymbol);
+
+  h = hash_continue (&psymbol->ginfo.value, sizeof (psymbol->ginfo.value), h);
+  h = hash_continue (&lang, sizeof (unsigned int), h);
+  h = hash_continue (&domain, sizeof (unsigned int), h);
+  h = hash_continue (&class, sizeof (unsigned int), h);
+  h = hash_continue (psymbol->ginfo.name, strlen (psymbol->ginfo.name), h);
+
+  return h;
+}
+
+/* Returns true if the symbol at addr1 equals the symbol at addr2.
+   For the comparison this function uses a symbols value,
+   language, domain, class and name.  */
+
+int
+psymbol_compare (const void *addr1, const void *addr2, int length)
+{
+  struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
+  struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
+
+  return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
+                  sizeof (sym1->ginfo.value)) == 0
+	  && sym1->ginfo.language == sym2->ginfo.language
+          && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
+          && PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2)
+          && sym1->ginfo.name == sym2->ginfo.name);
+}
+
 /* 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
@@ -1288,7 +1329,7 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_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;
+  struct partial_symbol psymbol;
 
   /* However, we must ensure that the entire 'value' field has been
      zeroed before assigning to it, because an assignment may not
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index de8b67e..0786944 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -20,6 +20,9 @@
 #ifndef PSYMTAB_H
 #define PSYMTAB_H
 
+extern unsigned long psymbol_hash (const void *addr, int length);
+extern int psymbol_compare (const void *addr1, const void *addr2, int length);
+
 void map_partial_symbol_names (void (*) (const char *, void *), void *);
 
 void map_partial_symbol_filenames (void (*) (const char *, const char *,
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2cb6b7f..048b8a8 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2432,11 +2432,12 @@ reread_symbols (void)
 
 	  /* Free the obstacks for non-reusable objfiles */
 	  bcache_xfree (objfile->psymbol_cache);
-	  objfile->psymbol_cache = bcache_xmalloc ();
+	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
+	                                           psymbol_compare);
 	  bcache_xfree (objfile->macro_cache);
-	  objfile->macro_cache = bcache_xmalloc ();
+	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
 	  bcache_xfree (objfile->filename_cache);
-	  objfile->filename_cache = bcache_xmalloc ();
+	  objfile->filename_cache = bcache_xmalloc (NULL,NULL);
 	  if (objfile->demangled_names_hash != NULL)
 	    {
 	      htab_delete (objfile->demangled_names_hash);
@@ -2458,9 +2459,10 @@ reread_symbols (void)
 	  memset (&objfile->msymbol_demangled_hash, 0,
 		  sizeof (objfile->msymbol_demangled_hash));
 
-	  objfile->psymbol_cache = bcache_xmalloc ();
-	  objfile->macro_cache = bcache_xmalloc ();
-	  objfile->filename_cache = bcache_xmalloc ();
+	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
+	                                           psymbol_compare);
+	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
+	  objfile->filename_cache = bcache_xmalloc (NULL, NULL);
 	  /* obstack_init also initializes the obstack so it is
 	     empty.  We could use obstack_specify_allocation but
 	     gdb_obstack.h specifies the alloc/dealloc

Re: [RFC] Use custom hash function with bcache

[sami wagiaalla]

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

On 08/18/2010 11:24 AM, Tom Tromey wrote:
>>>>>> "Sami" == sami wagiaalla<swagiaal@redhat.com>  writes:
>
> Sami>  Although, you are right, there is no strict type checking. Are you
> Sami>  suggesting we make psymbol_cache a new type ?
>
> Yeah; at least, if it isn't too big.
>

Patch attached.


[-- Attachment #2: custom_hash_2.patch --]
[-- Type: text/x-patch, Size: 7957 bytes --]

Create and use a specialized bcache type for psymbols

2010-08-19  Sami Wagiaalla  <swagiaal@redhat.com>

	* symfile.c (reread_symbols): Use psymbol_bcache_free, and
	psymbol_bcache_init.
	* psymtab.h (psymbol_bcache_init): New function prototype.
	(psymbol_bcache_free): New function prototype.
	* psymtab.c (psymbol_bcache_init): New function.
	(psymbol_bcache_free): New function.
	(psymbol_bcache_full): New function.
	(add_psymbol_to_bcache): use psymbol_bcache_full.
	* objfiles.h (psymbol_cache): Change type of psymbol_cache to
	psymbol_bcache.
	* symmisc.c (print_symbol_bcache_statistics): Updated.
	(print_objfile_statistics): Updated.
	* objfiles.c (allocate_objfile): Use psymbol_bcache_init to initialize
	psymbol_cache.
	(free_objfile): Use psymbol_bcache_free.

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index c479d22..6fc8a1f 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -199,7 +199,7 @@ allocate_objfile (bfd *abfd, int flags)
   struct objfile *objfile;
 
   objfile = (struct objfile *) xzalloc (sizeof (struct objfile));
-  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash, psymbol_compare);
+  objfile->psymbol_cache = psymbol_bcache_init ();
   objfile->macro_cache = bcache_xmalloc (NULL, NULL);
   objfile->filename_cache = bcache_xmalloc (NULL, NULL);
   /* We could use obstack_specify_allocation here instead, but
@@ -658,7 +658,7 @@ free_objfile (struct objfile *objfile)
   if (objfile->static_psymbols.list)
     xfree (objfile->static_psymbols.list);
   /* Free the obstacks for non-reusable objfiles */
-  bcache_xfree (objfile->psymbol_cache);
+  psymbol_bcache_free (objfile->psymbol_cache);
   bcache_xfree (objfile->macro_cache);
   bcache_xfree (objfile->filename_cache);
   if (objfile->demangled_names_hash)
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 62fc1cb..ec4870b 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -249,7 +249,7 @@ struct objfile
     /* A byte cache where we can stash arbitrary "chunks" of bytes that
        will not change. */
 
-    struct bcache *psymbol_cache;	/* Byte cache for partial syms */
+    struct psymbol_bcache *psymbol_cache; /* Byte cache for partial syms */
     struct bcache *macro_cache;          /* Byte cache for macros */
     struct bcache *filename_cache;	 /* Byte cache for file names.  */
 
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index 0d57b5e..ac013bb 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1275,7 +1275,7 @@ start_psymtab_common (struct objfile *objfile,
    and name. These are the values which are set by
    add_psymbol_to_bcache.  */
 
-unsigned long
+static unsigned long
 psymbol_hash (const void *addr, int length)
 {
   unsigned long h = 0;
@@ -1297,7 +1297,7 @@ psymbol_hash (const void *addr, int length)
    For the comparison this function uses a symbols value,
    language, domain, class and name.  */
 
-int
+static int
 psymbol_compare (const void *addr1, const void *addr2, int length)
 {
   struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
@@ -1311,6 +1311,44 @@ psymbol_compare (const void *addr1, const void *addr2, int length)
           && sym1->ginfo.name == sym2->ginfo.name);
 }
 
+/* Initialize a partial symbol bcache.  */
+
+struct psymbol_bcache *
+psymbol_bcache_init ()
+{
+    struct psymbol_bcache *bcache = XCALLOC (1, struct psymbol_bcache);
+    bcache->bcache = bcache_xmalloc (psymbol_hash, psymbol_compare);
+    return bcache;
+}
+
+/* Free a partial symbol bcache.  */
+void
+psymbol_bcache_free (struct psymbol_bcache *bcache)
+{
+
+  if (bcache == NULL)
+    return;
+
+  bcache_xfree(bcache->bcache);
+  xfree (bcache);
+}
+
+/* Find a copy of the SYM in BCACHE.  If BCACHE has never seen this
+   symbol before, add a copy to BCACHE.  In either case, return a pointer
+   to BCACHE's copy of the symbol.  If optional ADDED is not NULL, return
+   1 in case of new entry or 0 if returning an old entry.  */
+
+static const struct partial_symbol *
+psymbol_bcache_full (struct partial_symbol *sym,
+             struct psymbol_bcache *bcache,
+             int *added)
+{
+    return bcache_full (sym,
+                        sizeof (struct partial_symbol),
+                        bcache->bcache,
+                        added);
+}
+
 /* 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
@@ -1352,8 +1390,9 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
   SYMBOL_SET_NAMES (&psymbol, name, namelength, copy_name, objfile);
 
   /* Stash the partial symbol away in the cache */
-  return bcache_full (&psymbol, sizeof (struct partial_symbol),
-		      objfile->psymbol_cache, added);
+  return psymbol_bcache_full (&psymbol,
+                              objfile->psymbol_cache,
+                              added);
 }
 
 /* Helper function, adds partial symbol to the given partial symbol
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index 0786944..f3ea677 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -20,8 +20,14 @@
 #ifndef PSYMTAB_H
 #define PSYMTAB_H
 
-extern unsigned long psymbol_hash (const void *addr, int length);
-extern int psymbol_compare (const void *addr1, const void *addr2, int length);
+/* A bcache for partial symbols.  */
+
+struct psymbol_bcache {
+    struct bcache *bcache;
+};
+
+extern struct psymbol_bcache *psymbol_bcache_init ();
+extern void psymbol_bcache_free (struct psymbol_bcache *);
 
 void map_partial_symbol_names (void (*) (const char *, void *), void *);
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 048b8a8..087cd3a 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2431,9 +2431,8 @@ reread_symbols (void)
 		  sizeof (objfile->static_psymbols));
 
 	  /* Free the obstacks for non-reusable objfiles */
-	  bcache_xfree (objfile->psymbol_cache);
-	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
-	                                           psymbol_compare);
+	  psymbol_bcache_free (objfile->psymbol_cache);
+	  objfile->psymbol_cache = psymbol_bcache_init ();
 	  bcache_xfree (objfile->macro_cache);
 	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
 	  bcache_xfree (objfile->filename_cache);
@@ -2459,8 +2458,7 @@ reread_symbols (void)
 	  memset (&objfile->msymbol_demangled_hash, 0,
 		  sizeof (objfile->msymbol_demangled_hash));
 
-	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
-	                                           psymbol_compare);
+	  objfile->psymbol_cache = psymbol_bcache_init ();
 	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
 	  objfile->filename_cache = bcache_xmalloc (NULL, NULL);
 	  /* obstack_init also initializes the obstack so it is
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 62e6b97..399843b 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -130,7 +130,8 @@ print_symbol_bcache_statistics (void)
     ALL_PSPACE_OBJFILES (pspace, objfile)
   {
     printf_filtered (_("Byte cache statistics for '%s':\n"), objfile->name);
-    print_bcache_statistics (objfile->psymbol_cache, "partial symbol cache");
+    print_bcache_statistics (objfile->psymbol_cache->bcache,
+                             "partial symbol cache");
     print_bcache_statistics (objfile->macro_cache, "preprocessor macro cache");
     print_bcache_statistics (objfile->filename_cache, "file name cache");
   }
@@ -188,7 +189,7 @@ print_objfile_statistics (void)
     printf_filtered (_("  Total memory used for objfile obstack: %d\n"),
 		     obstack_memory_used (&objfile->objfile_obstack));
     printf_filtered (_("  Total memory used for psymbol cache: %d\n"),
-		     bcache_memory_used (objfile->psymbol_cache));
+		     bcache_memory_used (objfile->psymbol_cache->bcache));
     printf_filtered (_("  Total memory used for macro cache: %d\n"),
 		     bcache_memory_used (objfile->macro_cache));
     printf_filtered (_("  Total memory used for file name cache: %d\n"),

[patch 2/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[sami wagiaalla]

On 08/19/2010 12:33 PM, sami wagiaalla wrote:
> On 08/18/2010 11:24 AM, Tom Tromey wrote:
>>>>>>> "Sami" == sami wagiaalla<swagiaal@redhat.com> writes:
>>
>> Sami> Although, you are right, there is no strict type checking. Are you
>> Sami> suggesting we make psymbol_cache a new type ?
>>
>> Yeah; at least, if it isn't too big.
>>
>
> Patch attached.
>

Updated subject; this^ is patch 2/2

Re: [patch 1/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> +/* Forwarding function for memcmp.  */
Sami> +//bcache_memcmp ()

Remove.

Sami> +bcache_xmalloc (unsigned long (*hash_function)(const void*, int length),
Sami> +                int (*compare_function)(const void*, const void*, int length))

Space in "void *".

Sami> +extern struct bcache *bcache_xmalloc (
Sami> +    unsigned long (*hash_function)(const void*, int length),
Sami> +    int (*compare_function)(const void*, const void*, int length));

Likewise.

Sami>    /* psymbol is static so that there will be no uninitialized gaps in the
Sami>       structure which might contain random data, causing cache misses in
Sami>       bcache. */
Sami> -  static struct partial_symbol psymbol;
Sami> +  struct partial_symbol psymbol;

Remove the comment here.  It is no longer valid.

Sami>    /* However, we must ensure that the entire 'value' field has been
Sami>       zeroed before assigning to it, because an assignment may not

We probably don't need this comment or the memset any more.

Tom

Re: [RFC] Use custom hash function with bcache

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> Although, you are right, there is no strict type checking. Are you
Sami> suggesting we make psymbol_cache a new type ?

Tom> Yeah; at least, if it isn't too big.

Sami> Patch attached.

Aside from some formatting nits, this looks good to me.

Sami> +struct psymbol_bcache *
Sami> +psymbol_bcache_init ()

(void)

Sami> +{
Sami> +    struct psymbol_bcache *bcache = XCALLOC (1, struct psymbol_bcache);
Sami> +    bcache->bcache = bcache_xmalloc (psymbol_hash, psymbol_compare);
Sami> +    return bcache;

Wrong amount of indentation here.
Also for a single object, use XNEW, not XCALLOC.

Sami> +void
Sami> +psymbol_bcache_free (struct psymbol_bcache *bcache)
Sami> +{
Sami> +
Sami> +  if (bcache == NULL)
Sami> +    return;

Spurious blank line.

Sami> +  bcache_xfree(bcache->bcache);

Space before paren.

Sami> +static const struct partial_symbol *
Sami> +psymbol_bcache_full (struct partial_symbol *sym,
Sami> +             struct psymbol_bcache *bcache,
Sami> +             int *added)
Sami> +{
Sami> +    return bcache_full (sym,

Wrong indentation.

Sami> +/* A bcache for partial symbols.  */
Sami> +
Sami> +struct psymbol_bcache {

I usually put the brace in column 0 on the next line.
I don't think we're consistent here though.

Sami> +extern struct psymbol_bcache *psymbol_bcache_init ();

(void)

Tom

Re: [patch 1/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[sami wagiaalla]

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

Revised patch attached.

[-- Attachment #2: custom_hash_1.patch --]
[-- Type: text/x-patch, Size: 10318 bytes --]

Enable custom bcache hash function.

2010-08-25  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (add_psymbol_to_bcache): Remove 'static' from
	'static partial_symbol psymbol'.
	(psymbol_hash): New function.
	(psymbol_compare): New function.
	* bcache.c (hash_continue): New.
	(hash): Use hash_continue.
	* bcache.c: Add hash_function and compare_function
	pointers to bcache struct.
	(bcache_full): Use bcache->hash_function, and
	bcache->compare_function.
	(bcache_compare): New function.
	(bcache_xmalloc): Take hash_function and
	compare_function arguments and initialize the
	bcach's pointers.
	Updated comment.
	* objfiles.c (allocate_objfile): Updated.
	* symfile.c (reread_symbols): Updated.

diff --git a/gdb/bcache.c b/gdb/bcache.c
index 7d9180c..b72bcaf 100644
--- a/gdb/bcache.c
+++ b/gdb/bcache.c
@@ -89,6 +89,12 @@ struct bcache
      16 bits of hash values) hit, but the corresponding combined
      length/data compare missed.  */
   unsigned long half_hash_miss_count;
+
+  /* Hash function to be used for this bcache object.  */
+  unsigned long (*hash_function)(const void *addr, int length);
+
+  /* Compare function to be used for this bcache object.  */
+  int (*compare_function)(const void *, const void *, int length);
 };
 
 /* The old hash function was stolen from SDBM. This is what DB 3.0 uses now,
@@ -98,12 +104,19 @@ struct bcache
 unsigned long
 hash(const void *addr, int length)
 {
+  return hash_continue (addr, length, 0);
+}
+
+/* Continue the calculation of the hash H at the given address.  */
+
+unsigned long
+hash_continue (const void *addr, int length, unsigned long h)
+{
   const unsigned char *k, *e;
-  unsigned long h;
 
   k = (const unsigned char *)addr;
   e = k+length;
-  for (h=0; k< e;++k)
+  for (; k< e;++k)
     {
       h *=16777619;
       h ^= *k;
@@ -235,7 +248,8 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
   bcache->total_count++;
   bcache->total_size += length;
 
-  full_hash = hash (addr, length);
+  full_hash = bcache->hash_function (addr, length);
+
   half_hash = (full_hash >> 16);
   hash_index = full_hash % bcache->num_buckets;
 
@@ -247,7 +261,7 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
       if (s->half_hash == half_hash)
 	{
 	  if (s->length == length
-	      && ! memcmp (&s->d.data, addr, length))
+	      && bcache->compare_function (&s->d.data, addr, length))
 	    return &s->d.data;
 	  else
 	    bcache->half_hash_miss_count++;
@@ -276,14 +290,39 @@ bcache_full (const void *addr, int length, struct bcache *bcache, int *added)
   }
 }
 \f
+
+/* Compare the byte string at ADDR1 of lenght LENGHT to the
+   string at ADDR2.  Return 1 if they are equal.  */
+
+static int
+bcache_compare (const void* addr1, const void* addr2, int length)
+{
+  return memcmp (addr1, addr2, length) == 0;
+}
+
 /* Allocating and freeing bcaches.  */
 
+/* Allocated a bcache.  HASH_FUNCTION and COMPARE_FUNCTION can be used
+   to pass in custom hash, and compare functions to be used by this
+   bcache. If HASH_FUNCTION is NULL hash() is used and if COMPARE_FUNCTION
+   is NULL memcmp() is used.  */
+
 struct bcache *
-bcache_xmalloc (void)
+bcache_xmalloc (unsigned long (*hash_function)(const void *, int length),
+                int (*compare_function)(const void *, const void *, int length))
 {
   /* Allocate the bcache pre-zeroed.  */
   struct bcache *b = XCALLOC (1, struct bcache);
 
+  if (hash_function)
+    b->hash_function = hash_function;
+  else
+    b->hash_function = hash;
+
+  if (compare_function)
+    b->compare_function = compare_function;
+  else
+    b->compare_function = bcache_compare;
   return b;
 }
 
diff --git a/gdb/bcache.h b/gdb/bcache.h
index da69a2d..d000962 100644
--- a/gdb/bcache.h
+++ b/gdb/bcache.h
@@ -158,7 +158,9 @@ extern const void *bcache_full (const void *addr, int length,
 extern void bcache_xfree (struct bcache *bcache);
 
 /* Create a new bcache object.  */
-extern struct bcache *bcache_xmalloc (void);
+extern struct bcache *bcache_xmalloc (
+    unsigned long (*hash_function)(const void *, int length),
+    int (*compare_function)(const void *, const void *, int length));
 
 /* Print statistics on BCACHE's memory usage and efficacity at
    eliminating duplication.  TYPE should be a string describing the
@@ -167,7 +169,9 @@ extern struct bcache *bcache_xmalloc (void);
 extern void print_bcache_statistics (struct bcache *bcache, char *type);
 extern int bcache_memory_used (struct bcache *bcache);
 
-/* The hash function */
+/* The hash functions */
 extern unsigned long hash(const void *addr, int length);
+extern unsigned long hash_continue (const void *addr, int length,
+                                    unsigned long h);
 
 #endif /* BCACHE_H */
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index b522189..c479d22 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -199,9 +199,9 @@ allocate_objfile (bfd *abfd, int flags)
   struct objfile *objfile;
 
   objfile = (struct objfile *) xzalloc (sizeof (struct objfile));
-  objfile->psymbol_cache = bcache_xmalloc ();
-  objfile->macro_cache = bcache_xmalloc ();
-  objfile->filename_cache = bcache_xmalloc ();
+  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash, psymbol_compare);
+  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
+  objfile->filename_cache = bcache_xmalloc (NULL, NULL);
   /* We could use obstack_specify_allocation here instead, but
      gdb_obstack.h specifies the alloc/dealloc functions.  */
   obstack_init (&objfile->objfile_obstack);
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index bc47681..aa7e3a1 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1270,6 +1270,47 @@ start_psymtab_common (struct objfile *objfile,
   return (psymtab);
 }
 
+/* Calculate a hash code for the given partial symbol.  The hash is
+   calculated using the symbol's value, language, domain, class
+   and name. These are the values which are set by
+   add_psymbol_to_bcache.  */
+
+unsigned long
+psymbol_hash (const void *addr, int length)
+{
+  unsigned long h = 0;
+  struct partial_symbol *psymbol = (struct partial_symbol *) addr;
+  unsigned int lang = psymbol->ginfo.language;
+  unsigned int domain = PSYMBOL_DOMAIN (psymbol);
+  unsigned int class = PSYMBOL_CLASS (psymbol);
+
+  h = hash_continue (&psymbol->ginfo.value, sizeof (psymbol->ginfo.value), h);
+  h = hash_continue (&lang, sizeof (unsigned int), h);
+  h = hash_continue (&domain, sizeof (unsigned int), h);
+  h = hash_continue (&class, sizeof (unsigned int), h);
+  h = hash_continue (psymbol->ginfo.name, strlen (psymbol->ginfo.name), h);
+
+  return h;
+}
+
+/* Returns true if the symbol at addr1 equals the symbol at addr2.
+   For the comparison this function uses a symbols value,
+   language, domain, class and name.  */
+
+int
+psymbol_compare (const void *addr1, const void *addr2, int length)
+{
+  struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
+  struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
+
+  return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
+                  sizeof (sym1->ginfo.value)) == 0
+	  && sym1->ginfo.language == sym2->ginfo.language
+          && PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
+          && PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2)
+          && sym1->ginfo.name == sym2->ginfo.name);
+}
+
 /* 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
@@ -1285,15 +1326,8 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
 		       enum language language, struct objfile *objfile,
 		       int *added)
 {
-  /* 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;
-
-  /* However, we must ensure that the entire 'value' field has been
-     zeroed before assigning to it, because an assignment may not
-     write the entire field.  */
-  memset (&psymbol.ginfo.value, 0, sizeof (psymbol.ginfo.value));
+  struct partial_symbol psymbol;
+
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index de8b67e..0786944 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -20,6 +20,9 @@
 #ifndef PSYMTAB_H
 #define PSYMTAB_H
 
+extern unsigned long psymbol_hash (const void *addr, int length);
+extern int psymbol_compare (const void *addr1, const void *addr2, int length);
+
 void map_partial_symbol_names (void (*) (const char *, void *), void *);
 
 void map_partial_symbol_filenames (void (*) (const char *, const char *,
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 2cb6b7f..048b8a8 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2432,11 +2432,12 @@ reread_symbols (void)
 
 	  /* Free the obstacks for non-reusable objfiles */
 	  bcache_xfree (objfile->psymbol_cache);
-	  objfile->psymbol_cache = bcache_xmalloc ();
+	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
+	                                           psymbol_compare);
 	  bcache_xfree (objfile->macro_cache);
-	  objfile->macro_cache = bcache_xmalloc ();
+	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
 	  bcache_xfree (objfile->filename_cache);
-	  objfile->filename_cache = bcache_xmalloc ();
+	  objfile->filename_cache = bcache_xmalloc (NULL,NULL);
 	  if (objfile->demangled_names_hash != NULL)
 	    {
 	      htab_delete (objfile->demangled_names_hash);
@@ -2458,9 +2459,10 @@ reread_symbols (void)
 	  memset (&objfile->msymbol_demangled_hash, 0,
 		  sizeof (objfile->msymbol_demangled_hash));
 
-	  objfile->psymbol_cache = bcache_xmalloc ();
-	  objfile->macro_cache = bcache_xmalloc ();
-	  objfile->filename_cache = bcache_xmalloc ();
+	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
+	                                           psymbol_compare);
+	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
+	  objfile->filename_cache = bcache_xmalloc (NULL, NULL);
 	  /* obstack_init also initializes the obstack so it is
 	     empty.  We could use obstack_specify_allocation but
 	     gdb_obstack.h specifies the alloc/dealloc

Re: [patch 2/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[sami wagiaalla]

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

Revised patch attached.

[-- Attachment #2: custom_hash_2.patch --]
[-- Type: text/x-patch, Size: 7967 bytes --]

Create and use a specialized bcache type for psymbols

2010-08-25  Sami Wagiaalla  <swagiaal@redhat.com>

	* symfile.c (reread_symbols): Use psymbol_bcache_free, and
	psymbol_bcache_init.
	* psymtab.h (psymbol_bcache_init): New function prototype.
	(psymbol_bcache_free): New function prototype.
	* psymtab.c (psymbol_bcache_init): New function.
	(psymbol_bcache_free): New function.
	(psymbol_bcache_full): New function.
	(add_psymbol_to_bcache): use psymbol_bcache_full.
	* objfiles.h (psymbol_cache): Change type of psymbol_cache to
	psymbol_bcache.
	* symmisc.c (print_symbol_bcache_statistics): Updated.
	(print_objfile_statistics): Updated.
	* objfiles.c (allocate_objfile): Use psymbol_bcache_init to initialize
	psymbol_cache.
	(free_objfile): Use psymbol_bcache_free.

diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index c479d22..6fc8a1f 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -199,7 +199,7 @@ allocate_objfile (bfd *abfd, int flags)
   struct objfile *objfile;
 
   objfile = (struct objfile *) xzalloc (sizeof (struct objfile));
-  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash, psymbol_compare);
+  objfile->psymbol_cache = psymbol_bcache_init ();
   objfile->macro_cache = bcache_xmalloc (NULL, NULL);
   objfile->filename_cache = bcache_xmalloc (NULL, NULL);
   /* We could use obstack_specify_allocation here instead, but
@@ -658,7 +658,7 @@ free_objfile (struct objfile *objfile)
   if (objfile->static_psymbols.list)
     xfree (objfile->static_psymbols.list);
   /* Free the obstacks for non-reusable objfiles */
-  bcache_xfree (objfile->psymbol_cache);
+  psymbol_bcache_free (objfile->psymbol_cache);
   bcache_xfree (objfile->macro_cache);
   bcache_xfree (objfile->filename_cache);
   if (objfile->demangled_names_hash)
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 62fc1cb..ec4870b 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -249,7 +249,7 @@ struct objfile
     /* A byte cache where we can stash arbitrary "chunks" of bytes that
        will not change. */
 
-    struct bcache *psymbol_cache;	/* Byte cache for partial syms */
+    struct psymbol_bcache *psymbol_cache; /* Byte cache for partial syms */
     struct bcache *macro_cache;          /* Byte cache for macros */
     struct bcache *filename_cache;	 /* Byte cache for file names.  */
 
diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index aa7e3a1..a80b975 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1275,7 +1275,7 @@ start_psymtab_common (struct objfile *objfile,
    and name. These are the values which are set by
    add_psymbol_to_bcache.  */
 
-unsigned long
+static unsigned long
 psymbol_hash (const void *addr, int length)
 {
   unsigned long h = 0;
@@ -1297,7 +1297,7 @@ psymbol_hash (const void *addr, int length)
    For the comparison this function uses a symbols value,
    language, domain, class and name.  */
 
-int
+static int
 psymbol_compare (const void *addr1, const void *addr2, int length)
 {
   struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
@@ -1311,6 +1311,43 @@ psymbol_compare (const void *addr1, const void *addr2, int length)
           && sym1->ginfo.name == sym2->ginfo.name);
 }
 
+/* Initialize a partial symbol bcache.  */
+
+struct psymbol_bcache *
+psymbol_bcache_init (void)
+{
+  struct psymbol_bcache *bcache = XCALLOC (1, struct psymbol_bcache);
+  bcache->bcache = bcache_xmalloc (psymbol_hash, psymbol_compare);
+  return bcache;
+}
+
+/* Free a partial symbol bcache.  */
+void
+psymbol_bcache_free (struct psymbol_bcache *bcache)
+{
+  if (bcache == NULL)
+    return;
+
+  bcache_xfree (bcache->bcache);
+  xfree (bcache);
+}
+
+/* Find a copy of the SYM in BCACHE.  If BCACHE has never seen this
+   symbol before, add a copy to BCACHE.  In either case, return a pointer
+   to BCACHE's copy of the symbol.  If optional ADDED is not NULL, return
+   1 in case of new entry or 0 if returning an old entry.  */
+
+static const struct partial_symbol *
+psymbol_bcache_full (struct partial_symbol *sym,
+                     struct psymbol_bcache *bcache,
+                     int *added)
+{
+  return bcache_full (sym,
+                      sizeof (struct partial_symbol),
+                      bcache->bcache,
+                      added);
+}
+
 /* 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
@@ -1345,8 +1382,9 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
   SYMBOL_SET_NAMES (&psymbol, name, namelength, copy_name, objfile);
 
   /* Stash the partial symbol away in the cache */
-  return bcache_full (&psymbol, sizeof (struct partial_symbol),
-		      objfile->psymbol_cache, added);
+  return psymbol_bcache_full (&psymbol,
+                              objfile->psymbol_cache,
+                              added);
 }
 
 /* Helper function, adds partial symbol to the given partial symbol
diff --git a/gdb/psymtab.h b/gdb/psymtab.h
index 0786944..9182f33 100644
--- a/gdb/psymtab.h
+++ b/gdb/psymtab.h
@@ -20,8 +20,15 @@
 #ifndef PSYMTAB_H
 #define PSYMTAB_H
 
-extern unsigned long psymbol_hash (const void *addr, int length);
-extern int psymbol_compare (const void *addr1, const void *addr2, int length);
+/* A bcache for partial symbols.  */
+
+struct psymbol_bcache
+{
+    struct bcache *bcache;
+};
+
+extern struct psymbol_bcache *psymbol_bcache_init (void);
+extern void psymbol_bcache_free (struct psymbol_bcache *);
 
 void map_partial_symbol_names (void (*) (const char *, void *), void *);
 
diff --git a/gdb/symfile.c b/gdb/symfile.c
index 048b8a8..087cd3a 100644
--- a/gdb/symfile.c
+++ b/gdb/symfile.c
@@ -2431,9 +2431,8 @@ reread_symbols (void)
 		  sizeof (objfile->static_psymbols));
 
 	  /* Free the obstacks for non-reusable objfiles */
-	  bcache_xfree (objfile->psymbol_cache);
-	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
-	                                           psymbol_compare);
+	  psymbol_bcache_free (objfile->psymbol_cache);
+	  objfile->psymbol_cache = psymbol_bcache_init ();
 	  bcache_xfree (objfile->macro_cache);
 	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
 	  bcache_xfree (objfile->filename_cache);
@@ -2459,8 +2458,7 @@ reread_symbols (void)
 	  memset (&objfile->msymbol_demangled_hash, 0,
 		  sizeof (objfile->msymbol_demangled_hash));
 
-	  objfile->psymbol_cache = bcache_xmalloc (psymbol_hash,
-	                                           psymbol_compare);
+	  objfile->psymbol_cache = psymbol_bcache_init ();
 	  objfile->macro_cache = bcache_xmalloc (NULL, NULL);
 	  objfile->filename_cache = bcache_xmalloc (NULL, NULL);
 	  /* obstack_init also initializes the obstack so it is
diff --git a/gdb/symmisc.c b/gdb/symmisc.c
index 62e6b97..399843b 100644
--- a/gdb/symmisc.c
+++ b/gdb/symmisc.c
@@ -130,7 +130,8 @@ print_symbol_bcache_statistics (void)
     ALL_PSPACE_OBJFILES (pspace, objfile)
   {
     printf_filtered (_("Byte cache statistics for '%s':\n"), objfile->name);
-    print_bcache_statistics (objfile->psymbol_cache, "partial symbol cache");
+    print_bcache_statistics (objfile->psymbol_cache->bcache,
+                             "partial symbol cache");
     print_bcache_statistics (objfile->macro_cache, "preprocessor macro cache");
     print_bcache_statistics (objfile->filename_cache, "file name cache");
   }
@@ -188,7 +189,7 @@ print_objfile_statistics (void)
     printf_filtered (_("  Total memory used for objfile obstack: %d\n"),
 		     obstack_memory_used (&objfile->objfile_obstack));
     printf_filtered (_("  Total memory used for psymbol cache: %d\n"),
-		     bcache_memory_used (objfile->psymbol_cache));
+		     bcache_memory_used (objfile->psymbol_cache->bcache));
     printf_filtered (_("  Total memory used for macro cache: %d\n"),
 		     bcache_memory_used (objfile->macro_cache));
     printf_filtered (_("  Total memory used for file name cache: %d\n"),

Re: [patch 1/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> Enable custom bcache hash function.
Sami> 2010-08-25  Sami Wagiaalla  <swagiaal@redhat.com>
[...]

This is ok.  Thanks.

Tom

Re: [patch 2/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[Tom Tromey]

>>>>> "Sami" == sami wagiaalla <swagiaal@redhat.com> writes:

Sami> Revised patch attached.
Sami> Create and use a specialized bcache type for psymbols

Thanks for doing this.  A couple nits, nothing major.

Sami> +/* A bcache for partial symbols.  */
Sami> +
Sami> +struct psymbol_bcache
Sami> +{
Sami> +    struct bcache *bcache;

Too much indentation here.

Since psymbol_bcache is only used in psymtab.c, the definition should be
moved there, leaving just a forward declaration of the tag in psymtab.h.
This makes the type opaque, which is generally better.

Tom

Re: [patch 2/2] Use custom hash function with bcache [Re: [RFC] Use custom hash function with bcache]

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Too much indentation here.

Tom> Since psymbol_bcache is only used in psymtab.c, the definition should be
Tom> moved there, leaving just a forward declaration of the tag in psymtab.h.
Tom> This makes the type opaque, which is generally better.

I forgot to mention -- this is ok with those 2 changes.  Thanks again.


Tom

Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Jan Kratochvil]

On Wed, 25 Aug 2010 20:30:10 +0200, sami wagiaalla wrote:
> Enable custom bcache hash function.
> 
> 2010-08-25  Sami Wagiaalla  <swagiaal@redhat.com>
> 
> 	* psymtab.c (add_psymbol_to_bcache): Remove 'static' from
> 	'static partial_symbol psymbol'.
> 	(psymbol_hash): New function.
> 	(psymbol_compare): New function.
> 	* bcache.c (hash_continue): New.
> 	(hash): Use hash_continue.
> 	* bcache.c: Add hash_function and compare_function
> 	pointers to bcache struct.
> 	(bcache_full): Use bcache->hash_function, and
> 	bcache->compare_function.
> 	(bcache_compare): New function.
> 	(bcache_xmalloc): Take hash_function and
> 	compare_function arguments and initialize the
> 	bcach's pointers.
> 	Updated comment.
> 	* objfiles.c (allocate_objfile): Updated.
> 	* symfile.c (reread_symbols): Updated.

fb846e72510040325bffd8f755180ea0025108dc
http://sourceware.org/ml/gdb-cvs/2010-08/msg00197.html

It has a regression for gdb.stabs/gdb11479.exp on all the tested distros
{x86_64,x86_64-m32,i686}-fedora{12,13,14snapshot}-linux-gnu even without
-lmcheck.


Regards,
Jan

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Joel Brobecker]

> > 2010-08-25  Sami Wagiaalla  <swagiaal@redhat.com>
> > 
> > 	* psymtab.c (add_psymbol_to_bcache): Remove 'static' from
> > 	'static partial_symbol psymbol'.
> > 	(psymbol_hash): New function.
> > 	(psymbol_compare): New function.
> > 	* bcache.c (hash_continue): New.
> > 	(hash): Use hash_continue.
> > 	* bcache.c: Add hash_function and compare_function
> > 	pointers to bcache struct.
> > 	(bcache_full): Use bcache->hash_function, and
> > 	bcache->compare_function.
> > 	(bcache_compare): New function.
> > 	(bcache_xmalloc): Take hash_function and
> > 	compare_function arguments and initialize the
> > 	bcach's pointers.
> > 	Updated comment.
> > 	* objfiles.c (allocate_objfile): Updated.
> > 	* symfile.c (reread_symbols): Updated.
> 
> fb846e72510040325bffd8f755180ea0025108dc
> http://sourceware.org/ml/gdb-cvs/2010-08/msg00197.html
> 
> It has a regression for gdb.stabs/gdb11479.exp on all the tested distros
> {x86_64,x86_64-m32,i686}-fedora{12,13,14snapshot}-linux-gnu even without
> -lmcheck.

Just so we don't duplicate efforts, I've have been looking into this.
I'm not completely done, yet, but I've found something suspicious.
For me, it causes a crash in gdb.ada/complete.exp:

    (gdb) b 7
    Breakpoint 1 at 0x401f96: file /[...]/foo.adb, line 7.
    (gdb) run
    Starting program: /[...]/foo 
    
    Breakpoint 1, foo () at /[...]/foo.adb:7
    7          My_Global_Variable := Some_Local_Variable + 1; -- START
    (gdb) complete p my_glob
    [1]    22302 segmentation fault  ../../../gdb foo


-- 
Joel

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Joel Brobecker]

> Just so we don't duplicate efforts, I've have been looking into this.
> I'm not completely done, yet, but I've found something suspicious.

Unfortunately, it turned out to be nothing (those damn macros).

> For me, it causes a crash in gdb.ada/complete.exp:
> 
>     (gdb) b 7
>     Breakpoint 1 at 0x401f96: file /[...]/foo.adb, line 7.
>     (gdb) run
>     Starting program: /[...]/foo 
>     
>     Breakpoint 1, foo () at /[...]/foo.adb:7
>     7          My_Global_Variable := Some_Local_Variable + 1; -- START
>     (gdb) complete p my_glob
>     [1]    22302 segmentation fault  ../../../gdb foo

As far as I can tell, it looks like there is either a memory
corruption somewhere, or we fail to set a field in the psymbol
ginfo. We iterate over all psymbols, and the associated gsym
has an invalid obj_section.

Unfortunately, that's as much time as I have for today. I will look
at it again tomorrow if no one else can.  I does looks like a pretty
scary issue, so I think we should try to fix it ASAP.

-- 
Joel

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[sami wagiaalla]

> Unfortunately, that's as much time as I have for today. I will look
> at it again tomorrow if no one else can.  I does looks like a pretty
> scary issue, so I think we should try to fix it ASAP.
>

Agreed. I apologize for the breakage. I am working on this and will try 
to fix it ASAP. I will also look to try to figure out why I missed this 
in my testing.

Thanks for the info Jan and Joel.

Sami

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Joel Brobecker]

> Agreed. I apologize for the breakage. I am working on this and will
> try to fix it ASAP. I will also look to try to figure out why I
> missed this in my testing.

I wouldn't worry about having missed this: stabs is hardly every used
and certainly not by default, and I know that we're still behind
submitting debug-info-generation patches for Ada in GCC. So I know that
it is difficult for anyone outside of AdaCore to really run the gdb.ada
testcases.

My worry is that we might have 2 distinct issues. I don't see yet
how breakage with stabs and Ada can be related...

-- 
Joel

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[sami wagiaalla]

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


> As far as I can tell, it looks like there is either a memory
> corruption somewhere, or we fail to set a field in the psymbol
> ginfo. We iterate over all psymbols, and the associated gsym
> has an invalid obj_section.
> 

Yup exactly. This part of the patch caused the problem:

>   /* 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
> @@ -1285,15 +1326,8 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
>   		       enum language language, struct objfile *objfile,
>   		       int *added)
>   {
> -  /* 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;
> -

I thought this was OK because the new hash function looked only at the
initialized fields. Of course this left obj_section uninitialized.
Adding:

  memset (&psymbol, 0, sizeof (psymbol));

seems to fix the problem:

Running ../../../gdb/testsuite/gdb.ada/complete.exp ...
Running ../../../gdb/testsuite/gdb.stabs/gdb11479.exp ...

		=== gdb Summary ===

# of expected passes		39

The problem with my testing is that my script did not look for the word
ERROR in the log file.

The attached patch has been regression tested on F13 gcc 4.4.4... 
properly :)



[-- Attachment #2: hash_regression.patch --]
[-- Type: text/x-patch, Size: 565 bytes --]

Fix custom hash regression.

2010-09-01  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (add_psymbol_to_bcache): memset psymbol to 0
	before initializing individual fields.

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index a5d2f98..a8b63f7 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1378,6 +1378,8 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
 {
   struct partial_symbol psymbol;
 
+  memset (&psymbol, 0, sizeof (psymbol));
+
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Jan Kratochvil]

On Wed, 01 Sep 2010 20:10:02 +0200, sami wagiaalla wrote:
> The attached patch has been regression tested on F13 gcc 4.4.4... 
> properly :)

Confirming I also find it fixed now.


Thanks,
Jan

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 11:10 AM, sami wagiaalla <swagiaal@redhat.com> wrote:
> Yup exactly. This part of the patch caused the problem:
>
>>   /* 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
>> @@ -1285,15 +1326,8 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
>>                      enum language language, struct objfile *objfile,
>>                      int *added)
>>   {
>> -  /* 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;
>> -
>
> I thought this was OK because the new hash function looked only at the
> initialized fields. Of course this left obj_section uninitialized.
> Adding:
>
>  memset (&psymbol, 0, sizeof (psymbol));
>
> seems to fix the problem:

One would expect the original code to have done a memset too, instead
of using "static".  Presumably it didn't for performance reasons.  Do
we know if the performance concerns were real?

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Tom Tromey]

>>>>> "Doug" == Doug Evans <dje@google.com> writes:

Doug> One would expect the original code to have done a memset too, instead
Doug> of using "static".  Presumably it didn't for performance reasons.  Do
Doug> we know if the performance concerns were real?

No, we don't know.
It is safest to just revert to what it was before.

The end of the previous thread is here
http://sourceware.org/ml/gdb-patches/2009-11/msg00104.html

Tom

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[sami wagiaalla]

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

On 09/01/2010 02:37 PM, Tom Tromey wrote:
>>>>>> "Doug" == Doug Evans<dje@google.com>  writes:
>
> Doug>  One would expect the original code to have done a memset too, instead
> Doug>  of using "static".  Presumably it didn't for performance reasons.  Do
> Doug>  we know if the performance concerns were real?
>
> No, we don't know.
> It is safest to just revert to what it was before.
>

Sounds good to me. Patch attached.

Sami

[-- Attachment #2: hash_regression.patch --]
[-- Type: text/x-patch, Size: 1042 bytes --]

Fix custom hash regression.

2010-09-01  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (add_psymbol_to_bcache): Declare psymbol
	as static.
	memset psymbol.ginfo.value to 0.

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index a5d2f98..24dd301 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1376,7 +1376,15 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
 		       enum language language, struct objfile *objfile,
 		       int *added)
 {
-  struct partial_symbol psymbol;
+  /* 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;
+
+  /* However, we must ensure that the entire 'value' field has been
+     zeroed before assigning to it, because an assignment may not
+     write the entire field.  */
+  memset (&psymbol.ginfo.value, 0, sizeof (psymbol.ginfo.value));
 
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 12:01 PM, sami wagiaalla <swagiaal@redhat.com> wrote:
> On 09/01/2010 02:37 PM, Tom Tromey wrote:
>>>>>>>
>>>>>>> "Doug" == Doug Evans<dje@google.com>  writes:
>>
>> Doug>  One would expect the original code to have done a memset too,
>> instead
>> Doug>  of using "static".  Presumably it didn't for performance reasons.
>>  Do
>> Doug>  we know if the performance concerns were real?
>>
>> No, we don't know.
>> It is safest to just revert to what it was before.

I hope I'm not reducing the S/N ratio here, but there's something I
don't understand.
The original patch doesn't initialize obj_section (right?) (except by
virtue of using "static").  So if this fixes things, does it do so
only because we're taking advantage of the fact that obj_section will
be NULL in the static version and that's sufficient?  So do we need
the memset of the original patch (which only zeros
psymbol.ginfo.value).

Plus, reverting to the original patch leaves me a little
uncomfortable: There's a comment that mentions gaps in the struct
causing cache misses, but that's no longer an issue with the custom
hash function (right?).

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 12:01 PM, sami wagiaalla <swagiaal@redhat.com> wrote:
> On 09/01/2010 02:37 PM, Tom Tromey wrote:
>>>>>>>
>>>>>>> "Doug" == Doug Evans<dje@google.com>  writes:
>>
>> Doug>  One would expect the original code to have done a memset too,
>> instead
>> Doug>  of using "static".  Presumably it didn't for performance reasons.
>>  Do
>> Doug>  we know if the performance concerns were real?
>>
>> No, we don't know.
>> It is safest to just revert to what it was before.

I hope I'm not reducing the S/N ratio here, but there's something I
don't understand.
The original patch doesn't initialize obj_section (right?) (except by
virtue of using "static").  So if this fixes things, does it do so
only because we're taking advantage of the fact that obj_section will
be NULL in the static version and that's sufficient?  So do we need
the memset of the original patch (which only zeros
psymbol.ginfo.value).

Plus, reverting to the original patch leaves me a little
uncomfortable: There's a comment that mentions gaps in the struct
causing cache misses, but that's no longer an issue with the custom
hash function (right?).

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[sami wagiaalla]

On 09/01/2010 03:14 PM, Doug Evans wrote:
> On Wed, Sep 1, 2010 at 12:01 PM, sami wagiaalla<swagiaal@redhat.com>  wrote:
>> On 09/01/2010 02:37 PM, Tom Tromey wrote:
>>>>>>>>
>>>>>>>> "Doug" == Doug Evans<dje@google.com>    writes:
>>>
>>> Doug>    One would expect the original code to have done a memset too,
>>> instead
>>> Doug>    of using "static".  Presumably it didn't for performance reasons.
>>>   Do
>>> Doug>    we know if the performance concerns were real?
>>>
>>> No, we don't know.
>>> It is safest to just revert to what it was before.
>
> I hope I'm not reducing the S/N ratio here, but there's something I
> don't understand.
> The original patch doesn't initialize obj_section (right?) (except by
> virtue of using "static").  So if this fixes things, does it do so
> only because we're taking advantage of the fact that obj_section will
> be NULL in the static version

Yes.

  and that's sufficient?

IIUC, it should be.

   So do we need
> the memset of the original patch (which only zeros
> psymbol.ginfo.value).
>

I don't really see why it is needed but I am inclined to believe the 
comment.

> Plus, reverting to the original patch leaves me a little
> uncomfortable: There's a comment that mentions gaps in the struct
> causing cache misses, but that's no longer an issue with the custom
> hash function (right?).

I should probably fix that comment. The gabs in the struct, while bad, 
would not cause hash misses since the new supplied hash function only 
looks at the initialized fields.

Sami

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 12:28 PM, sami wagiaalla <swagiaal@redhat.com> wrote:
>> I hope I'm not reducing the S/N ratio here, but there's something I
>> don't understand.
>> The original patch doesn't initialize obj_section (right?) (except by
>> virtue of using "static").  So if this fixes things, does it do so
>> only because we're taking advantage of the fact that obj_section will
>> be NULL in the static version
>
> Yes.
>
>  and that's sufficient?
>
> IIUC, it should be.
>
>  So do we need
>>
>> the memset of the original patch (which only zeros
>> psymbol.ginfo.value).
>>
>
> I don't really see why it is needed but I am inclined to believe the
> comment.

I think that stems from the old
bcache-the-entire-thing-as-a-byte-array behaviour, but that's gone now
(thankfully).

>> Plus, reverting to the original patch leaves me a little
>> uncomfortable: There's a comment that mentions gaps in the struct
>> causing cache misses, but that's no longer an issue with the custom
>> hash function (right?).
>
> I should probably fix that comment. The gabs in the struct, while bad, would
> not cause hash misses since the new supplied hash function only looks at the
> initialized fields.

Looking at the struct, and what's left to initialize, I like this patch.
Now everything is initialized.


2010-09-01  Doug Evans  <dje@google.com>

        * psymtab.c (add_psymbol_to_bcache): Initialize obj_section.

Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.12
diff -u -p -r1.12 psymtab.c
--- psymtab.c   1 Sep 2010 21:50:26 -0000       1.12
+++ psymtab.c   1 Sep 2010 22:50:05 -0000
@@ -1394,6 +1394,7 @@ add_psymbol_to_bcache (char *name, int n
       SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
     }
   SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
   SYMBOL_SET_LANGUAGE (&psymbol, language);
   PSYMBOL_DOMAIN (&psymbol) = domain;
   PSYMBOL_CLASS (&psymbol) = class;

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 12:28 PM, sami wagiaalla <swagiaal@redhat.com> wrote:
>> I hope I'm not reducing the S/N ratio here, but there's something I
>> don't understand.
>> The original patch doesn't initialize obj_section (right?) (except by
>> virtue of using "static").  So if this fixes things, does it do so
>> only because we're taking advantage of the fact that obj_section will
>> be NULL in the static version
>
> Yes.
>
>  and that's sufficient?
>
> IIUC, it should be.
>
>  So do we need
>>
>> the memset of the original patch (which only zeros
>> psymbol.ginfo.value).
>>
>
> I don't really see why it is needed but I am inclined to believe the
> comment.

I think that stems from the old
bcache-the-entire-thing-as-a-byte-array behaviour, but that's gone now
(thankfully).

>> Plus, reverting to the original patch leaves me a little
>> uncomfortable: There's a comment that mentions gaps in the struct
>> causing cache misses, but that's no longer an issue with the custom
>> hash function (right?).
>
> I should probably fix that comment. The gabs in the struct, while bad, would
> not cause hash misses since the new supplied hash function only looks at the
> initialized fields.

Looking at the struct, and what's left to initialize, I like this patch.
Now everything is initialized.


2010-09-01  Doug Evans  <dje@google.com>

        * psymtab.c (add_psymbol_to_bcache): Initialize obj_section.

Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.12
diff -u -p -r1.12 psymtab.c
--- psymtab.c   1 Sep 2010 21:50:26 -0000       1.12
+++ psymtab.c   1 Sep 2010 22:50:05 -0000
@@ -1394,6 +1394,7 @@ add_psymbol_to_bcache (char *name, int n
       SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
     }
   SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
   SYMBOL_SET_LANGUAGE (&psymbol, language);
   PSYMBOL_DOMAIN (&psymbol) = domain;
   PSYMBOL_CLASS (&psymbol) = class;

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 12:28 PM, sami wagiaalla <swagiaal@redhat.com> wrote:
>> I hope I'm not reducing the S/N ratio here, but there's something I
>> don't understand.
>> The original patch doesn't initialize obj_section (right?) (except by
>> virtue of using "static").  So if this fixes things, does it do so
>> only because we're taking advantage of the fact that obj_section will
>> be NULL in the static version
>
> Yes.
>
>  and that's sufficient?
>
> IIUC, it should be.
>
>  So do we need
>>
>> the memset of the original patch (which only zeros
>> psymbol.ginfo.value).
>>
>
> I don't really see why it is needed but I am inclined to believe the
> comment.

I think that stems from the old
bcache-the-entire-thing-as-a-byte-array behaviour, but that's gone now
(thankfully).

>> Plus, reverting to the original patch leaves me a little
>> uncomfortable: There's a comment that mentions gaps in the struct
>> causing cache misses, but that's no longer an issue with the custom
>> hash function (right?).
>
> I should probably fix that comment. The gabs in the struct, while bad, would
> not cause hash misses since the new supplied hash function only looks at the
> initialized fields.

Looking at the struct, and what's left to initialize, I like this patch.
Now everything is initialized.


2010-09-01  Doug Evans  <dje@google.com>

        * psymtab.c (add_psymbol_to_bcache): Initialize obj_section.

Index: psymtab.c
===================================================================
RCS file: /cvs/src/src/gdb/psymtab.c,v
retrieving revision 1.12
diff -u -p -r1.12 psymtab.c
--- psymtab.c   1 Sep 2010 21:50:26 -0000       1.12
+++ psymtab.c   1 Sep 2010 22:50:05 -0000
@@ -1394,6 +1394,7 @@ add_psymbol_to_bcache (char *name, int n
       SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
     }
   SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
   SYMBOL_SET_LANGUAGE (&psymbol, language);
   PSYMBOL_DOMAIN (&psymbol) = domain;
   PSYMBOL_CLASS (&psymbol) = class;

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Wed, Sep 1, 2010 at 4:02 PM, Doug Evans <dje@google.com> wrote:
> I think that stems from the old
> bcache-the-entire-thing-as-a-byte-array behaviour, but that's gone now
> (thankfully).

Err, s/bcache-the/hash-the/

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[sami wagiaalla]

This sounds good to me. If we are not using a static psymbol struct 
should we zero out the value union at least ?

> 2010-09-01  Doug Evans<dje@google.com>
>
>          * psymtab.c (add_psymbol_to_bcache): Initialize obj_section.
>
> Index: psymtab.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/psymtab.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 psymtab.c
> --- psymtab.c   1 Sep 2010 21:50:26 -0000       1.12
> +++ psymtab.c   1 Sep 2010 22:50:05 -0000
> @@ -1394,6 +1394,7 @@ add_psymbol_to_bcache (char *name, int n
>         SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
>       }
>     SYMBOL_SECTION (&psymbol) = 0;
> +  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
>     SYMBOL_SET_LANGUAGE (&psymbol, language);
>     PSYMBOL_DOMAIN (&psymbol) = domain;
>     PSYMBOL_CLASS (&psymbol) = class;

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Thu, Sep 2, 2010 at 8:36 AM, sami wagiaalla <swagiaal@redhat.com> wrote:
> This sounds good to me. If we are not using a static psymbol struct should
> we zero out the value union at least ?
>
>> 2010-09-01  Doug Evans<dje@google.com>
>>
>>         * psymtab.c (add_psymbol_to_bcache): Initialize obj_section.
>>
>> Index: psymtab.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/psymtab.c,v
>> retrieving revision 1.12
>> diff -u -p -r1.12 psymtab.c
>> --- psymtab.c   1 Sep 2010 21:50:26 -0000       1.12
>> +++ psymtab.c   1 Sep 2010 22:50:05 -0000
>> @@ -1394,6 +1394,7 @@ add_psymbol_to_bcache (char *name, int n
>>        SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
>>      }
>>    SYMBOL_SECTION (&psymbol) = 0;
>> +  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
>>    SYMBOL_SET_LANGUAGE (&psymbol, language);
>>    PSYMBOL_DOMAIN (&psymbol) = domain;
>>    PSYMBOL_CLASS (&psymbol) = class;

The value union was only zeroed out before in case sizeof
(SYMBOL_VALUE) != sizeof (SYMBOL_VALUE_ADDRESS) != sizeof (union), to
maintain a consistent hash.
The code does set the value union, and we no longer hash as a byte
array, so I think we're fine.

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Doug Evans]

On Thu, Sep 2, 2010 at 12:01 PM, Doug Evans <dje@google.com> wrote:
> On Thu, Sep 2, 2010 at 8:36 AM, sami wagiaalla <swagiaal@redhat.com> wrote:
>> This sounds good to me. If we are not using a static psymbol struct should
>> we zero out the value union at least ?
>>
>>> 2010-09-01  Doug Evans<dje@google.com>
>>>
>>>         * psymtab.c (add_psymbol_to_bcache): Initialize obj_section.
>>>
>>> Index: psymtab.c
>>> ===================================================================
>>> RCS file: /cvs/src/src/gdb/psymtab.c,v
>>> retrieving revision 1.12
>>> diff -u -p -r1.12 psymtab.c
>>> --- psymtab.c   1 Sep 2010 21:50:26 -0000       1.12
>>> +++ psymtab.c   1 Sep 2010 22:50:05 -0000
>>> @@ -1394,6 +1394,7 @@ add_psymbol_to_bcache (char *name, int n
>>>        SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
>>>      }
>>>    SYMBOL_SECTION (&psymbol) = 0;
>>> +  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
>>>    SYMBOL_SET_LANGUAGE (&psymbol, language);
>>>    PSYMBOL_DOMAIN (&psymbol) = domain;
>>>    PSYMBOL_CLASS (&psymbol) = class;
>
> The value union was only zeroed out before in case sizeof
> (SYMBOL_VALUE) != sizeof (SYMBOL_VALUE_ADDRESS) != sizeof (union), to
> maintain a consistent hash.
> The code does set the value union, and we no longer hash as a byte
> array, so I think we're fine.

Blech.  "never mind".  We do need the memset.
The hash still includes sizeof (ginfo.value), which is reasonable (I
thought it didn't for some reason).
gcc turns it in to a move instruction anyway so no worries there.

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[sami wagiaalla]

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


> Blech.  "never mind".  We do need the memset.
> The hash still includes sizeof (ginfo.value), which is reasonable (I
> thought it didn't for some reason).
> gcc turns it in to a move instruction anyway so no worries there.

Alright, the attached patch memsets value and initializes obj_section as 
you suggested. Is this ok to commit ?

[-- Attachment #2: hash_regression.patch --]
[-- Type: text/x-patch, Size: 1092 bytes --]

Fix custom hash regression.

2010-09-03  Sami Wagiaalla  <swagiaal@redhat.com>

	* psymtab.c (add_psymbol_to_bcache): Initialize
	obj_section.
	memset psymbol.ginfo.value to 0.

diff --git a/gdb/psymtab.c b/gdb/psymtab.c
index a5d2f98..2f1bfc6 100644
--- a/gdb/psymtab.c
+++ b/gdb/psymtab.c
@@ -1378,6 +1378,11 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
 {
   struct partial_symbol psymbol;
 
+  /* We must ensure that the entire 'value' field has been zeroed
+     before assigning to it, because an assignment may not write the
+     entire field.  */
+  memset (&psymbol.ginfo.value, 0, sizeof (psymbol.ginfo.value));
+
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -1388,6 +1393,7 @@ add_psymbol_to_bcache (char *name, int namelength, int copy_name,
       SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
     }
   SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_OBJ_SECTION (&psymbol) = NULL;
   SYMBOL_SET_LANGUAGE (&psymbol, language);
   PSYMBOL_DOMAIN (&psymbol) = domain;
   PSYMBOL_CLASS (&psymbol) = class;

Re: Regression for gdb.stabs/gdb11479.exp [Re: [patch 1/2] Use custom hash function with bcache]

[Daniel Jacobowitz]

On Fri, Sep 03, 2010 at 04:03:07PM -0400, sami wagiaalla wrote:
> 
> >Blech.  "never mind".  We do need the memset.
> >The hash still includes sizeof (ginfo.value), which is reasonable (I
> >thought it didn't for some reason).
> >gcc turns it in to a move instruction anyway so no worries there.
> 
> Alright, the attached patch memsets value and initializes obj_section
> as you suggested. Is this ok to commit ?

This works for me; I think it's OK to commit.


-- 
Daniel Jacobowitz
CodeSourcery