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

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

[Aleksandar Ristovski]

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

Hello,

The attached patch checks if partial_symbol has already been added to the list 
instead of adding duplicate records.

Tested on linux x86, no regression.

Thanks,

---
Aleksandar Ristovski
QNX Software Systems


ChangeLog:
2008-02-11  Aleksandar Ristovski  <aristovski@qnx.com>

	* symfile.c (add_psymbol_to_list): Do not alloca and copy name
	if it's already properly terminated. Check if the partial_symbol
	structure has already been added to the list. If it has, do not
	add it again.

[-- Attachment #2: symfile.c.diff --]
[-- Type: text/plain, Size: 1665 bytes --]

Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.198
diff -u -p -r1.198 symfile.c
--- gdb/symfile.c	29 Jan 2008 22:47:20 -0000	1.198
+++ gdb/symfile.c	11 Feb 2008 20:12:48 -0000
@@ -3102,15 +3103,20 @@ add_psymbol_to_list (char *name, int nam
 		     enum language language, struct objfile *objfile)
 {
   struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
+  struct partial_symbol **ps;
+  char *buf = name;  
   /* psymbol is static so that there will be no uninitialized gaps in the
      structure which might contain random data, causing cache misses in
      bcache. */
   static struct partial_symbol psymbol;
-
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -3131,6 +3137,14 @@ add_psymbol_to_list (char *name, int nam
   psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
 			    objfile->psymbol_cache);
 
+  /* Check if the partial_symbol is already in the list. Do not add
+     it again if it is.  */
+  for (ps = list->list; ps != list->next; ps++)
+    {
+      if (psym == *ps)
+	return psym;
+    }
+
   /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
   if (list->next >= list->list + list->size)
     {

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

[Daniel Jacobowitz]

On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
> The attached patch checks if partial_symbol has already been added to the 
> list instead of adding duplicate records.

How does this ever happen?  It seems very wrong.  Also, I am worried
that the linear search will be a bottleneck (this is quadratic as each
psymtab grows).

> @@ -3102,15 +3103,20 @@ add_psymbol_to_list (char *name, int nam
>  		     enum language language, struct objfile *objfile)
>  {
>    struct partial_symbol *psym;
> -  char *buf = alloca (namelength + 1);
> +  struct partial_symbol **ps;
> +  char *buf = name;  
>    /* psymbol is static so that there will be no uninitialized gaps in the
>       structure which might contain random data, causing cache misses in
>       bcache. */
>    static struct partial_symbol psymbol;
> -
> -  /* Create local copy of the partial symbol */
> -  memcpy (buf, name, namelength);
> -  buf[namelength] = '\0';
> +  
> +  if (name[namelength] != '\0')
> +    {
> +      buf = alloca (namelength + 1);
> +      /* Create local copy of the partial symbol */
> +      memcpy (buf, name, namelength);
> +      buf[namelength] = '\0';
> +    }
>    /* val and coreaddr are mutually exclusive, one of them *will* be zero */
>    if (val != 0)
>      {

Even easier: eliminate the copy entirely.  SYMBOL_SET_NAMES -> symbol_set_names
already does this and it never modifies its argument.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
>> The attached patch checks if partial_symbol has already been added to the 
>> list instead of adding duplicate records.
> 
> How does this ever happen?  It seems very wrong.  Also, I am worried
> that the linear search will be a bottleneck (this is quadratic as each
> psymtab grows).

Yes, I understand your concern about the complexity... but...

As a reference point, the binary I was mentioning here:
http://sourceware.org/ml/gdb-patches/2008-02/msg00174.html
with 9M+ partial symbols, after the patch it dropped to below 100K partial symbols:
Statistics:
     Number of psymtabs         :    16659
     Number of global symbols   :    98286
     Number of static symbols   :    59275
     Number of glob. sym. sorts :        4

Now the sorting is much less costly.

> 
> 
> Even easier: eliminate the copy entirely.  SYMBOL_SET_NAMES -> symbol_set_names
> already does this and it never modifies its argument.
> 
I will take a look.

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

[Daniel Jacobowitz]

On Mon, Feb 11, 2008 at 03:51:59PM -0500, Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
>> On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
>>> The attached patch checks if partial_symbol has already been added to 
>>> the list instead of adding duplicate records.
>>
>> How does this ever happen?  It seems very wrong.  Also, I am worried
>> that the linear search will be a bottleneck (this is quadratic as each
>> psymtab grows).
>
> Yes, I understand your concern about the complexity... but...

That's only part of the problem.  You have this huge duplication of
identical partial symbols within the same block.  How did that happen?
It shouldn't.  Maybe we can avoid creating them in the first place.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 03:51:59PM -0500, Aleksandar Ristovski wrote:
>> Daniel Jacobowitz wrote:
>>> On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
>>>> The attached patch checks if partial_symbol has already been added to 
>>>> the list instead of adding duplicate records.
>>> How does this ever happen?  It seems very wrong.  Also, I am worried
>>> that the linear search will be a bottleneck (this is quadratic as each
>>> psymtab grows).
>> Yes, I understand your concern about the complexity... but...
> 
> That's only part of the problem.  You have this huge duplication of
> identical partial symbols within the same block.  How did that happen?
> It shouldn't.  Maybe we can avoid creating them in the first place.
> 
Daniel, could you clarify: when you say "maybe we can avoid..." who is "we" - 
gdb or gcc?

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

[Daniel Jacobowitz]

On Mon, Feb 11, 2008 at 04:41:08PM -0500, Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
>> On Mon, Feb 11, 2008 at 03:51:59PM -0500, Aleksandar Ristovski wrote:
>>> Daniel Jacobowitz wrote:
>>>> On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
>>>>> The attached patch checks if partial_symbol has already been added 
>>>>> to the list instead of adding duplicate records.
>>>> How does this ever happen?  It seems very wrong.  Also, I am worried
>>>> that the linear search will be a bottleneck (this is quadratic as each
>>>> psymtab grows).
>>> Yes, I understand your concern about the complexity... but...
>>
>> That's only part of the problem.  You have this huge duplication of
>> identical partial symbols within the same block.  How did that happen?
>> It shouldn't.  Maybe we can avoid creating them in the first place.
>>
> Daniel, could you clarify: when you say "maybe we can avoid..." who is 
> "we" - gdb or gcc?

Probably GDB, but I don't know.  Can you show me an example of these
unnecessary psymbols?

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 04:41:08PM -0500, Aleksandar Ristovski wrote:
>> Daniel Jacobowitz wrote:
>>> On Mon, Feb 11, 2008 at 03:51:59PM -0500, Aleksandar Ristovski wrote:
>>>> Daniel Jacobowitz wrote:
>>>>> On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
>>>>>> The attached patch checks if partial_symbol has already been added 
>>>>>> to the list instead of adding duplicate records.
>>>>> How does this ever happen?  It seems very wrong.  Also, I am worried
>>>>> that the linear search will be a bottleneck (this is quadratic as each
>>>>> psymtab grows).
>>>> Yes, I understand your concern about the complexity... but...
>>> That's only part of the problem.  You have this huge duplication of
>>> identical partial symbols within the same block.  How did that happen?
>>> It shouldn't.  Maybe we can avoid creating them in the first place.
>>>
>> Daniel, could you clarify: when you say "maybe we can avoid..." who is 
>> "we" - gdb or gcc?
> 
> Probably GDB, but I don't know.  Can you show me an example of these
> unnecessary psymbols?
> 
Some of them:
unsigned int
_GCC_ATTR_ALIGN_64t
long long int
_GCC_ATTR_ALIGN_u64t
long long unsigned int
_Int64t
_Uint64t
_GCC_ATTR_ALIGN_u32t
unsigned int
_GCC_ATTR_ALIGN_32t
int
_Uint32t
_Int32t
_GCC_ATTR_ALIGN_16t
short int
_GCC_ATTR_ALIGN_u16t
short unsigned int
_Int16t
_Uint16t
_GCC_ATTR_ALIGN_8t
signed char
_GCC_ATTR_ALIGN_u8t
unsigned char
_Int8t
_Uint8t
_Intptrt
_Uintptrt
_Longlong
_ULonglong

Additionally, please take a look at the modified patch, I have added 
bcache_added function to return whether it added the data or returned cached 
one. This way linear search is avoided.


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

Index: gdb/bcache.h
===================================================================
RCS file: /cvs/src/src/gdb/bcache.h,v
retrieving revision 1.12
diff -u -p -r1.12 bcache.h
--- gdb/bcache.h	1 Jan 2008 22:53:09 -0000	1.12
+++ gdb/bcache.h	11 Feb 2008 21:55:41 -0000
@@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo
 extern const void *bcache (const void *addr, int length,
 			   struct bcache *bcache);
 
+extern void *bcache_added (const void *addr, int length,
+				 struct bcache *bcache, int *added);
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
 
Index: gdb/bcache.c
===================================================================
RCS file: /cvs/src/src/gdb/bcache.c,v
retrieving revision 1.20
diff -u -p -r1.20 bcache.c
--- gdb/bcache.c	1 Jan 2008 22:53:09 -0000	1.20
+++ gdb/bcache.c	11 Feb 2008 21:55:41 -0000
@@ -197,11 +197,34 @@ expand_hash_table (struct bcache *bcache
 static void *
 bcache_data (const void *addr, int length, struct bcache *bcache)
 {
+  return bcache_added (addr, length, bcache, NULL);
+}
+
+
+void *
+deprecated_bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+const void *
+bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+void *
+bcache_added (const void *addr, int length, struct bcache *bcache, 
+		   int *added)
+{
   unsigned long full_hash;
   unsigned short half_hash;
   int hash_index;
   struct bstring *s;
 
+  if (added)
+    *added = 0;
+
   /* If our average chain length is too high, expand the hash table.  */
   if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
     expand_hash_table (bcache);
@@ -242,21 +265,12 @@ bcache_data (const void *addr, int lengt
     bcache->unique_size += length;
     bcache->structure_size += BSTRING_SIZE (length);
 
+    if (added)
+      *added = 1;
+
     return &new->d.data;
   }
 }
-
-void *
-deprecated_bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
-
-const void *
-bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
 \f
 /* Allocating and freeing bcaches.  */
 
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.198
diff -u -p -r1.198 symfile.c
--- gdb/symfile.c	29 Jan 2008 22:47:20 -0000	1.198
+++ gdb/symfile.c	11 Feb 2008 21:55:46 -0000
@@ -3102,15 +3103,20 @@ add_psymbol_to_list (char *name, int nam
 		     enum language language, struct objfile *objfile)
 {
   struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
+  int added;
+  char *buf = name;  
   /* psymbol is static so that there will be no uninitialized gaps in the
      structure which might contain random data, causing cache misses in
      bcache. */
   static struct partial_symbol psymbol;
-
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -3128,8 +3134,11 @@ add_psymbol_to_list (char *name, int nam
   SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
 
   /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+  psym = bcache_added (&psymbol, sizeof (struct partial_symbol),
+			    objfile->psymbol_cache, &added);
+
+  if (!added)
+    return psym;
 
   /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
   if (list->next >= list->list + list->size)

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

[Daniel Jacobowitz]

On Mon, Feb 11, 2008 at 05:10:14PM -0500, Aleksandar Ristovski wrote:
> unsigned int
> _GCC_ATTR_ALIGN_64t
> long long int
> _GCC_ATTR_ALIGN_u64t
> long long unsigned int
> _Int64t

OK, I remember what's going on now.  I think you've missed something
about these lists.

Suppose we've got two C files combined into one objfile by the linker.
One has "typedef long long _Int64t" and the other has "typedef struct
{ int hi, int lo; } _Int64t".  GDB records that type as present in
both files.  All the partial symbol says is that there's a typedef
named _Int64t; it doesn't say what it's typedef'd to.  Because it's
a typedef, the DWARF-2 reader adds it to the list of file-scope
symbols.

There's a single list of file-scope partial symbols for the whole
objfile, objfile->static_psymbols.  Each psymtab has an offset
(statics_offset) saying where in that file-wide list of symbols to
look for static symbols for this particular file.  So it's a single
"list", but it contains many individual lists.  That's why there's
a bcache; many files may have identical psymbols, but the psymbol
belongs to each individual file.

Your patch leaves the types out of every file after the first.  I
believe it will cause GDB to fail to locate the correct type or
enumerator.  Functions will be unaffected, since the psymbol includes
the address, so there won't be duplicates anyway.  We don't know
at this stage of processing if the typedef, struct, or enumerator
in the new file has the same value as the one in the previous file;
we don't have values until we read in full symbols.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 05:10:14PM -0500, Aleksandar Ristovski wrote:
>> unsigned int
>> _GCC_ATTR_ALIGN_64t
>> long long int
>> _GCC_ATTR_ALIGN_u64t
>> long long unsigned int
>> _Int64t
> 
> OK, I remember what's going on now.  I think you've missed something
> about these lists.
> 
> Suppose we've got two C files combined into one objfile by the linker.
> One has "typedef long long _Int64t" and the other has "typedef struct
> { int hi, int lo; } _Int64t".  GDB records that type as present in
> both files.  All the partial symbol says is that there's a typedef
> named _Int64t; it doesn't say what it's typedef'd to.  Because it's
> a typedef, the DWARF-2 reader adds it to the list of file-scope
> symbols.
> 
> There's a single list of file-scope partial symbols for the whole
> objfile, objfile->static_psymbols.  Each psymtab has an offset
> (statics_offset) saying where in that file-wide list of symbols to
> look for static symbols for this particular file.  So it's a single
> "list", but it contains many individual lists.  That's why there's
> a bcache; many files may have identical psymbols, but the psymbol
> belongs to each individual file.
> 
> Your patch leaves the types out of every file after the first.  I
> believe it will cause GDB to fail to locate the correct type or
> enumerator.  Functions will be unaffected, since the psymbol includes
> the address, so there won't be duplicates anyway.  We don't know
> at this stage of processing if the typedef, struct, or enumerator
> in the new file has the same value as the one in the previous file;
> we don't have values until we read in full symbols.
> 

If one compilation unit has a list of symbols and they appear more than once... 
do we really need to have all duplicate records in partial symbols list? The 
partial symbol lookup (by symbol name) will find only the first one matching and 
probably cause loading the full symbols at which point all works as before.

Not sure if it is a valid indicator but I didn't have any regressions in 'make 
check'.

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

[Daniel Jacobowitz]

On Mon, Feb 11, 2008 at 05:43:22PM -0500, Aleksandar Ristovski wrote:
> If one compilation unit has a list of symbols and they appear more than 
> once... do we really need to have all duplicate records in partial symbols 
> list? The partial symbol lookup (by symbol name) will find only the first 
> one matching and probably cause loading the full symbols at which point 
> all works as before.

Sure.  But your patches aren't checking one compilation unit, they're
checking every compilation unit in the objfile at once.  That's why
they found duplicates.  I don't think any one compilation unit will
have a duplicate.

> Not sure if it is a valid indicator but I didn't have any regressions in 
> 'make check'.

Yeah, I think I could write some testcases that were affected by this,
but I'm not sure.  It's tricky because if something else causes the
full symtab to load, the problem won't appear.

Maybe there's some way we can avoid needing psymbols for types at all.
But I think we need to have a real design and some documentation for
it instead of just accidentally omitting them.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 05:43:22PM -0500, Aleksandar Ristovski wrote:
>> If one compilation unit has a list of symbols and they appear more than 
>> once... do we really need to have all duplicate records in partial symbols 
>> list? The partial symbol lookup (by symbol name) will find only the first 
>> one matching and probably cause loading the full symbols at which point 
>> all works as before.
> 
> Sure.  But your patches aren't checking one compilation unit, they're
> checking every compilation unit in the objfile at once.  That's why
> they found duplicates.  I don't think any one compilation unit will
> have a duplicate.
Then I am definitely missing something big. Please correct me where I'm wrong.

struct dwarf2_cu has a pointer to struct objfile, which (if I'm not mistaken) 
get's allocated per compilation unit.

Further, struct objfile contains pointer to struct bcache: psymbol_cache which 
gets initialized in objfile initialization (allocate_objfile function). 
Therefore, there should be exactly one psymbol_cache per objfile.

bcache_data hashes objects in psymbol_cache, that is, it will hash only partial 
symbols of that particular compilation unit.

There are two pointers to struct psymbol_allocation_list, both declared in 
struct objfile: global_psymbols and static_psymbols. Therefore, there should be 
exactly one of each per each compilation unit.

In dwarf2read.c we determine whether a symbol belongs to the global or static 
psymbol list and we pass this list along with the objfile pointer to 
add_psymbol_to_list function.

Now we get to the point where I made the change:
The first patch I submitted allows for having a psymbol listed in each list, but 
only once (as opposed to many times before). The second patch I submitted will 
prevent adding a partial symbol to both lists.

What am I missing?

> 
>> Not sure if it is a valid indicator but I didn't have any regressions in 
>> 'make check'.
> 
> Yeah, I think I could write some testcases that were affected by this,
> but I'm not sure.  It's tricky because if something else causes the
> full symtab to load, the problem won't appear.

Yes, that is the tricky part. We want lookup by symbol name to occur in order to 
test this. Maybe adding a maintenance command that would explicitly lookup a 
partial symbol by name?

> 
> Maybe there's some way we can avoid needing psymbols for types at all.
> But I think we need to have a real design and some documentation for
> it instead of just accidentally omitting them.
> 

Not sure how would that work. But I think it would make sense to have only one 
global_psymbols list.

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

[Daniel Jacobowitz]

On Mon, Feb 11, 2008 at 08:07:55PM -0500, Aleksandar Ristovski wrote:
> struct dwarf2_cu has a pointer to struct objfile, which (if I'm not 
> mistaken) get's allocated per compilation unit.

That's where your confusion is.  An objfile is something like
"/lib/libc.so.6", a linked file.  A compilation unit is something
like "init.o", a single "gcc -c" output.

Look at how n_static_syms is set in dwarf2read.c to see how
multiple psymtabs, for different CUs, share the same static_psymbols
list.

>>> Not sure if it is a valid indicator but I didn't have any regressions 
>>> in 'make check'.
>>
>> Yeah, I think I could write some testcases that were affected by this,
>> but I'm not sure.  It's tricky because if something else causes the
>> full symtab to load, the problem won't appear.
>
> Yes, that is the tricky part. We want lookup by symbol name to occur in 
> order to test this. Maybe adding a maintenance command that would 
> explicitly lookup a partial symbol by name?

The trick for writing testcases is to not stop in the second file or
"list" it or set a breakpoint in it; any of those things will load the
symtab.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 08:07:55PM -0500, Aleksandar Ristovski wrote:
>> struct dwarf2_cu has a pointer to struct objfile, which (if I'm not 
>> mistaken) get's allocated per compilation unit.
> 
> That's where your confusion is.  An objfile is something like
> "/lib/libc.so.6", a linked file.  A compilation unit is something
> like "init.o", a single "gcc -c" output.

Ahhh.... Thank you. I did get confused with the name.

However, I think the patch is still good.

What happens with the patch is that for an objfile, we will add global type 
information only once, in the first partial symbol table where the symbol was 
encountered. I think this will be fine. Type info will not have address 
associated and all information we can get about it will be there. Finding the 
first matching partial symbol for a type symbol is as good as finding the second 
or N-th partial symbol for that type (and domain).

On the other hand, for anything that is linked, i.e. has an address, the 
partial_symbol must be unique (linker should make sure of that: mangled name, 
etc...) and my patch will not affect adding such symbols.

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

[Daniel Jacobowitz]

On Tue, Feb 12, 2008 at 12:34:49AM -0500, Aleksandar Ristovski wrote:
> What happens with the patch is that for an objfile, we will add global 
> type information only once, in the first partial symbol table where the 
> symbol was encountered. I think this will be fine. Type info will not have 
> address associated and all information we can get about it will be there. 
> Finding the first matching partial symbol for a type symbol is as good as 
> finding the second or N-th partial symbol for that type (and domain).

Types are not (in C) global.  They're file-static.  We need to know
which types are present in each file, because they may have different
definitions in each file.

The psymbols for two types with the same name will be the same.  But
the full symbols may be different.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Tue, Feb 12, 2008 at 12:34:49AM -0500, Aleksandar Ristovski wrote:
>> What happens with the patch is that for an objfile, we will add global 
>> type information only once, in the first partial symbol table where the 
>> symbol was encountered. I think this will be fine. Type info will not have 
>> address associated and all information we can get about it will be there. 
>> Finding the first matching partial symbol for a type symbol is as good as 
>> finding the second or N-th partial symbol for that type (and domain).
> 
> Types are not (in C) global.  They're file-static.  We need to know
> which types are present in each file, because they may have different
> definitions in each file.
> 
> The psymbols for two types with the same name will be the same.  But
> the full symbols may be different.
> 
Yes, you are correct. The patch should not filter out duplicate static symbols, 
they should stay. Something like this:

(in add_psymbol_to_list)

   /* Filter out duplicate global symbols for c++ only.  */
   if (language == language_cplus
       && list == objfile->global_psymbols
       && !added)
     return psym;

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

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> 
> Maybe there's some way we can avoid needing psymbols for types at all.
> But I think we need to have a real design and some documentation for
> it instead of just accidentally omitting them.
> 
Here is a revised patch I am suggesting. It does not completely eliminate the
need for psymbols for types, but does significantly reduce number of global type
symbols. I tried to explain in detail why is it possible and safe. Here is the 
explanation:

First the facts about the partial symbols:

1. add_partial_symbol
This function gets called during reading the partial symbol information to add a
partial_symbol to either global_list or static_list declared in struct objfile.
The following table lists partial symbol types and scope.

DW_TAG_		 Lang		Scope (DOMAIN,LOC)
subprogram	 Ada		global (VAR,BLOCK)
		 All other	global if ext or static	(VAR,BLOCK)
variable	 All		global if ext or static (VAR,STATIC)
typedef
base_type
subrange_type	 All		static (VAR,TYPEDEF)
namespace	 All		global (VAR,TYPEDEF)
class_type
interface_type
union_type
enumeration_type cplus,java	global (STRUCT,TYPEDEF)
		 cplus,java,ada	again to global (VAR,TYPEDEF)
enumerator	 cplus,java	global (VAR,CONST)
		 All other	static (VAR,CONST)

Note that psymbol types namespace, class_type, interface_type, union_type,
enumeration_type and enumerator, for c++ and java, always go to global list and
will always have address 0 (anything else wouldn't make sense).

2. add_psymbol_to_list
This function adds partial symbol first to bcache (objfile->psymbol_cache) and
then to objfile->global_psymbols or objfile->static_psymbols list, depending on
which one was passed in.
It gets called by add_partial_symbol function.

In this function we will add partial symbol to one of the lists; the partial
symbols will always have exactly the same order in which they were added. This
fact will be used when building partial symbol table which will use this list
and with offset and number of symbols determine which partial symbols "belong"
to it. The reason why partial symbol table needs to know about a symbol is to be
able to eventually load full symbols.

3. lookup_partial_symbol
Function looks up global or static list of objfile, but only subset of it
determined by partial symtab's offset and number of symbols. Returns partial
symbol found. This function is always called from a loop through all partial
symtabs (this fact is important).


When is a partial symbol really matched:

Now let's look at the partial symbols which are added only to global partial
symbol list: namespace, class_type, interface_type, union_type,
enumeration_type, enumerator. The partial symbol will contain names and no
address (it will be 0). Therefore, lookup_partial_symbol will always return the
first partial symbol table where the psymbol was found. Once the psymbol was
found, most probably full symbols will be loaded; once full symbols are loaded,
all subsequent lookups for a symbol will first find it in full symtab and will
never look for it in the partial symtab again. As a result, all but the first
addition of such partial symbol will never be found and are therefore redundant
and only take up time to qsort partial symbols within a partial symbol table.


The patch:

This revised patch adds global symbols only once to first partial symbol table 
where the symbol was encountered, but only for namespace,
class_type, interface_type, union_type, enumeration_type and enumerator partial
symbol types (for the last two, only for java and cplus).


2008-02-12  Aleksandar Ristovski  <aristovski@qnx.com>

	* bcache.c (bcache_data): Call bcache_added function.
	(bcache_added): New function name. Body of function bcache_data
	is used here with the addition of 'added' argument. The argument
	is optional and if specified, bcache_added returns 1 if the data
	was added and 0 if it was found in the hash.
	* bcache.h (bcache_added): New function.
	* dwarf2read.c (add_partial_symbol): Make call to
	new function add_psymbol_to_global_list for namespace, class_type,
	interface_type, union_type, enumeration_type and enumerator partial
	symbol types.
	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
	work from add_psymbol_to_list - initialises partial symbol and stashes
	it in objfile's cache.
	(append_psymbol_to_list): New helper function, takes other part of
	work from add_psymbol_to_list - adds partial symbol to the given list.
	(add_psymbol_to_list): Call helper functions instead of doing work
	here. Functionally, the function hasn't changed.
	(add_psymbol_to_global_list): New function, adds partial symbol to
	global list and does it only once per objfile.
	* symfile.h (add_psymbol_to_global_list): New function.


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

Index: gdb/bcache.c
===================================================================
RCS file: /cvs/src/src/gdb/bcache.c,v
retrieving revision 1.20
diff -u -p -r1.20 bcache.c
--- gdb/bcache.c	1 Jan 2008 22:53:09 -0000	1.20
+++ gdb/bcache.c	13 Feb 2008 04:24:37 -0000
@@ -197,11 +197,34 @@ expand_hash_table (struct bcache *bcache
 static void *
 bcache_data (const void *addr, int length, struct bcache *bcache)
 {
+  return bcache_added (addr, length, bcache, NULL);
+}
+
+
+void *
+deprecated_bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+const void *
+bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+void *
+bcache_added (const void *addr, int length, struct bcache *bcache, 
+		   int *added)
+{
   unsigned long full_hash;
   unsigned short half_hash;
   int hash_index;
   struct bstring *s;
 
+  if (added)
+    *added = 0;
+
   /* If our average chain length is too high, expand the hash table.  */
   if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
     expand_hash_table (bcache);
@@ -242,21 +265,12 @@ bcache_data (const void *addr, int lengt
     bcache->unique_size += length;
     bcache->structure_size += BSTRING_SIZE (length);
 
+    if (added)
+      *added = 1;
+
     return &new->d.data;
   }
 }
-
-void *
-deprecated_bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
-
-const void *
-bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
 \f
 /* Allocating and freeing bcaches.  */
 
Index: gdb/bcache.h
===================================================================
RCS file: /cvs/src/src/gdb/bcache.h,v
retrieving revision 1.12
diff -u -p -r1.12 bcache.h
--- gdb/bcache.h	1 Jan 2008 22:53:09 -0000	1.12
+++ gdb/bcache.h	13 Feb 2008 04:24:37 -0000
@@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo
 extern const void *bcache (const void *addr, int length,
 			   struct bcache *bcache);
 
+extern void *bcache_added (const void *addr, int length,
+				 struct bcache *bcache, int *added);
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
 
Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.251
diff -u -p -r1.251 dwarf2read.c
--- gdb/dwarf2read.c	1 Feb 2008 22:45:13 -0000	1.251
+++ gdb/dwarf2read.c	13 Feb 2008 04:24:40 -0000
@@ -1976,9 +1976,8 @@ add_partial_symbol (struct partial_die_i
 			   0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_namespace:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
+      add_psymbol_to_global_list (actual_name, strlen (actual_name),
 			   VAR_DOMAIN, LOC_TYPEDEF,
-			   &objfile->global_psymbols,
 			   0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_class_type:
@@ -2000,32 +1999,37 @@ add_partial_symbol (struct partial_die_i
 
       /* NOTE: carlton/2003-10-07: See comment in new_symbol about
 	 static vs. global.  */
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   STRUCT_DOMAIN, LOC_TYPEDEF,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      if (cu->language == language_cplus
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen(actual_name),
+				    STRUCT_DOMAIN, LOC_TYPEDEF,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     STRUCT_DOMAIN, LOC_TYPEDEF,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
 
       if (cu->language == language_cplus
           || cu->language == language_java
           || cu->language == language_ada)
 	{
 	  /* For C++ and Java, these implicitly act as typedefs as well. */
-	  add_psymbol_to_list (actual_name, strlen (actual_name),
-			       VAR_DOMAIN, LOC_TYPEDEF,
-			       &objfile->global_psymbols,
-			       0, (CORE_ADDR) 0, cu->language, objfile);
+	  add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				      VAR_DOMAIN, LOC_TYPEDEF,
+				      0, (CORE_ADDR) 0, cu->language, objfile);
 	}
       break;
     case DW_TAG_enumerator:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
+      if (cu->language == language_cplus
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    VAR_DOMAIN, LOC_CONST,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+        else
+	  add_psymbol_to_list (actual_name, strlen (actual_name),
 			   VAR_DOMAIN, LOC_CONST,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
+			   &objfile->static_psymbols,
 			   0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     default:
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.198
diff -u -p -r1.198 symfile.c
--- gdb/symfile.c	29 Jan 2008 22:47:20 -0000	1.198
+++ gdb/symfile.c	13 Feb 2008 04:24:41 -0000
@@ -3079,38 +3079,27 @@ start_psymtab_common (struct objfile *ob
   return (psymtab);
 }
 \f
-/* Add a symbol with a long value to a psymtab.
-   Since one arg is a struct, we pass in a ptr and deref it (sigh).
-   Return the partial symbol that has been added.  */
-
-/* NOTE: carlton/2003-09-11: The reason why we return the partial
-   symbol is so that callers can get access to the symbol's demangled
-   name, which they don't have any cheap way to determine otherwise.
-   (Currenly, dwarf2read.c is the only file who uses that information,
-   though it's possible that other readers might in the future.)
-   Elena wasn't thrilled about that, and I don't blame her, but we
-   couldn't come up with a better way to get that information.  If
-   it's needed in other situations, we could consider breaking up
-   SYMBOL_SET_NAMES to provide access to the demangled name lookup
-   cache.  */
-
-const struct partial_symbol *
-add_psymbol_to_list (char *name, int namelength, domain_enum domain,
-		     enum address_class class,
-		     struct psymbol_allocation_list *list, long val,	/* Value as a long */
-		     CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
-		     enum language language, struct objfile *objfile)
+static struct partial_symbol *
+add_psymbol_to_bcache (char *name, int namelength, domain_enum domain,
+		       enum address_class class,
+		       long val,	/* Value as a long */
+		       CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		       enum language language, struct objfile *objfile,
+		       int *added)
 {
-  struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
+  char *buf = name;  
   /* psymbol is static so that there will be no uninitialized gaps in the
      structure which might contain random data, causing cache misses in
      bcache. */
   static struct partial_symbol psymbol;
-
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -3128,17 +3117,78 @@ add_psymbol_to_list (char *name, int nam
   SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
 
   /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+  return bcache_added (&psymbol, sizeof (struct partial_symbol),
+			    objfile->psymbol_cache, added);
+}
 
-  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
-  if (list->next >= list->list + list->size)
+static void
+append_psymbol_to_list (struct psymbol_allocation_list *list,
+			struct partial_symbol *psym,
+			struct objfile *objfile)
+{
+if (list->next >= list->list + list->size)
     {
       extend_psymbol_list (list, objfile);
     }
   *list->next++ = psym;
   OBJSTAT (objfile, n_psyms++);
+}
+
+/* Add a symbol with a long value to a psymtab.
+   Since one arg is a struct, we pass in a ptr and deref it (sigh).
+   Return the partial symbol that has been added.  */
 
+/* NOTE: carlton/2003-09-11: The reason why we return the partial
+   symbol is so that callers can get access to the symbol's demangled
+   name, which they don't have any cheap way to determine otherwise.
+   (Currenly, dwarf2read.c is the only file who uses that information,
+   though it's possible that other readers might in the future.)
+   Elena wasn't thrilled about that, and I don't blame her, but we
+   couldn't come up with a better way to get that information.  If
+   it's needed in other situations, we could consider breaking up
+   SYMBOL_SET_NAMES to provide access to the demangled name lookup
+   cache.  */
+
+const struct partial_symbol *
+add_psymbol_to_list (char *name, int namelength, domain_enum domain,
+		     enum address_class class,
+		     struct psymbol_allocation_list *list, long val,	/* Value as a long */
+		     CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		     enum language language, struct objfile *objfile)
+{
+  struct partial_symbol *psym;
+  /* Stash the partial symbol away in the cache */
+  psym = add_psymbol_to_bcache (name, namelength, domain, class,
+				val, coreaddr, language, objfile, NULL);
+
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (list, psym, objfile);
+  return psym;
+}
+
+/* This function should be called only for those types (and for supporting
+   languages) that do not need duplicate global type information. For C++,
+   and java these partial symbol types are: 
+   namespace, class_type, interface_type.  */
+
+const struct partial_symbol *
+add_psymbol_to_global_list (char *name, int namelength, domain_enum domain,
+		     enum address_class class,
+		     long val,	/* Value as a long */
+		     CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		     enum language language, struct objfile *objfile)
+{
+  struct partial_symbol *psym;
+  int added;
+  
+  psym = add_psymbol_to_bcache (name, namelength, domain, class, val,
+			        coreaddr, language, objfile, &added);
+
+  if (!added)
+    return psym;
+
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (&objfile->global_psymbols, psym, objfile);
   return psym;
 }
 
Index: gdb/symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.46
diff -u -p -r1.46 symfile.h
--- gdb/symfile.h	3 Feb 2008 22:13:29 -0000	1.46
+++ gdb/symfile.h	13 Feb 2008 04:24:41 -0000
@@ -199,6 +199,12 @@ struct partial_symbol *add_psymbol_to_li
 					    long, CORE_ADDR,
 					    enum language, struct objfile *);
 
+extern const
+struct partial_symbol *add_psymbol_to_global_list (char *, int, domain_enum,
+					    enum address_class,
+					    long, CORE_ADDR,
+					    enum language, struct objfile *);
+
 extern void init_psymbol_list (struct objfile *, int);
 
 extern void sort_pst_symbols (struct partial_symtab *);

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

[Aleksandar Ristovski]

Aleksandar Ristovski wrote:
> 
> The patch:
> 
> This revised patch adds global symbols only once to first partial symbol table 
> where the symbol was encountered, but only for namespace,
> class_type, interface_type, union_type, enumeration_type and enumerator partial
> symbol types (for the last two, only for java and cplus).
> 

Here are some timings with a real-life binary. The time to load symbols for 
unpatched (gdb from current HEAD) is around 70 seconds, while the patched gdb 
needed less than 40 seconds. Almost 50% less for this particular binary which is 
built from c++ source using stl heavily.

I have run tests (make check) and saw no regression; I also did other testing 
(many debugging sessions) and haven't seen anything broken, but if you can come 
up with a test case that breaks after the patch, I would be very interested to see.

Note that the patch does not change anything about static partial symbols, only 
global psyms.


$ cat ../nopatch.log
GNU gdb 6.7.50.20080214-cvs
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
Command execution time: 0.000000
Space used: 135168 (+0 for this command)
+sym /tmp/at/libalib.so
Command execution time: 69.076317
Space used: 36360192 (+36225024 for this command)
+quit

$ cat ../withpatch.log
GNU gdb 6.7.50.20080213-cvs
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-linux-gnu".
Command execution time: 0.000000
Space used: 135168 (+0 for this command)
+sym /tmp/at/libalib.so
Command execution time: 32.830052
Space used: 36319232 (+36184064 for this command)
+quit

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

[Aleksandar Ristovski]

Aleksandar Ristovski wrote:
> Aleksandar Ristovski wrote:
>>
>> The patch:
>>
>> This revised patch adds global symbols only once to first partial 
>> symbol table where the symbol was encountered, but only for namespace,
>> class_type, interface_type, union_type, enumeration_type and 
>> enumerator partial
>> symbol types (for the last two, only for java and cplus).
>>
> 

Just a reminder... (a.k.a. ping).


Thanks,

Aleksandar

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

[Daniel Jacobowitz]

On Wed, Feb 13, 2008 at 12:22:57AM -0500, Aleksandar Ristovski wrote:
> 3. lookup_partial_symbol
> Function looks up global or static list of objfile, but only subset of it
> determined by partial symtab's offset and number of symbols. Returns partial
> symbol found. This function is always called from a loop through all partial
> symtabs (this fact is important).

Ah!  That's the part I didn't understand before.

I was worried about the construct tested in scope.exp, "print
'scope0.c'::filelocal".  But it uses full symtabs, not partial
symtabs.

> 2008-02-12  Aleksandar Ristovski  <aristovski@qnx.com>
>
> 	* bcache.c (bcache_data): Call bcache_added function.
> 	(bcache_added): New function name. Body of function bcache_data
> 	is used here with the addition of 'added' argument. The argument
> 	is optional and if specified, bcache_added returns 1 if the data
> 	was added and 0 if it was found in the hash.

Whenever you find yourself writing this sort of explanation in the
changelog, it's a sign that you should have a shorter changelog entry
and a long comment in the source code.  Please move the explanation to
bcache.h.

> 	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
> 	work from add_psymbol_to_list - initialises partial symbol and stashes
> 	it in objfile's cache.
> 	(append_psymbol_to_list): New helper function, takes other part of
> 	work from add_psymbol_to_list - adds partial symbol to the given list.
> 	(add_psymbol_to_list): Call helper functions instead of doing work
> 	here. Functionally, the function hasn't changed.
> 	(add_psymbol_to_global_list): New function, adds partial symbol to
> 	global list and does it only once per objfile.

Same thing for these.

> +const void *
> +bcache (const void *addr, int length, struct bcache *bcache)
> +{
> +  return bcache_data (addr, length, bcache);
> +}
> +
> +void *
> +bcache_added (const void *addr, int length, struct bcache *bcache, 
> +		   int *added)

It should return a const pointer, like bcache.  Also the indentation
on the second line is too deep.

>      case DW_TAG_namespace:
> -      add_psymbol_to_list (actual_name, strlen (actual_name),
> +      add_psymbol_to_global_list (actual_name, strlen (actual_name),
>  			   VAR_DOMAIN, LOC_TYPEDEF,
> -			   &objfile->global_psymbols,
>  			   0, (CORE_ADDR) 0, cu->language, objfile);

Please adjust indentation on the subsequent lines, since the open
paren moved over.

>        if (cu->language == language_cplus
>            || cu->language == language_java
>            || cu->language == language_ada)
>  	{
>  	  /* For C++ and Java, these implicitly act as typedefs as well. */
> -	  add_psymbol_to_list (actual_name, strlen (actual_name),
> -			       VAR_DOMAIN, LOC_TYPEDEF,
> -			       &objfile->global_psymbols,
> -			       0, (CORE_ADDR) 0, cu->language, objfile);
> +	  add_psymbol_to_global_list (actual_name, strlen (actual_name),
> +				      VAR_DOMAIN, LOC_TYPEDEF,
> +				      0, (CORE_ADDR) 0, cu->language, objfile);
>  	}

You've deleted this bit in your other patch fortunately :-)

>      case DW_TAG_enumerator:
> -      add_psymbol_to_list (actual_name, strlen (actual_name),
> +      if (cu->language == language_cplus
> +	  || cu->language == language_java)
> +	add_psymbol_to_global_list (actual_name, strlen (actual_name),
> +				    VAR_DOMAIN, LOC_CONST,
> +				    0, (CORE_ADDR) 0, cu->language, objfile);

This part I don't understand.  Why does the behavior of enumerators
depend on the language?  The logic for finding them during lookup is
the same.

In fact, if lookup_partial_symbol will never find a global symbol
with an identical name why do we need them for any duplicated symbol,
even legitimately duplicated things like static functions?  Well,
the check in cp-support.c is one reason for functions.  And I can
imagine other valid reasons in the future.  So functions are special.
But for things other than functions.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
>> 	* bcache.c (bcache_data): Call bcache_added function.
>> 	(bcache_added): New function name. Body of function bcache_data
>> 	is used here with the addition of 'added' argument. The argument
>> 	is optional and if specified, bcache_added returns 1 if the data
>> 	was added and 0 if it was found in the hash.
> 
> Whenever you find yourself writing this sort of explanation in the
> changelog, it's a sign that you should have a shorter changelog entry
> and a long comment in the source code.  Please move the explanation to
> bcache.h.

Comments added.

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

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

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

Done.

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

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

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

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

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


Thanks,
Aleksandar Ristovski
QNX Software Systems



ChangeLog:

	* bcache.c (bcache_data): Call bcache_added function.
	(bcache_added): New function name. Body of function bcache_data
	is used here with the addition of 'added' argument. 
	* bcache.h (bcache_added): New function.
	* dwarf2read.c (add_partial_symbol): Make call to 
	new function add_psymbol_to_global_list for partial symbols that
	are being added to the global list.
	(load_partial_dies): Likewise.
	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
	work from add_psymbol_to_list - initialises partial symbol and stashes
	it in objfile's cache.
	(append_psymbol_to_list): New helper function, takes other part of 
	work from add_psymbol_to_list - adds partial symbol to the given list.
	(add_psymbol_to_list): Call helper functions instead of doing work 
	here. Functionally, the function hasn't changed.
	(add_psymbol_to_global_list): New function, adds partial symbol to
	global list and does it only once per objfile.
	* symfile.h (add_psymbol_to_global_list): New function.

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

Index: gdb/bcache.c
===================================================================
RCS file: /cvs/src/src/gdb/bcache.c,v
retrieving revision 1.20
diff -u -p -r1.20 bcache.c
--- gdb/bcache.c	1 Jan 2008 22:53:09 -0000	1.20
+++ gdb/bcache.c	5 May 2008 18:49:56 -0000
@@ -197,11 +197,40 @@ expand_hash_table (struct bcache *bcache
 static void *
 bcache_data (const void *addr, int length, struct bcache *bcache)
 {
+  return bcache_added (addr, length, bcache, NULL);
+}
+
+
+void *
+deprecated_bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+const void *
+bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+/* Find a copy of the LENGTH bytes at ADDR in BCACHE.  If BCACHE has
+   never seen those bytes before, add a copy of them to BCACHE.  In
+   either case, return a pointer to BCACHE's copy of that string.  If
+   optional ADDED is not NULL, return 1 in case of new entry or 0 if
+   returning an old entry.  */
+
+void *
+bcache_added (const void *addr, int length, struct bcache *bcache, 
+		   int *added)
+{
   unsigned long full_hash;
   unsigned short half_hash;
   int hash_index;
   struct bstring *s;
 
+  if (added)
+    *added = 0;
+
   /* If our average chain length is too high, expand the hash table.  */
   if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
     expand_hash_table (bcache);
@@ -242,21 +271,12 @@ bcache_data (const void *addr, int lengt
     bcache->unique_size += length;
     bcache->structure_size += BSTRING_SIZE (length);
 
+    if (added)
+      *added = 1;
+
     return &new->d.data;
   }
 }
-
-void *
-deprecated_bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
-
-const void *
-bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
 \f
 /* Allocating and freeing bcaches.  */
 
Index: gdb/bcache.h
===================================================================
RCS file: /cvs/src/src/gdb/bcache.h,v
retrieving revision 1.12
diff -u -p -r1.12 bcache.h
--- gdb/bcache.h	1 Jan 2008 22:53:09 -0000	1.12
+++ gdb/bcache.h	5 May 2008 18:49:56 -0000
@@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo
 extern const void *bcache (const void *addr, int length,
 			   struct bcache *bcache);
 
+extern void *bcache_added (const void *addr, int length,
+			   struct bcache *bcache, int *added);
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
 
Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.263
diff -u -p -r1.263 dwarf2read.c
--- gdb/dwarf2read.c	5 May 2008 14:37:32 -0000	1.263
+++ gdb/dwarf2read.c	5 May 2008 18:49:57 -0000
@@ -1964,11 +1964,10 @@ add_partial_symbol (struct partial_die_i
              in the global scope.  */
 	  /*prim_record_minimal_symbol (actual_name, pdi->lowpc + baseaddr,
 	     mst_text, objfile); */
-	  psym = add_psymbol_to_list (actual_name, strlen (actual_name),
-				      VAR_DOMAIN, LOC_BLOCK,
-				      &objfile->global_psymbols,
-				      0, pdi->lowpc + baseaddr,
-				      cu->language, objfile);
+	  psym = add_psymbol_to_global_list (actual_name, strlen (actual_name),
+					     VAR_DOMAIN, LOC_BLOCK,
+					     0, pdi->lowpc + baseaddr,
+					     cu->language, objfile);
 	}
       else
 	{
@@ -2000,11 +1999,11 @@ add_partial_symbol (struct partial_die_i
 	  if (pdi->locdesc)
 	    addr = decode_locdesc (pdi->locdesc, cu);
 	  if (pdi->locdesc || pdi->has_type)
-	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
-					VAR_DOMAIN, LOC_STATIC,
-					&objfile->global_psymbols,
-					0, addr + baseaddr,
-					cu->language, objfile);
+	    psym = add_psymbol_to_global_list (actual_name, 
+					       strlen (actual_name),
+					       VAR_DOMAIN, LOC_STATIC,
+					       0, addr + baseaddr,
+					       cu->language, objfile);
 	}
       else
 	{
@@ -2034,10 +2033,9 @@ add_partial_symbol (struct partial_die_i
 			   0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_namespace:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   VAR_DOMAIN, LOC_TYPEDEF,
-			   &objfile->global_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				  VAR_DOMAIN, LOC_TYPEDEF,
+				  0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_class_type:
     case DW_TAG_interface_type:
@@ -2058,23 +2056,28 @@ add_partial_symbol (struct partial_die_i
 
       /* NOTE: carlton/2003-10-07: See comment in new_symbol about
 	 static vs. global.  */
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   STRUCT_DOMAIN, LOC_TYPEDEF,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
-
+      if (cu->language == language_cplus 
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    STRUCT_DOMAIN, LOC_TYPEDEF,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     STRUCT_DOMAIN, LOC_TYPEDEF,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_enumerator:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   VAR_DOMAIN, LOC_CONST,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      if (cu->language == language_cplus 
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    VAR_DOMAIN, LOC_CONST,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else 
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     VAR_DOMAIN, LOC_CONST,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     default:
       break;
@@ -5638,13 +5641,21 @@ load_partial_dies (bfd *abfd, gdb_byte *
 	  if (part_die->name == NULL)
 	    complaint (&symfile_complaints, _("malformed enumerator DIE ignored"));
 	  else if (building_psymtab)
-	    add_psymbol_to_list (part_die->name, strlen (part_die->name),
-				 VAR_DOMAIN, LOC_CONST,
-				 (cu->language == language_cplus
-				  || cu->language == language_java)
-				 ? &cu->objfile->global_psymbols
-				 : &cu->objfile->static_psymbols,
-				 0, (CORE_ADDR) 0, cu->language, cu->objfile);
+	    {
+	      if (cu->language == language_cplus
+		  || cu->language == language_java)
+		add_psymbol_to_global_list (part_die->name, 
+					    strlen (part_die->name),
+					    VAR_DOMAIN, LOC_CONST,
+					    0, (CORE_ADDR) 0, 
+					    cu->language, cu->objfile);
+	      else
+		add_psymbol_to_list (part_die->name, strlen (part_die->name),
+				     VAR_DOMAIN, LOC_CONST,
+				     &cu->objfile->static_psymbols,
+				     0, (CORE_ADDR) 0, 
+				     cu->language, cu->objfile);
+	    }
 
 	  info_ptr = locate_pdi_sibling (part_die, info_ptr, abfd, cu);
 	  continue;
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.203
diff -u -p -r1.203 symfile.c
--- gdb/symfile.c	5 May 2008 16:13:49 -0000	1.203
+++ gdb/symfile.c	5 May 2008 18:49:57 -0000
@@ -3082,6 +3082,68 @@ start_psymtab_common (struct objfile *ob
   return (psymtab);
 }
 \f
+/* Helper function, initialises partial symbol structure and stashes 
+   it into objfile's bcache.  Note that our caching mechanism will
+   use all fields of struct partial_symbol to determine hash value of the
+   structure.  In other words, having two symbols with the same name but
+   different domain (or address) is possible and correct.  */
+
+static struct partial_symbol *
+add_psymbol_to_bcache (char *name, int namelength, domain_enum domain,
+		       enum address_class class,
+		       long val,	/* Value as a long */
+		       CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		       enum language language, struct objfile *objfile,
+		       int *added)
+{
+  char *buf = name;  
+  /* psymbol is static so that there will be no uninitialized gaps in the
+     structure which might contain random data, causing cache misses in
+     bcache. */
+  static struct partial_symbol psymbol;
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
+  /* val and coreaddr are mutually exclusive, one of them *will* be zero */
+  if (val != 0)
+    {
+      SYMBOL_VALUE (&psymbol) = val;
+    }
+  else
+    {
+      SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
+    }
+  SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_LANGUAGE (&psymbol) = language;
+  PSYMBOL_DOMAIN (&psymbol) = domain;
+  PSYMBOL_CLASS (&psymbol) = class;
+
+  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+
+  /* Stash the partial symbol away in the cache */
+  return bcache_added (&psymbol, sizeof (struct partial_symbol),
+		       objfile->psymbol_cache, added);
+}
+
+/* Helper function, adds partial symbol to the given partial symbol
+   list.  */
+
+static void
+append_psymbol_to_list (struct psymbol_allocation_list *list,
+			struct partial_symbol *psym,
+			struct objfile *objfile)
+{
+  if (list->next >= list->list + list->size)
+    extend_psymbol_list (list, objfile);
+  *list->next++ = psym;
+  OBJSTAT (objfile, n_psyms++);
+}
+
 /* Add a symbol with a long value to a psymtab.
    Since one arg is a struct, we pass in a ptr and deref it (sigh).
    Return the partial symbol that has been added.  */
@@ -3105,43 +3167,37 @@ add_psymbol_to_list (char *name, int nam
 		     enum language language, struct objfile *objfile)
 {
   struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
-  /* psymbol is static so that there will be no uninitialized gaps in the
-     structure which might contain random data, causing cache misses in
-     bcache. */
-  static struct partial_symbol psymbol;
+  /* Stash the partial symbol away in the cache */
+  psym = add_psymbol_to_bcache (name, namelength, domain, class,
+				val, coreaddr, language, objfile, NULL);
 
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
-  /* val and coreaddr are mutually exclusive, one of them *will* be zero */
-  if (val != 0)
-    {
-      SYMBOL_VALUE (&psymbol) = val;
-    }
-  else
-    {
-      SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
-    }
-  SYMBOL_SECTION (&psymbol) = 0;
-  SYMBOL_LANGUAGE (&psymbol) = language;
-  PSYMBOL_DOMAIN (&psymbol) = domain;
-  PSYMBOL_CLASS (&psymbol) = class;
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (list, psym, objfile);
+  return psym;
+}
 
-  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+/* Add partial symbol to OBJFILE's global list of partial symbols. 
+   Make sure we do not duplicate this information.  See add_psymbol_to_bcache
+   for more details.  */
 
-  /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+const struct partial_symbol *
+add_psymbol_to_global_list (char *name, int namelength, domain_enum domain,
+			    enum address_class class,
+			    long val,	/* Value as a long */
+			    CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+			    enum language language, struct objfile *objfile)
+{
+  struct partial_symbol *psym;
+  int added;
+  
+  psym = add_psymbol_to_bcache (name, namelength, domain, class, val,
+			        coreaddr, language, objfile, &added);
 
-  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
-  if (list->next >= list->list + list->size)
-    {
-      extend_psymbol_list (list, objfile);
-    }
-  *list->next++ = psym;
-  OBJSTAT (objfile, n_psyms++);
+  if (!added)
+    return psym;
 
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (&objfile->global_psymbols, psym, objfile);
   return psym;
 }
 
Index: gdb/symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.46
diff -u -p -r1.46 symfile.h
--- gdb/symfile.h	3 Feb 2008 22:13:29 -0000	1.46
+++ gdb/symfile.h	5 May 2008 18:49:57 -0000
@@ -199,6 +199,13 @@ struct partial_symbol *add_psymbol_to_li
 					    long, CORE_ADDR,
 					    enum language, struct objfile *);
 
+extern const
+struct partial_symbol *add_psymbol_to_global_list (char *, int, domain_enum,
+						   enum address_class,
+						   long, CORE_ADDR,
+						   enum language,
+						   struct objfile *);
+
 extern void init_psymbol_list (struct objfile *, int);
 
 extern void sort_pst_symbols (struct partial_symtab *);

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

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
>> 	* bcache.c (bcache_data): Call bcache_added function.
>> 	(bcache_added): New function name. Body of function bcache_data
>> 	is used here with the addition of 'added' argument. The argument
>> 	is optional and if specified, bcache_added returns 1 if the data
>> 	was added and 0 if it was found in the hash.
> 
> Whenever you find yourself writing this sort of explanation in the
> changelog, it's a sign that you should have a shorter changelog entry
> and a long comment in the source code.  Please move the explanation to
> bcache.h.

Comments added.

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

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

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

Done.

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

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

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

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

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


Thanks,
Aleksandar Ristovski
QNX Software Systems



ChangeLog:

	* bcache.c (bcache_data): Call bcache_added function.
	(bcache_added): New function name. Body of function bcache_data
	is used here with the addition of 'added' argument. 
	* bcache.h (bcache_added): New function.
	* dwarf2read.c (add_partial_symbol): Make call to 
	new function add_psymbol_to_global_list for partial symbols that
	are being added to the global list.
	(load_partial_dies): Likewise.
	* symfile.c (add_psymbol_to_bcache): New helper function, takes part of
	work from add_psymbol_to_list - initialises partial symbol and stashes
	it in objfile's cache.
	(append_psymbol_to_list): New helper function, takes other part of 
	work from add_psymbol_to_list - adds partial symbol to the given list.
	(add_psymbol_to_list): Call helper functions instead of doing work 
	here. Functionally, the function hasn't changed.
	(add_psymbol_to_global_list): New function, adds partial symbol to
	global list and does it only once per objfile.
	* symfile.h (add_psymbol_to_global_list): New function.

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

Index: gdb/bcache.c
===================================================================
RCS file: /cvs/src/src/gdb/bcache.c,v
retrieving revision 1.20
diff -u -p -r1.20 bcache.c
--- gdb/bcache.c	1 Jan 2008 22:53:09 -0000	1.20
+++ gdb/bcache.c	5 May 2008 18:49:56 -0000
@@ -197,11 +197,40 @@ expand_hash_table (struct bcache *bcache
 static void *
 bcache_data (const void *addr, int length, struct bcache *bcache)
 {
+  return bcache_added (addr, length, bcache, NULL);
+}
+
+
+void *
+deprecated_bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+const void *
+bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+/* Find a copy of the LENGTH bytes at ADDR in BCACHE.  If BCACHE has
+   never seen those bytes before, add a copy of them to BCACHE.  In
+   either case, return a pointer to BCACHE's copy of that string.  If
+   optional ADDED is not NULL, return 1 in case of new entry or 0 if
+   returning an old entry.  */
+
+void *
+bcache_added (const void *addr, int length, struct bcache *bcache, 
+		   int *added)
+{
   unsigned long full_hash;
   unsigned short half_hash;
   int hash_index;
   struct bstring *s;
 
+  if (added)
+    *added = 0;
+
   /* If our average chain length is too high, expand the hash table.  */
   if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
     expand_hash_table (bcache);
@@ -242,21 +271,12 @@ bcache_data (const void *addr, int lengt
     bcache->unique_size += length;
     bcache->structure_size += BSTRING_SIZE (length);
 
+    if (added)
+      *added = 1;
+
     return &new->d.data;
   }
 }
-
-void *
-deprecated_bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
-
-const void *
-bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
 \f
 /* Allocating and freeing bcaches.  */
 
Index: gdb/bcache.h
===================================================================
RCS file: /cvs/src/src/gdb/bcache.h,v
retrieving revision 1.12
diff -u -p -r1.12 bcache.h
--- gdb/bcache.h	1 Jan 2008 22:53:09 -0000	1.12
+++ gdb/bcache.h	5 May 2008 18:49:56 -0000
@@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo
 extern const void *bcache (const void *addr, int length,
 			   struct bcache *bcache);
 
+extern void *bcache_added (const void *addr, int length,
+			   struct bcache *bcache, int *added);
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
 
Index: gdb/dwarf2read.c
===================================================================
RCS file: /cvs/src/src/gdb/dwarf2read.c,v
retrieving revision 1.263
diff -u -p -r1.263 dwarf2read.c
--- gdb/dwarf2read.c	5 May 2008 14:37:32 -0000	1.263
+++ gdb/dwarf2read.c	5 May 2008 18:49:57 -0000
@@ -1964,11 +1964,10 @@ add_partial_symbol (struct partial_die_i
              in the global scope.  */
 	  /*prim_record_minimal_symbol (actual_name, pdi->lowpc + baseaddr,
 	     mst_text, objfile); */
-	  psym = add_psymbol_to_list (actual_name, strlen (actual_name),
-				      VAR_DOMAIN, LOC_BLOCK,
-				      &objfile->global_psymbols,
-				      0, pdi->lowpc + baseaddr,
-				      cu->language, objfile);
+	  psym = add_psymbol_to_global_list (actual_name, strlen (actual_name),
+					     VAR_DOMAIN, LOC_BLOCK,
+					     0, pdi->lowpc + baseaddr,
+					     cu->language, objfile);
 	}
       else
 	{
@@ -2000,11 +1999,11 @@ add_partial_symbol (struct partial_die_i
 	  if (pdi->locdesc)
 	    addr = decode_locdesc (pdi->locdesc, cu);
 	  if (pdi->locdesc || pdi->has_type)
-	    psym = add_psymbol_to_list (actual_name, strlen (actual_name),
-					VAR_DOMAIN, LOC_STATIC,
-					&objfile->global_psymbols,
-					0, addr + baseaddr,
-					cu->language, objfile);
+	    psym = add_psymbol_to_global_list (actual_name, 
+					       strlen (actual_name),
+					       VAR_DOMAIN, LOC_STATIC,
+					       0, addr + baseaddr,
+					       cu->language, objfile);
 	}
       else
 	{
@@ -2034,10 +2033,9 @@ add_partial_symbol (struct partial_die_i
 			   0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_namespace:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   VAR_DOMAIN, LOC_TYPEDEF,
-			   &objfile->global_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				  VAR_DOMAIN, LOC_TYPEDEF,
+				  0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_class_type:
     case DW_TAG_interface_type:
@@ -2058,23 +2056,28 @@ add_partial_symbol (struct partial_die_i
 
       /* NOTE: carlton/2003-10-07: See comment in new_symbol about
 	 static vs. global.  */
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   STRUCT_DOMAIN, LOC_TYPEDEF,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
-
+      if (cu->language == language_cplus 
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    STRUCT_DOMAIN, LOC_TYPEDEF,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     STRUCT_DOMAIN, LOC_TYPEDEF,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     case DW_TAG_enumerator:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   VAR_DOMAIN, LOC_CONST,
-			   (cu->language == language_cplus
-			    || cu->language == language_java)
-			   ? &objfile->global_psymbols
-			   : &objfile->static_psymbols,
-			   0, (CORE_ADDR) 0, cu->language, objfile);
+      if (cu->language == language_cplus 
+	  || cu->language == language_java)
+	add_psymbol_to_global_list (actual_name, strlen (actual_name),
+				    VAR_DOMAIN, LOC_CONST,
+				    0, (CORE_ADDR) 0, cu->language, objfile);
+      else 
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     VAR_DOMAIN, LOC_CONST,
+			     &objfile->static_psymbols,
+			     0, (CORE_ADDR) 0, cu->language, objfile);
       break;
     default:
       break;
@@ -5638,13 +5641,21 @@ load_partial_dies (bfd *abfd, gdb_byte *
 	  if (part_die->name == NULL)
 	    complaint (&symfile_complaints, _("malformed enumerator DIE ignored"));
 	  else if (building_psymtab)
-	    add_psymbol_to_list (part_die->name, strlen (part_die->name),
-				 VAR_DOMAIN, LOC_CONST,
-				 (cu->language == language_cplus
-				  || cu->language == language_java)
-				 ? &cu->objfile->global_psymbols
-				 : &cu->objfile->static_psymbols,
-				 0, (CORE_ADDR) 0, cu->language, cu->objfile);
+	    {
+	      if (cu->language == language_cplus
+		  || cu->language == language_java)
+		add_psymbol_to_global_list (part_die->name, 
+					    strlen (part_die->name),
+					    VAR_DOMAIN, LOC_CONST,
+					    0, (CORE_ADDR) 0, 
+					    cu->language, cu->objfile);
+	      else
+		add_psymbol_to_list (part_die->name, strlen (part_die->name),
+				     VAR_DOMAIN, LOC_CONST,
+				     &cu->objfile->static_psymbols,
+				     0, (CORE_ADDR) 0, 
+				     cu->language, cu->objfile);
+	    }
 
 	  info_ptr = locate_pdi_sibling (part_die, info_ptr, abfd, cu);
 	  continue;
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.203
diff -u -p -r1.203 symfile.c
--- gdb/symfile.c	5 May 2008 16:13:49 -0000	1.203
+++ gdb/symfile.c	5 May 2008 18:49:57 -0000
@@ -3082,6 +3082,68 @@ start_psymtab_common (struct objfile *ob
   return (psymtab);
 }
 \f
+/* Helper function, initialises partial symbol structure and stashes 
+   it into objfile's bcache.  Note that our caching mechanism will
+   use all fields of struct partial_symbol to determine hash value of the
+   structure.  In other words, having two symbols with the same name but
+   different domain (or address) is possible and correct.  */
+
+static struct partial_symbol *
+add_psymbol_to_bcache (char *name, int namelength, domain_enum domain,
+		       enum address_class class,
+		       long val,	/* Value as a long */
+		       CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+		       enum language language, struct objfile *objfile,
+		       int *added)
+{
+  char *buf = name;  
+  /* psymbol is static so that there will be no uninitialized gaps in the
+     structure which might contain random data, causing cache misses in
+     bcache. */
+  static struct partial_symbol psymbol;
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
+  /* val and coreaddr are mutually exclusive, one of them *will* be zero */
+  if (val != 0)
+    {
+      SYMBOL_VALUE (&psymbol) = val;
+    }
+  else
+    {
+      SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
+    }
+  SYMBOL_SECTION (&psymbol) = 0;
+  SYMBOL_LANGUAGE (&psymbol) = language;
+  PSYMBOL_DOMAIN (&psymbol) = domain;
+  PSYMBOL_CLASS (&psymbol) = class;
+
+  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+
+  /* Stash the partial symbol away in the cache */
+  return bcache_added (&psymbol, sizeof (struct partial_symbol),
+		       objfile->psymbol_cache, added);
+}
+
+/* Helper function, adds partial symbol to the given partial symbol
+   list.  */
+
+static void
+append_psymbol_to_list (struct psymbol_allocation_list *list,
+			struct partial_symbol *psym,
+			struct objfile *objfile)
+{
+  if (list->next >= list->list + list->size)
+    extend_psymbol_list (list, objfile);
+  *list->next++ = psym;
+  OBJSTAT (objfile, n_psyms++);
+}
+
 /* Add a symbol with a long value to a psymtab.
    Since one arg is a struct, we pass in a ptr and deref it (sigh).
    Return the partial symbol that has been added.  */
@@ -3105,43 +3167,37 @@ add_psymbol_to_list (char *name, int nam
 		     enum language language, struct objfile *objfile)
 {
   struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
-  /* psymbol is static so that there will be no uninitialized gaps in the
-     structure which might contain random data, causing cache misses in
-     bcache. */
-  static struct partial_symbol psymbol;
+  /* Stash the partial symbol away in the cache */
+  psym = add_psymbol_to_bcache (name, namelength, domain, class,
+				val, coreaddr, language, objfile, NULL);
 
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
-  /* val and coreaddr are mutually exclusive, one of them *will* be zero */
-  if (val != 0)
-    {
-      SYMBOL_VALUE (&psymbol) = val;
-    }
-  else
-    {
-      SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr;
-    }
-  SYMBOL_SECTION (&psymbol) = 0;
-  SYMBOL_LANGUAGE (&psymbol) = language;
-  PSYMBOL_DOMAIN (&psymbol) = domain;
-  PSYMBOL_CLASS (&psymbol) = class;
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (list, psym, objfile);
+  return psym;
+}
 
-  SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
+/* Add partial symbol to OBJFILE's global list of partial symbols. 
+   Make sure we do not duplicate this information.  See add_psymbol_to_bcache
+   for more details.  */
 
-  /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+const struct partial_symbol *
+add_psymbol_to_global_list (char *name, int namelength, domain_enum domain,
+			    enum address_class class,
+			    long val,	/* Value as a long */
+			    CORE_ADDR coreaddr,	/* Value as a CORE_ADDR */
+			    enum language language, struct objfile *objfile)
+{
+  struct partial_symbol *psym;
+  int added;
+  
+  psym = add_psymbol_to_bcache (name, namelength, domain, class, val,
+			        coreaddr, language, objfile, &added);
 
-  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
-  if (list->next >= list->list + list->size)
-    {
-      extend_psymbol_list (list, objfile);
-    }
-  *list->next++ = psym;
-  OBJSTAT (objfile, n_psyms++);
+  if (!added)
+    return psym;
 
+  /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
+  append_psymbol_to_list (&objfile->global_psymbols, psym, objfile);
   return psym;
 }
 
Index: gdb/symfile.h
===================================================================
RCS file: /cvs/src/src/gdb/symfile.h,v
retrieving revision 1.46
diff -u -p -r1.46 symfile.h
--- gdb/symfile.h	3 Feb 2008 22:13:29 -0000	1.46
+++ gdb/symfile.h	5 May 2008 18:49:57 -0000
@@ -199,6 +199,13 @@ struct partial_symbol *add_psymbol_to_li
 					    long, CORE_ADDR,
 					    enum language, struct objfile *);
 
+extern const
+struct partial_symbol *add_psymbol_to_global_list (char *, int, domain_enum,
+						   enum address_class,
+						   long, CORE_ADDR,
+						   enum language,
+						   struct objfile *);
+
 extern void init_psymbol_list (struct objfile *, int);
 
 extern void sort_pst_symbols (struct partial_symtab *);

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

[Daniel Jacobowitz]

On Mon, May 05, 2008 at 02:58:53PM -0400, Aleksandar Ristovski wrote:
>>> +const void *
>>> +bcache (const void *addr, int length, struct bcache *bcache)
>>> +{
>>> +  return bcache_data (addr, length, bcache);
>>> +}
>>> +
>>> +void *
>>> +bcache_added (const void *addr, int length, struct bcache *bcache,  
>>> +		   int *added)
>>
>> It should return a const pointer, like bcache.  Also the indentation
>> on the second line is too deep.
>
> bcache_added is more like bcache_data which returns void*. It would make 
> sense to return void const* but then I would have to change const-ness in 
> many places (too many: I would leave that for some other patch).

This really is not OK.  Take a look at the explanation of a bcache:

   A bcache is a data structure for factoring out duplication in
   read-only structures.

If you modify the pointed-to object then things go wrong: we put
duplicates in the cache which is what it's supposed to avoid.  And you
modify the other copies which share the same bcache entry, corrupting
them.

If you don't want to fix the constification issues right now, then
just call this deprecated_bcache_added and we can fix it later; you're
not making it any worse.  But this is something you have to understand
about the bcache.

>> In fact, if lookup_partial_symbol will never find a global symbol
>> with an identical name why do we need them for any duplicated symbol,
>> even legitimately duplicated things like static functions?  
>
> I don't think static functions are a good example since they would not go 
> into the global list anyway (see dwarf2read.c, case DW_TAG_subprogram).
>
> The attached is revised patch with the changes above; additionally, it  
> calls new add_partial_symbol_to_global_list for all partial symbols that  
> are being added to the global list. 

This is not symbol-reader specific, right?  There's nothing that makes
it safe for DWARF2 but unsafe for stabs?

If so, a smaller patch would be do just do this in add_psymbol_to_list
if list == &objfile->global_symbols.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, May 05, 2008 at 02:58:53PM -0400, Aleksandar Ristovski wrote:
>>>> +const void *
>>>> +bcache (const void *addr, int length, struct bcache *bcache)
>>>> +{
>>>> +  return bcache_data (addr, length, bcache);
>>>> +}
>>>> +
>>>> +void *
>>>> +bcache_added (const void *addr, int length, struct bcache *bcache,  
>>>> +		   int *added)
>>> It should return a const pointer, like bcache.  Also the indentation
>>> on the second line is too deep.
>> bcache_added is more like bcache_data which returns void*. It would make 
>> sense to return void const* but then I would have to change const-ness in 
>> many places (too many: I would leave that for some other patch).
> 
> This really is not OK.  Take a look at the explanation of a bcache:
> 
>    A bcache is a data structure for factoring out duplication in
>    read-only structures.

Yes, but only while loading partial symbols; I am not aware it is being used after psymbols have been loaded.

> 
> If you modify the pointed-to object then things go wrong: we put
> duplicates in the cache which is what it's supposed to avoid.  And you
> modify the other copies which share the same bcache entry, corrupting
> them.
> 
> If you don't want to fix the constification issues right now, then
> just call this deprecated_bcache_added and we can fix it later; you're
> not making it any worse.  But this is something you have to understand
> about the bcache.

Understood and agreed. As you said, this patch does not make things any worse than what it is now.

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

I don't see how. Each reader treats psymbols in its own way (unless I am missing something here).

> 
> This is not symbol-reader specific, right?  There's nothing that makes
> it safe for DWARF2 but unsafe for stabs?
> 
> If so, a smaller patch would be do just do this in add_psymbol_to_list
> if list == &objfile->global_symbols.
> 

Yes it could be done that way, I thought what I proposed was 'cleaner'... but either way it would work.

I am not, however, happy with the patch completely - I think we can optimize it a bit more. Both struct partial_symbol and struct symbol are space critical and we should save everything we (reasonably) can.

My "feeling" is that we could use struct symbol for both, populate only the "partial_symbol" portion and then, as we load full symbols, "promote" the existing allocated structures to full symbols; some operations would not repeat (like name construction or fixup_[p]?symbol_section); initial memory allocation would be greater, but in the long run I would expect that gdb would use less memory than it does now. Savings I am hoping to see from such approach are: overall lower memory consumption, much lower number of memory alloc's, and saving some redundant operations we do for both psyms and syms.

Or, maybe, we could go step by step: commit this patch (providing you find it good enough) and then continue with more extensive changes?

Either way, let me know what you think.

Thanks,

Aleksandar

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

[Aleksandar Ristovski]

Daniel Jacobowitz wrote:
> On Mon, May 05, 2008 at 02:58:53PM -0400, Aleksandar Ristovski wrote:
>>>> +const void *
>>>> +bcache (const void *addr, int length, struct bcache *bcache)
>>>> +{
>>>> +  return bcache_data (addr, length, bcache);
>>>> +}
>>>> +
>>>> +void *
>>>> +bcache_added (const void *addr, int length, struct bcache *bcache,  
>>>> +		   int *added)
>>> It should return a const pointer, like bcache.  Also the indentation
>>> on the second line is too deep.
>> bcache_added is more like bcache_data which returns void*. It would make 
>> sense to return void const* but then I would have to change const-ness in 
>> many places (too many: I would leave that for some other patch).
> 
> This really is not OK.  Take a look at the explanation of a bcache:
> 
>    A bcache is a data structure for factoring out duplication in
>    read-only structures.

Yes, but only while loading partial symbols; I am not aware it is being used after psymbols have been loaded.

> 
> If you modify the pointed-to object then things go wrong: we put
> duplicates in the cache which is what it's supposed to avoid.  And you
> modify the other copies which share the same bcache entry, corrupting
> them.
> 
> If you don't want to fix the constification issues right now, then
> just call this deprecated_bcache_added and we can fix it later; you're
> not making it any worse.  But this is something you have to understand
> about the bcache.

Understood and agreed. As you said, this patch does not make things any worse than what it is now.

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

I don't see how. Each reader treats psymbols in its own way (unless I am missing something here).

> 
> This is not symbol-reader specific, right?  There's nothing that makes
> it safe for DWARF2 but unsafe for stabs?
> 
> If so, a smaller patch would be do just do this in add_psymbol_to_list
> if list == &objfile->global_symbols.
> 

Yes it could be done that way, I thought what I proposed was 'cleaner'... but either way it would work.

I am not, however, happy with the patch completely - I think we can optimize it a bit more. Both struct partial_symbol and struct symbol are space critical and we should save everything we (reasonably) can.

My "feeling" is that we could use struct symbol for both, populate only the "partial_symbol" portion and then, as we load full symbols, "promote" the existing allocated structures to full symbols; some operations would not repeat (like name construction or fixup_[p]?symbol_section); initial memory allocation would be greater, but in the long run I would expect that gdb would use less memory than it does now. Savings I am hoping to see from such approach are: overall lower memory consumption, much lower number of memory alloc's, and saving some redundant operations we do for both psyms and syms.

Or, maybe, we could go step by step: commit this patch (providing you find it good enough) and then continue with more extensive changes?

Either way, let me know what you think.

Thanks,

Aleksandar

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

[Daniel Jacobowitz]

On Tue, May 06, 2008 at 01:42:41PM -0400, Aleksandar Ristovski wrote:
> I am not, however, happy with the patch completely - I think we can
> optimize it a bit more. Both struct partial_symbol and struct symbol
> are space critical and we should save everything we (reasonably)
> can.
>
> My "feeling" is that we could use struct symbol for both, populate
> only the "partial_symbol" portion and then, as we load full symbols,
> "promote" the existing allocated structures to full symbols; some
> operations would not repeat (like name construction or
> fixup_[p]?symbol_section); initial memory allocation would be
> greater, but in the long run I would expect that gdb would use less
> memory than it does now. Savings I am hoping to see from such
> approach are: overall lower memory consumption, much lower number of
> memory alloc's, and saving some redundant operations we do for both
> psyms and syms.

A symbol is about twice as big as a psymbol.  So this would only lead
to less overall memory if we loaded more than half the full symbols in
an average debugging session.  I just don't think that's true.  It's
probably more like 5%.

And the bcache allows psymbols that look the same to use the same
memory.  It's hugely effective - check maint print statistics.  No
two symbols can share memory for various reasons.  For instance, two
global functions will have different block pointers.

> Or, maybe, we could go step by step: commit this patch (providing
> you find it good enough) and then continue with more extensive
> changes?

I think we should finish up this patch, and then proceed from there.

Should we use this optimization for all global psymtabs?  If so, then
we ought to do it in common code so that all the other symbol readers,
like stabs, benefit.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

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

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

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

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

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


Thanks,

Aleksandar


ChangeLog

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



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

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

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

[Aleksandar Ristovski]

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

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

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

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

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


Thanks,

Aleksandar


ChangeLog

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



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

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

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

[Daniel Jacobowitz]

On Tue, May 06, 2008 at 03:24:34PM -0400, Aleksandar Ristovski wrote:
> Daniel Jacobowitz wrote:
>>
>> I think we should finish up this patch, and then proceed from there.
>>
>> Should we use this optimization for all global psymtabs?  If so, then
>> we ought to do it in common code so that all the other symbol readers,
>> like stabs, benefit.
>>
>
> Ok, here is simplified, but with broader consequences patch.
>
> The patch now affects all readers that use add_psymbol_to_list by not 
> allowing duplicate partial symbols in the global psymbol list (for a given 
> objfile).
>
> Tested on linux dwarf2 format, no regression. I did
> not test other debug formats.

This is OK if you'll rename bcache_added to deprecated_bcache_added,
as we discussed before.  I think it'll work fine.

> +void *
> +bcache_added (const void *addr, int length, struct bcache *bcache, 
> +		   int *added)

Wrong indentation there?


-- 
Daniel Jacobowitz
CodeSourcery

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

[Aleksandar Ristovski]

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

Daniel Jacobowitz wrote:
> 
> This is OK if you'll rename bcache_added to deprecated_bcache_added,
> as we discussed before.  I think it'll work fine.
> 

Committed. Final patch attached (with deprecated_bcache_added and fixed formatting issues).

2008-06-05  Aleksandar Ristovski  <aristovski@qnx.com>

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


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

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