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

Aleksandar Ristovski
2008-02-11T22:10:43Z

[-- 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)