From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17619 invoked by alias); 5 May 2008 18:59:16 -0000 Received: (qmail 17605 invoked by uid 22791); 5 May 2008 18:59:14 -0000 X-Spam-Check-By: sourceware.org Received: from qnxmail.qnx.com (HELO nimbus.ott.qnx.com) (209.226.137.76) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 05 May 2008 18:58:56 +0000 Received: from [10.42.100.129] (dhcp-100-129.ott.qnx.com [10.42.100.129]) by nimbus.ott.qnx.com with SMTP (Microsoft Exchange Internet Mail Service Version 5.5.2653.13) id J6QQ493R; Mon, 5 May 2008 14:58:53 -0400 Message-ID: <481F58ED.5070207@qnx.com> Date: Mon, 05 May 2008 19:38:00 -0000 From: Aleksandar Ristovski User-Agent: Thunderbird 2.0.0.14 (Windows/20080421) MIME-Version: 1.0 Newsgroups: gmane.comp.gdb.patches To: gdb-patches@sourceware.org, Daniel Jacobowitz CC: Aleksandar Ristovski Subject: Re: [patch] Do not add partial_symbol again and again to the list References: <20080211203809.GA29560@caradoc.them.org> <47B0B56F.4010607@qnx.com> <20080211210935.GA31767@caradoc.them.org> <47B0C0F4.4090302@qnx.com> <20080211214750.GA1953@caradoc.them.org> <47B0C7C6.9090605@qnx.com> <20080211223056.GA3833@caradoc.them.org> <47B0CF8A.6080306@qnx.com> <20080211225314.GA5832@caradoc.them.org> <47B27EB1.6030606@qnx.com> <20080503205413.GA22704@caradoc.them.org> In-Reply-To: <20080503205413.GA22704@caradoc.them.org> Content-Type: multipart/mixed; boundary="------------050805050808060608060809" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-05/txt/msg00212.txt.bz2 Message-ID: <20080505193800.CjFLLfnhUXpRWxJfGzvUu9nUo84eMHlaaA2vpBrsPUw@z> This is a multi-part message in MIME format. --------------050805050808060608060809 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3925 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. --------------050805050808060608060809 Content-Type: text/plain; name="bcache_added_donotduplicatepsyms20080505.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="bcache_added_donotduplicatepsyms20080505.diff" Content-length: 13319 Index: gdb/bcache.c =================================================================== RCS file: /cvs/src/src/gdb/bcache.c,v retrieving revision 1.20 diff -u -p -r1.20 bcache.c --- gdb/bcache.c 1 Jan 2008 22:53:09 -0000 1.20 +++ gdb/bcache.c 5 May 2008 18:49:56 -0000 @@ -197,11 +197,40 @@ expand_hash_table (struct bcache *bcache static void * bcache_data (const void *addr, int length, struct bcache *bcache) { + return bcache_added (addr, length, bcache, NULL); +} + + +void * +deprecated_bcache (const void *addr, int length, struct bcache *bcache) +{ + return bcache_data (addr, length, bcache); +} + +const void * +bcache (const void *addr, int length, struct bcache *bcache) +{ + return bcache_data (addr, length, bcache); +} + +/* Find a copy of the LENGTH bytes at ADDR in BCACHE. If BCACHE has + never seen those bytes before, add a copy of them to BCACHE. In + either case, return a pointer to BCACHE's copy of that string. If + optional ADDED is not NULL, return 1 in case of new entry or 0 if + returning an old entry. */ + +void * +bcache_added (const void *addr, int length, struct bcache *bcache, + int *added) +{ unsigned long full_hash; unsigned short half_hash; int hash_index; struct bstring *s; + if (added) + *added = 0; + /* If our average chain length is too high, expand the hash table. */ if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD) expand_hash_table (bcache); @@ -242,21 +271,12 @@ bcache_data (const void *addr, int lengt bcache->unique_size += length; bcache->structure_size += BSTRING_SIZE (length); + if (added) + *added = 1; + return &new->d.data; } } - -void * -deprecated_bcache (const void *addr, int length, struct bcache *bcache) -{ - return bcache_data (addr, length, bcache); -} - -const void * -bcache (const void *addr, int length, struct bcache *bcache) -{ - return bcache_data (addr, length, bcache); -} /* Allocating and freeing bcaches. */ Index: gdb/bcache.h =================================================================== RCS file: /cvs/src/src/gdb/bcache.h,v retrieving revision 1.12 diff -u -p -r1.12 bcache.h --- gdb/bcache.h 1 Jan 2008 22:53:09 -0000 1.12 +++ gdb/bcache.h 5 May 2008 18:49:56 -0000 @@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo extern const void *bcache (const void *addr, int length, struct bcache *bcache); +extern void *bcache_added (const void *addr, int length, + struct bcache *bcache, int *added); /* Free all the storage used by BCACHE. */ extern void bcache_xfree (struct bcache *bcache); Index: gdb/dwarf2read.c =================================================================== RCS file: /cvs/src/src/gdb/dwarf2read.c,v retrieving revision 1.263 diff -u -p -r1.263 dwarf2read.c --- gdb/dwarf2read.c 5 May 2008 14:37:32 -0000 1.263 +++ gdb/dwarf2read.c 5 May 2008 18:49:57 -0000 @@ -1964,11 +1964,10 @@ add_partial_symbol (struct partial_die_i in the global scope. */ /*prim_record_minimal_symbol (actual_name, pdi->lowpc + baseaddr, mst_text, objfile); */ - psym = add_psymbol_to_list (actual_name, strlen (actual_name), - VAR_DOMAIN, LOC_BLOCK, - &objfile->global_psymbols, - 0, pdi->lowpc + baseaddr, - cu->language, objfile); + psym = add_psymbol_to_global_list (actual_name, strlen (actual_name), + VAR_DOMAIN, LOC_BLOCK, + 0, pdi->lowpc + baseaddr, + cu->language, objfile); } else { @@ -2000,11 +1999,11 @@ add_partial_symbol (struct partial_die_i if (pdi->locdesc) addr = decode_locdesc (pdi->locdesc, cu); if (pdi->locdesc || pdi->has_type) - psym = add_psymbol_to_list (actual_name, strlen (actual_name), - VAR_DOMAIN, LOC_STATIC, - &objfile->global_psymbols, - 0, addr + baseaddr, - cu->language, objfile); + psym = add_psymbol_to_global_list (actual_name, + strlen (actual_name), + VAR_DOMAIN, LOC_STATIC, + 0, addr + baseaddr, + cu->language, objfile); } else { @@ -2034,10 +2033,9 @@ add_partial_symbol (struct partial_die_i 0, (CORE_ADDR) 0, cu->language, objfile); break; case DW_TAG_namespace: - add_psymbol_to_list (actual_name, strlen (actual_name), - VAR_DOMAIN, LOC_TYPEDEF, - &objfile->global_psymbols, - 0, (CORE_ADDR) 0, cu->language, objfile); + add_psymbol_to_global_list (actual_name, strlen (actual_name), + VAR_DOMAIN, LOC_TYPEDEF, + 0, (CORE_ADDR) 0, cu->language, objfile); break; case DW_TAG_class_type: case DW_TAG_interface_type: @@ -2058,23 +2056,28 @@ add_partial_symbol (struct partial_die_i /* NOTE: carlton/2003-10-07: See comment in new_symbol about static vs. global. */ - add_psymbol_to_list (actual_name, strlen (actual_name), - STRUCT_DOMAIN, LOC_TYPEDEF, - (cu->language == language_cplus - || cu->language == language_java) - ? &objfile->global_psymbols - : &objfile->static_psymbols, - 0, (CORE_ADDR) 0, cu->language, objfile); - + if (cu->language == language_cplus + || cu->language == language_java) + add_psymbol_to_global_list (actual_name, strlen (actual_name), + STRUCT_DOMAIN, LOC_TYPEDEF, + 0, (CORE_ADDR) 0, cu->language, objfile); + else + add_psymbol_to_list (actual_name, strlen (actual_name), + STRUCT_DOMAIN, LOC_TYPEDEF, + &objfile->static_psymbols, + 0, (CORE_ADDR) 0, cu->language, objfile); break; case DW_TAG_enumerator: - add_psymbol_to_list (actual_name, strlen (actual_name), - VAR_DOMAIN, LOC_CONST, - (cu->language == language_cplus - || cu->language == language_java) - ? &objfile->global_psymbols - : &objfile->static_psymbols, - 0, (CORE_ADDR) 0, cu->language, objfile); + if (cu->language == language_cplus + || cu->language == language_java) + add_psymbol_to_global_list (actual_name, strlen (actual_name), + VAR_DOMAIN, LOC_CONST, + 0, (CORE_ADDR) 0, cu->language, objfile); + else + add_psymbol_to_list (actual_name, strlen (actual_name), + VAR_DOMAIN, LOC_CONST, + &objfile->static_psymbols, + 0, (CORE_ADDR) 0, cu->language, objfile); break; default: break; @@ -5638,13 +5641,21 @@ load_partial_dies (bfd *abfd, gdb_byte * if (part_die->name == NULL) complaint (&symfile_complaints, _("malformed enumerator DIE ignored")); else if (building_psymtab) - add_psymbol_to_list (part_die->name, strlen (part_die->name), - VAR_DOMAIN, LOC_CONST, - (cu->language == language_cplus - || cu->language == language_java) - ? &cu->objfile->global_psymbols - : &cu->objfile->static_psymbols, - 0, (CORE_ADDR) 0, cu->language, cu->objfile); + { + if (cu->language == language_cplus + || cu->language == language_java) + add_psymbol_to_global_list (part_die->name, + strlen (part_die->name), + VAR_DOMAIN, LOC_CONST, + 0, (CORE_ADDR) 0, + cu->language, cu->objfile); + else + add_psymbol_to_list (part_die->name, strlen (part_die->name), + VAR_DOMAIN, LOC_CONST, + &cu->objfile->static_psymbols, + 0, (CORE_ADDR) 0, + cu->language, cu->objfile); + } info_ptr = locate_pdi_sibling (part_die, info_ptr, abfd, cu); continue; Index: gdb/symfile.c =================================================================== RCS file: /cvs/src/src/gdb/symfile.c,v retrieving revision 1.203 diff -u -p -r1.203 symfile.c --- gdb/symfile.c 5 May 2008 16:13:49 -0000 1.203 +++ gdb/symfile.c 5 May 2008 18:49:57 -0000 @@ -3082,6 +3082,68 @@ start_psymtab_common (struct objfile *ob return (psymtab); } +/* Helper function, initialises partial symbol structure and stashes + it into objfile's bcache. Note that our caching mechanism will + use all fields of struct partial_symbol to determine hash value of the + structure. In other words, having two symbols with the same name but + different domain (or address) is possible and correct. */ + +static struct partial_symbol * +add_psymbol_to_bcache (char *name, int namelength, domain_enum domain, + enum address_class class, + long val, /* Value as a long */ + CORE_ADDR coreaddr, /* Value as a CORE_ADDR */ + enum language language, struct objfile *objfile, + int *added) +{ + char *buf = name; + /* psymbol is static so that there will be no uninitialized gaps in the + structure which might contain random data, causing cache misses in + bcache. */ + static struct partial_symbol psymbol; + + if (name[namelength] != '\0') + { + buf = alloca (namelength + 1); + /* Create local copy of the partial symbol */ + memcpy (buf, name, namelength); + buf[namelength] = '\0'; + } + /* val and coreaddr are mutually exclusive, one of them *will* be zero */ + if (val != 0) + { + SYMBOL_VALUE (&psymbol) = val; + } + else + { + SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr; + } + SYMBOL_SECTION (&psymbol) = 0; + SYMBOL_LANGUAGE (&psymbol) = language; + PSYMBOL_DOMAIN (&psymbol) = domain; + PSYMBOL_CLASS (&psymbol) = class; + + SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile); + + /* Stash the partial symbol away in the cache */ + return bcache_added (&psymbol, sizeof (struct partial_symbol), + objfile->psymbol_cache, added); +} + +/* Helper function, adds partial symbol to the given partial symbol + list. */ + +static void +append_psymbol_to_list (struct psymbol_allocation_list *list, + struct partial_symbol *psym, + struct objfile *objfile) +{ + if (list->next >= list->list + list->size) + extend_psymbol_list (list, objfile); + *list->next++ = psym; + OBJSTAT (objfile, n_psyms++); +} + /* Add a symbol with a long value to a psymtab. Since one arg is a struct, we pass in a ptr and deref it (sigh). Return the partial symbol that has been added. */ @@ -3105,43 +3167,37 @@ add_psymbol_to_list (char *name, int nam enum language language, struct objfile *objfile) { struct partial_symbol *psym; - char *buf = alloca (namelength + 1); - /* psymbol is static so that there will be no uninitialized gaps in the - structure which might contain random data, causing cache misses in - bcache. */ - static struct partial_symbol psymbol; + /* Stash the partial symbol away in the cache */ + psym = add_psymbol_to_bcache (name, namelength, domain, class, + val, coreaddr, language, objfile, NULL); - /* Create local copy of the partial symbol */ - memcpy (buf, name, namelength); - buf[namelength] = '\0'; - /* val and coreaddr are mutually exclusive, one of them *will* be zero */ - if (val != 0) - { - SYMBOL_VALUE (&psymbol) = val; - } - else - { - SYMBOL_VALUE_ADDRESS (&psymbol) = coreaddr; - } - SYMBOL_SECTION (&psymbol) = 0; - SYMBOL_LANGUAGE (&psymbol) = language; - PSYMBOL_DOMAIN (&psymbol) = domain; - PSYMBOL_CLASS (&psymbol) = class; + /* Save pointer to partial symbol in psymtab, growing symtab if needed. */ + append_psymbol_to_list (list, psym, objfile); + return psym; +} - SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile); +/* Add partial symbol to OBJFILE's global list of partial symbols. + Make sure we do not duplicate this information. See add_psymbol_to_bcache + for more details. */ - /* Stash the partial symbol away in the cache */ - psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol), - objfile->psymbol_cache); +const struct partial_symbol * +add_psymbol_to_global_list (char *name, int namelength, domain_enum domain, + enum address_class class, + long val, /* Value as a long */ + CORE_ADDR coreaddr, /* Value as a CORE_ADDR */ + enum language language, struct objfile *objfile) +{ + struct partial_symbol *psym; + int added; + + psym = add_psymbol_to_bcache (name, namelength, domain, class, val, + coreaddr, language, objfile, &added); - /* Save pointer to partial symbol in psymtab, growing symtab if needed. */ - if (list->next >= list->list + list->size) - { - extend_psymbol_list (list, objfile); - } - *list->next++ = psym; - OBJSTAT (objfile, n_psyms++); + if (!added) + return psym; + /* Save pointer to partial symbol in psymtab, growing symtab if needed. */ + append_psymbol_to_list (&objfile->global_psymbols, psym, objfile); return psym; } Index: gdb/symfile.h =================================================================== RCS file: /cvs/src/src/gdb/symfile.h,v retrieving revision 1.46 diff -u -p -r1.46 symfile.h --- gdb/symfile.h 3 Feb 2008 22:13:29 -0000 1.46 +++ gdb/symfile.h 5 May 2008 18:49:57 -0000 @@ -199,6 +199,13 @@ struct partial_symbol *add_psymbol_to_li long, CORE_ADDR, enum language, struct objfile *); +extern const +struct partial_symbol *add_psymbol_to_global_list (char *, int, domain_enum, + enum address_class, + long, CORE_ADDR, + enum language, + struct objfile *); + extern void init_psymbol_list (struct objfile *, int); extern void sort_pst_symbols (struct partial_symtab *); --------------050805050808060608060809--