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

Aleksandar Ristovski
2008-05-06T19:25:12Z

[-- 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;
}