* [PATCH 1/5] gdb/23712: Introduce multidictionary's @ 2018-11-09 2:28 Keith Seitz 2018-11-09 2:28 ` [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list Keith Seitz ` (4 more replies) 0 siblings, 5 replies; 15+ messages in thread From: Keith Seitz @ 2018-11-09 2:28 UTC (permalink / raw) To: gdb-patches gdb/23712 is a new manifestation of the now-infamous (at least to me) symtab/23010 assertion failure (DICT_LANGUAGE == SYMBOL_LANGAUGE). An example of the problem (using test case from symtab/23010): Reading symbols from /home/rdiez/rdiez/arduino/JtagDue/BuildOutput/JtagDue-obj-release/firmware.elf...done. (gdb) p SysTick_Handler dwarf2read.c:9715: internal-error: void dw2_add_symbol_to_list(symbol*, pending**): Assertion `(*listhead) == NULL || (SYMBOL_LANGUAGE ((*listhead)->symbol[0]) == SYMBOL_LANGUAGE (symbol))' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) This assertion was added specifically to catch this condition (of adding symbols of different languages to a single pending list). The problems we're now seeing on systems utilizing DWARF debugging seem to be caused by the use of LTO, which adds a CU with an artificial DIE of language C99 which references DIEs in other CUs of language C++. Thus, we create a dictionary containing symbols of C99 but end up stuffing C++ symbols into it, and the dw2_add_symbol_to_list triggers. The approach taken here to fix this is to introduce multi-language dictionaries to "replace" the standard, single-language dictionaries used today. Note to reviewers: This patch introduces some temporary functions to aide with review. This and other artifacts (such as "See dictionary.h" which appear incorrect) will all be valid at the end of the series. This first patch introduces the new multidictionary and its API (which is, by design, identical to the old dictionary interface). It also mutates dict_create_hashed and dict_create_linear so that they take a std::vector instead of the usual struct pending linked list. This will be needed later on. This patch does /not/ actually enable multidictionary's. That is left for a subsequent patch in the series. I've done exhaustive performance testing with this approach, and I've attempted to minimize the overhead for the (overwhelmingly) most common one-language scenario. On average, a -g3 -O0 GDB (the one we developers use) will see approximately a 4% slowdown when initially reading symbols. [I've tested only GDB and firefox with -readnow.] When using -O2, this difference shrinks to ~0.5%. Since a number of runs with these patches actually run /faster/ than unpatched GDB, I conclude that these tests have at least a 0.5% error margin. On our own gdb.perf test suite, again, results appear to be pretty negligible. Differences to unpatched GDB range from -7.8% (yes, patched version is again faster than unpatched) to 27%. All tests lying outside "negligible," such as the 27% slowdown, involve a total run time of 0.0007 (or less) with smaller numbers of CUs/DSOs (usually 10 or 100). In all cases, the follow-up tests with more CUs/DSOs is never more than 3% difference to the baseline, unpatched GDB. In my opinion, these results are satisfactory. gdb/ChangeLog: PR gdb/23712 * dictionary.c: Include unordered_map. (pending_to_vector): New function. (dict_create_hashed_1, dict_create_linear_1, dict_add_pending_1): Rewrite the non-"_1" functions to take vector instead of linked list. (dict_create_hashed, dict_create_linear, dict_add_pending): Use the "new" _1 versions of the same name. (multidictionary): Define. (std::hash<enum language): New definition. (collate_pending_symbols_by_language, mdict_create_hashed) (mdict_create_hashed_expandable, mdict_create_linear) (mdict_create_linear_expandable, mdict_free) (find_language_dictionary, create_new_language_dictionary) (mdict_add_symbol, mdict_add_pending, mdict_iterator_first) (mdict_iterator_next, mdict_iter_match_first, mdict_iter_match_next) (mdict_size, mdict_empty): New functions. * dictionary.h (mdict_iterator): Define. --- gdb/ChangeLog | 21 ++ gdb/dictionary.c | 554 +++++++++++++++++++++++++++++++++++++++++------ gdb/dictionary.h | 15 ++ 3 files changed, 522 insertions(+), 68 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index be1b3d8c7e..ccf5dbdf88 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,24 @@ +YYYY-MM-DD Keith Seitz <keiths@redhat.com> + + PR gdb/23712 + * dictionary.c: Include unordered_map. + (pending_to_vector): New function. + (dict_create_hashed_1, dict_create_linear_1, dict_add_pending_1): + Rewrite the non-"_1" functions to take vector instead + of linked list. + (dict_create_hashed, dict_create_linear, dict_add_pending): Use the + "new" _1 versions of the same name. + (multidictionary): Define. + (std::hash<enum language): New definition. + (collate_pending_symbols_by_language, mdict_create_hashed) + (mdict_create_hashed_expandable, mdict_create_linear) + (mdict_create_linear_expandable, mdict_free) + (find_language_dictionary, create_new_language_dictionary) + (mdict_add_symbol, mdict_add_pending, mdict_iterator_first) + (mdict_iterator_next, mdict_iter_match_first, mdict_iter_match_next) + (mdict_size, mdict_empty): New functions. + * dictionary.h (mdict_iterator): Define. + 2018-11-08 Joel Brobecker <brobecker@adacore.com> * aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): diff --git a/gdb/dictionary.c b/gdb/dictionary.c index da8b7da208..83d8126682 100644 --- a/gdb/dictionary.c +++ b/gdb/dictionary.c @@ -27,6 +27,7 @@ #include "buildsym.h" #include "dictionary.h" #include "safe-ctype.h" +#include <unordered_map> /* This file implements dictionaries, which are tables that associate symbols to names. They are represented by an opaque type 'struct @@ -341,53 +342,66 @@ static void insert_symbol_hashed (struct dictionary *dict, static void expand_hashtable (struct dictionary *dict); +/* A function to convert a linked list into a vector. */ + +static std::vector<symbol *> +pending_to_vector (const struct pending *symbol_list) +{ + std::vector<symbol *> symlist; + + for (const struct pending *list_counter = symbol_list; + list_counter != nullptr; list_counter = list_counter->next) + { + for (int i = list_counter->nsyms - 1; i >= 0; --i) + symlist.push_back (list_counter->symbol[i]); + } + + return symlist; +} + /* The creation functions. */ -/* See dictionary.h. */ +/* A function to transition dict_create_hashed to new API. */ -struct dictionary * -dict_create_hashed (struct obstack *obstack, - enum language language, - const struct pending *symbol_list) +static struct dictionary * +dict_create_hashed_1 (struct obstack *obstack, + enum language language, + const std::vector<symbol *> &symbol_list) { - struct dictionary *retval; - int nsyms = 0, nbuckets, i; - struct symbol **buckets; - const struct pending *list_counter; - - retval = XOBNEW (obstack, struct dictionary); + /* Allocate the dictionary. */ + struct dictionary *retval = XOBNEW (obstack, struct dictionary); DICT_VECTOR (retval) = &dict_hashed_vector; DICT_LANGUAGE (retval) = language_def (language); - /* Calculate the number of symbols, and allocate space for them. */ - for (list_counter = symbol_list; - list_counter != NULL; - list_counter = list_counter->next) - { - nsyms += list_counter->nsyms; - } - nbuckets = DICT_HASHTABLE_SIZE (nsyms); + /* Allocate space for symbols. */ + int nsyms = symbol_list.size (); + int nbuckets = DICT_HASHTABLE_SIZE (nsyms); DICT_HASHED_NBUCKETS (retval) = nbuckets; - buckets = XOBNEWVEC (obstack, struct symbol *, nbuckets); + struct symbol **buckets = XOBNEWVEC (obstack, struct symbol *, nbuckets); memset (buckets, 0, nbuckets * sizeof (struct symbol *)); DICT_HASHED_BUCKETS (retval) = buckets; /* Now fill the buckets. */ - for (list_counter = symbol_list; - list_counter != NULL; - list_counter = list_counter->next) - { - for (i = list_counter->nsyms - 1; i >= 0; --i) - { - insert_symbol_hashed (retval, list_counter->symbol[i]); - } - } + for (const auto &sym : symbol_list) + insert_symbol_hashed (retval, sym); return retval; } /* See dictionary.h. */ +struct dictionary * +dict_create_hashed (struct obstack *obstack, + enum language language, + const struct pending *symbol_list) +{ + std::vector<symbol *> symlist = pending_to_vector (symbol_list); + + return dict_create_hashed_1 (obstack, language, symlist); +} + +/* See dictionary.h. */ + extern struct dictionary * dict_create_hashed_expandable (enum language language) { @@ -403,52 +417,45 @@ dict_create_hashed_expandable (enum language language) return retval; } -/* See dictionary.h. */ +/* A function to transition dict_create_linear to new API. */ -struct dictionary * -dict_create_linear (struct obstack *obstack, - enum language language, - const struct pending *symbol_list) +static struct dictionary * +dict_create_linear_1 (struct obstack *obstack, + enum language language, + const std::vector<symbol *> &symbol_list) { - struct dictionary *retval; - int nsyms = 0, i, j; - struct symbol **syms; - const struct pending *list_counter; - - retval = XOBNEW (obstack, struct dictionary); + struct dictionary *retval = XOBNEW (obstack, struct dictionary); DICT_VECTOR (retval) = &dict_linear_vector; DICT_LANGUAGE (retval) = language_def (language); - /* Calculate the number of symbols, and allocate space for them. */ - for (list_counter = symbol_list; - list_counter != NULL; - list_counter = list_counter->next) - { - nsyms += list_counter->nsyms; - } + /* Allocate space for symbols. */ + int nsyms = symbol_list.size (); DICT_LINEAR_NSYMS (retval) = nsyms; - syms = XOBNEWVEC (obstack, struct symbol *, nsyms ); + struct symbol **syms = XOBNEWVEC (obstack, struct symbol *, nsyms); DICT_LINEAR_SYMS (retval) = syms; - /* Now fill in the symbols. Start filling in from the back, so as - to preserve the original order of the symbols. */ - for (list_counter = symbol_list, j = nsyms - 1; - list_counter != NULL; - list_counter = list_counter->next) - { - for (i = list_counter->nsyms - 1; - i >= 0; - --i, --j) - { - syms[j] = list_counter->symbol[i]; - } - } + /* Now fill in the symbols. */ + int idx = nsyms - 1; + for (const auto &sym : symbol_list) + syms[idx--] = sym; return retval; } /* See dictionary.h. */ +struct dictionary * +dict_create_linear (struct obstack *obstack, + enum language language, + const struct pending *symbol_list) +{ + std::vector<symbol *> symlist = pending_to_vector (symbol_list); + + return dict_create_linear_1 (obstack, language, symlist); +} + +/* See dictionary.h. */ + struct dictionary * dict_create_linear_expandable (enum language language) { @@ -483,20 +490,26 @@ dict_add_symbol (struct dictionary *dict, struct symbol *sym) (DICT_VECTOR (dict))->add_symbol (dict, sym); } +/* A function to transition dict_add_pending to new API. */ + +static void +dict_add_pending_1 (struct dictionary *dict, + const std::vector<symbol *> &symbol_list) +{ + /* Preserve ordering by reversing the list. */ + for (auto sym = symbol_list.rbegin (); sym != symbol_list.rend (); ++sym) + dict_add_symbol (dict, *sym); +} + /* Utility to add a list of symbols to a dictionary. DICT must be an expandable dictionary. */ void dict_add_pending (struct dictionary *dict, const struct pending *symbol_list) { - const struct pending *list; - int i; + std::vector<symbol *> symlist = pending_to_vector (symbol_list); - for (list = symbol_list; list != NULL; list = list->next) - { - for (i = 0; i < list->nsyms; ++i) - dict_add_symbol (dict, list->symbol[i]); - } + dict_add_pending_1 (dict, symlist); } /* Initialize ITERATOR to point at the first symbol in DICT, and @@ -929,3 +942,408 @@ add_symbol_linear_expandable (struct dictionary *dict, DICT_LINEAR_SYM (dict, nsyms - 1) = sym; } + +/* Multi-language dictionary support. */ + +/* The structure describing a multi-language dictionary. */ + +struct multidictionary +{ + /* An array of dictionaries, one per language. All dictionaries + must be of the same type. This should be free'd for expandable + dictionary types. */ + struct dictionary **dictionaries; + + /* The number of language dictionaries currently allocated. + Only used for expandable dictionaries. */ + unsigned short n_allocated_dictionaries; +}; + +/* A hasher for enum language. Injecting this into std is a convenience + when using unordered_map with C++11. */ + +namespace std +{ + template<> struct hash<enum language> + { + typedef enum language argument_type; + typedef std::size_t result_type; + + result_type operator() (const argument_type &l) const noexcept + { + return static_cast<result_type> (l); + } + }; +} /* namespace std */ + +/* A helper function to collate symbols on the pending list by language. */ + +static std::unordered_map<enum language, std::vector<symbol *>> +collate_pending_symbols_by_language (const struct pending *symbol_list) +{ + std::unordered_map<enum language, std::vector<symbol *>> nsyms; + + for (const struct pending *list_counter = symbol_list; + list_counter != nullptr; list_counter = list_counter->next) + { + for (int i = list_counter->nsyms - 1; i >= 0; --i) + { + enum language language = SYMBOL_LANGUAGE (list_counter->symbol[i]); + nsyms[language].push_back (list_counter->symbol[i]); + } + } + + return nsyms; +} + +/* See dictionary.h. */ + +struct multidictionary * +mdict_create_hashed (struct obstack *obstack, + const struct pending *symbol_list) +{ + struct multidictionary *retval + = XOBNEW (obstack, struct multidictionary); + std::unordered_map<enum language, std::vector<symbol *>> nsyms + = collate_pending_symbols_by_language (symbol_list); + + /* Loop over all languages and create/populate dictionaries. */ + retval->dictionaries + = XOBNEWVEC (obstack, struct dictionary *, nsyms.size ()); + retval->n_allocated_dictionaries = nsyms.size (); + + int idx = 0; + for (const auto &pair : nsyms) + { + enum language language = pair.first; + std::vector<symbol *> symlist = pair.second; + + retval->dictionaries[idx++] + = dict_create_hashed_1 (obstack, language, symlist); + } + + return retval; +} + +/* See dictionary.h. */ + +struct multidictionary * +mdict_create_hashed_expandable (enum language language) +{ + struct multidictionary *retval = XNEW (struct multidictionary); + + /* We have no symbol list to populate, but we create an empty + dictionary of the requested language to populate later. */ + retval->n_allocated_dictionaries = 1; + retval->dictionaries = XNEW (struct dictionary *); + retval->dictionaries[0] = dict_create_hashed_expandable (language); + + return retval; +} + +/* See dictionary.h. */ + +struct multidictionary * +mdict_create_linear (struct obstack *obstack, + const struct pending *symbol_list) +{ + struct multidictionary *retval + = XOBNEW (obstack, struct multidictionary); + std::unordered_map<enum language, std::vector<symbol *>> nsyms + = collate_pending_symbols_by_language (symbol_list); + + /* Loop over all languages and create/populate dictionaries. */ + retval->dictionaries + = XOBNEWVEC (obstack, struct dictionary *, nsyms.size ()); + retval->n_allocated_dictionaries = nsyms.size (); + + int idx = 0; + for (const auto &pair : nsyms) + { + enum language language = pair.first; + std::vector<symbol *> symlist = pair.second; + + retval->dictionaries[idx++] + = dict_create_linear_1 (obstack, language, symlist); + } + + return retval; +} + +/* See dictionary.h. */ + +struct multidictionary * +mdict_create_linear_expandable (enum language language) +{ + struct multidictionary *retval = XNEW (struct multidictionary); + + /* We have no symbol list to populate, but we create an empty + dictionary to populate later. */ + retval->n_allocated_dictionaries = 1; + retval->dictionaries = XNEW (struct dictionary *); + retval->dictionaries[0] = dict_create_linear_expandable (language); + + return retval; +} + +/* See dictionary.h. */ + +void +mdict_free (struct multidictionary *mdict) +{ + /* Grab the type of dictionary being used. */ + enum dict_type type = mdict->dictionaries[0]->vector->type; + + /* Loop over all dictionaries and free them. */ + for (unsigned short idx = 0; idx < mdict->n_allocated_dictionaries; ++idx) + dict_free (mdict->dictionaries[idx]); + + /* Free the dictionary list, if needed. */ + switch (type) + { + case DICT_HASHED: + case DICT_LINEAR: + /* Memory was allocated on an obstack when created. */ + break; + + case DICT_HASHED_EXPANDABLE: + case DICT_LINEAR_EXPANDABLE: + xfree (mdict->dictionaries); + break; + } +} + +/* Helper function to find the dictionary associated with LANGUAGE + or NULL if there is no dictionary of that language. */ + +static struct dictionary * +find_language_dictionary (const struct multidictionary *mdict, + enum language language) +{ + for (unsigned short idx = 0; idx < mdict->n_allocated_dictionaries; ++idx) + { + if (DICT_LANGUAGE (mdict->dictionaries[idx])->la_language == language) + return mdict->dictionaries[idx]; + } + + return nullptr; +} + +/* Create a new language dictionary for LANGUAGE and add it to the + multidictionary MDICT's list of dictionaries. If MDICT is not + based on expandable dictionaries, this function throws an + internal error. */ + +static struct dictionary * +create_new_language_dictionary (struct multidictionary *mdict, + enum language language) +{ + struct dictionary *retval = nullptr; + + /* We use the first dictionary entry to decide what create function + to call. Not optimal but sufficient. */ + gdb_assert (mdict->dictionaries[0] != nullptr); + switch (mdict->dictionaries[0]->vector->type) + { + case DICT_HASHED: + case DICT_LINEAR: + internal_error (__FILE__, __LINE__, + _("create_new_language_dictionary: attempted to expand " + "non-expandable multidictionary")); + + case DICT_HASHED_EXPANDABLE: + retval = dict_create_hashed_expandable (language); + break; + + case DICT_LINEAR_EXPANDABLE: + retval = dict_create_linear_expandable (language); + break; + } + + /* Grow the dictionary vector and save the new dictionary. */ + mdict->dictionaries + = (struct dictionary **) xrealloc (mdict->dictionaries, + (++mdict->n_allocated_dictionaries + * sizeof (struct dictionary *))); + mdict->dictionaries[mdict->n_allocated_dictionaries - 1] = retval; + + return retval; +} + +/* See dictionary.h. */ + +void +mdict_add_symbol (struct multidictionary *mdict, struct symbol *sym) +{ + struct dictionary *dict + = find_language_dictionary (mdict, SYMBOL_LANGUAGE (sym)); + + if (dict == nullptr) + { + /* SYM is of a new language that we haven't previously seen. + Create a new dictionary for it. */ + dict = create_new_language_dictionary (mdict, SYMBOL_LANGUAGE (sym)); + } + + dict_add_symbol (dict, sym); +} + +/* See dictionary.h. */ + +void +mdict_add_pending (struct multidictionary *mdict, + const struct pending *symbol_list) +{ + std::unordered_map<enum language, std::vector<symbol *>> nsyms + = collate_pending_symbols_by_language (symbol_list); + + for (const auto &pair : nsyms) + { + enum language language = pair.first; + std::vector<symbol *> symlist = pair.second; + struct dictionary *dict = find_language_dictionary (mdict, language); + + if (dict == nullptr) + { + /* The language was not previously seen. Create a new dictionary + for it. */ + dict = create_new_language_dictionary (mdict, language); + } + + dict_add_pending_1 (dict, symlist); + } +} + +/* See dictionary.h. */ + +struct symbol * +mdict_iterator_first (const multidictionary *mdict, + struct mdict_iterator *miterator) +{ + miterator->mdict = mdict; + miterator->current_idx = 0; + + for (unsigned short idx = miterator->current_idx; + idx < mdict->n_allocated_dictionaries; ++idx) + { + struct symbol *result + = dict_iterator_first (mdict->dictionaries[idx], &miterator->iterator); + + if (result != nullptr) + { + miterator->current_idx = idx; + return result; + } + } + + return nullptr; +} + +/* See dictionary.h. */ + +struct symbol * +mdict_iterator_next (struct mdict_iterator *miterator) +{ + struct symbol *result = dict_iterator_next (&miterator->iterator); + + if (result != nullptr) + return result; + + /* The current dictionary had no matches -- move to the next + dictionary, if any. */ + for (unsigned short idx = ++miterator->current_idx; + idx < miterator->mdict->n_allocated_dictionaries; ++idx) + { + result + = dict_iterator_first (miterator->mdict->dictionaries[idx], + &miterator->iterator); + if (result != nullptr) + { + miterator->current_idx = idx; + return result; + } + } + + return nullptr; +} + +/* See dictionary.h. */ + +struct symbol * +mdict_iter_match_first (const struct multidictionary *mdict, + const lookup_name_info &name, + struct mdict_iterator *miterator) +{ + miterator->mdict = mdict; + miterator->current_idx = 0; + + for (unsigned short idx = miterator->current_idx; + idx < mdict->n_allocated_dictionaries; ++idx) + { + struct symbol *result + = dict_iter_match_first (mdict->dictionaries[idx], name, + &miterator->iterator); + + if (result != nullptr) + return result; + } + + return nullptr; +} + +/* See dictionary.h. */ + +struct symbol * +mdict_iter_match_next (const lookup_name_info &name, + struct mdict_iterator *miterator) +{ + /* Search the current dictionary. */ + struct symbol *result = dict_iter_match_next (name, &miterator->iterator); + + if (result != nullptr) + return result; + + /* The current dictionary had no matches -- move to the next + dictionary, if any. */ + for (unsigned short idx = ++miterator->current_idx; + idx < miterator->mdict->n_allocated_dictionaries; ++idx) + { + result + = dict_iter_match_first (miterator->mdict->dictionaries[idx], + name, &miterator->iterator); + if (result != nullptr) + { + miterator->current_idx = idx; + return result; + } + } + + return nullptr; +} + +/* See dictionary.h. */ + +int +mdict_size (const struct multidictionary *mdict) +{ + int size = 0; + + for (unsigned short idx = 0; idx < mdict->n_allocated_dictionaries; ++idx) + size += dict_size (mdict->dictionaries[idx]); + + return size; +} + +/* See dictionary.h. */ + +bool +mdict_empty (const struct multidictionary *mdict) +{ + for (unsigned short idx = 0; idx < mdict->n_allocated_dictionaries; ++idx) + { + if (!dict_empty (mdict->dictionaries[idx])) + return false; + } + + return true; +} diff --git a/gdb/dictionary.h b/gdb/dictionary.h index c80a4b4fe2..d9c4ad491c 100644 --- a/gdb/dictionary.h +++ b/gdb/dictionary.h @@ -113,6 +113,21 @@ struct dict_iterator struct symbol *current; }; +/* The multi-language dictionary iterator. Like dict_iterator above, + these contents should be considered private. */ + +struct mdict_iterator +{ + /* The multidictionary with whcih this iterator is associated. */ + const struct multidictionary *mdict; + + /* The iterator used to iterate through individual dictionaries. */ + struct dict_iterator iterator; + + /* The current index of the dictionary being iterated over. */ + unsigned short current_idx; +}; + /* Initialize ITERATOR to point at the first symbol in DICT, and return that first symbol, or NULL if DICT is empty. */ -- 2.17.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list 2018-11-09 2:28 [PATCH 1/5] gdb/23712: Introduce multidictionary's Keith Seitz @ 2018-11-09 2:28 ` Keith Seitz 2018-11-30 20:58 ` Tom Tromey 2018-11-09 2:28 ` [PATCH 3/5] gdb/23712: Cleanup/Remove temporary dictionary functions Keith Seitz ` (3 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Keith Seitz @ 2018-11-09 2:28 UTC (permalink / raw) To: gdb-patches Finally, we can remove dw2_add_symbol_to_list since the wrapper function originally introduced to catch this multi-language scenario is no longer needed. With multi-language dictionaries, we can now support adding symbols of multiple languages, negating the need for the assertion entirely. This patch should now fix gdb/23712 (and symtab/23010). At least it will if the NULL buildsym_compunit problem doesn't strike first (see gdb/23773). gdb/ChangeLog: PR gdb/23712 * dwarf2read.c (dw2_add_symbol_to_list): Remove. (fixup_go_packaging, new_symbol): Use add_symbol_to_list. --- gdb/ChangeLog | 6 ++++++ gdb/dwarf2read.c | 25 ++++--------------------- 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 32c58ed1e1..0534f7843a 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,9 @@ +YYYY-MM-DD Keith Seitz <keiths@redhat.com> + + PR gdb/23712 + * dwarf2read.c (dw2_add_symbol_to_list): Remove. + (fixup_go_packaging, new_symbol): Use add_symbol_to_list. + YYYY-MM-DD Keith Seitz <keiths@redhat.com> PR gdb/23712 diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index d2a8cd44f9..e0676d7777 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -9788,23 +9788,6 @@ compute_delayed_physnames (struct dwarf2_cu *cu) cu->method_list.clear (); } -/* A wrapper for add_symbol_to_list to ensure that SYMBOL's language is - the same as all other symbols in LISTHEAD. If a new symbol is added - with a different language, this function asserts. */ - -static inline void -dw2_add_symbol_to_list (struct symbol *symbol, struct pending **listhead) -{ - /* Only assert if LISTHEAD already contains symbols of a different - language (dict_create_hashed/insert_symbol_hashed requires that all - symbols in this list are of the same language). */ - gdb_assert ((*listhead) == NULL - || (SYMBOL_LANGUAGE ((*listhead)->symbol[0]) - == SYMBOL_LANGUAGE (symbol))); - - add_symbol_to_list (symbol, listhead); -} - /* Go objects should be embedded in a DW_TAG_module DIE, and it's not clear if/how imported objects will appear. To keep Go support simple until that's worked out, @@ -9878,7 +9861,7 @@ fixup_go_packaging (struct dwarf2_cu *cu) SYMBOL_ACLASS_INDEX (sym) = LOC_TYPEDEF; SYMBOL_TYPE (sym) = type; - dw2_add_symbol_to_list (sym, cu->builder->get_global_symbols ()); + add_symbol_to_list (sym, cu->builder->get_global_symbols ()); xfree (package_name); } @@ -21437,7 +21420,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu, SYMBOL_TYPE (sym) = objfile_type (objfile)->builtin_core_addr; SYMBOL_DOMAIN (sym) = LABEL_DOMAIN; SYMBOL_ACLASS_INDEX (sym) = LOC_LABEL; - dw2_add_symbol_to_list (sym, cu->list_in_scope); + add_symbol_to_list (sym, cu->list_in_scope); break; case DW_TAG_subprogram: /* SYMBOL_BLOCK_VALUE (sym) will be filled in later by @@ -21706,7 +21689,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu, case DW_TAG_common_block: SYMBOL_ACLASS_INDEX (sym) = LOC_COMMON_BLOCK; SYMBOL_DOMAIN (sym) = COMMON_BLOCK_DOMAIN; - dw2_add_symbol_to_list (sym, cu->list_in_scope); + add_symbol_to_list (sym, cu->list_in_scope); break; default: /* Not a tag we recognize. Hopefully we aren't processing @@ -21726,7 +21709,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu, } if (list_to_add != NULL) - dw2_add_symbol_to_list (sym, list_to_add); + add_symbol_to_list (sym, list_to_add); /* For the benefit of old versions of GCC, check for anonymous namespaces based on the demangled name. */ -- 2.17.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list 2018-11-09 2:28 ` [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list Keith Seitz @ 2018-11-30 20:58 ` Tom Tromey 0 siblings, 0 replies; 15+ messages in thread From: Tom Tromey @ 2018-11-30 20:58 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> Finally, we can remove dw2_add_symbol_to_list since the wrapper function Keith> originally introduced to catch this multi-language scenario is no longer Keith> needed. With multi-language dictionaries, we can now support adding Keith> symbols of multiple languages, negating the need for the assertion Keith> entirely. Keith> This patch should now fix gdb/23712 (and symtab/23010). At least it will Keith> if the NULL buildsym_compunit problem doesn't strike first (see gdb/23773). Keith> gdb/ChangeLog: Keith> PR gdb/23712 Keith> * dwarf2read.c (dw2_add_symbol_to_list): Remove. Keith> (fixup_go_packaging, new_symbol): Use add_symbol_to_list. Thanks, this is ok. Maybe the ChangeLog should mention symtab/23010? Tom ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/5] gdb/23712: Cleanup/Remove temporary dictionary functions 2018-11-09 2:28 [PATCH 1/5] gdb/23712: Introduce multidictionary's Keith Seitz 2018-11-09 2:28 ` [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list Keith Seitz @ 2018-11-09 2:28 ` Keith Seitz 2018-11-30 21:26 ` Tom Tromey 2018-11-09 2:28 ` [PATCH 2/5] gdb/23712: Use new multidictionary API Keith Seitz ` (2 subsequent siblings) 4 siblings, 1 reply; 15+ messages in thread From: Keith Seitz @ 2018-11-09 2:28 UTC (permalink / raw) To: gdb-patches Now that multidictionary's are being used, there is no longer any need to retain the four temporary functions introduced in the beginning of this series. This patch removes them. As an additional cleanup, since the single-language dictionaries are no longer used outside dictionary.c, make all of those functions static. gdb/ChangeLog: PR gdb/23712 * dictionary.c (pending_to_vector): Remove. (dict_create_hashed_1, dict_create_linear_1, dict_add_pending_1): Remove _1 suffix, replacing functions of the same name. Update all callers. (dict_create_hashed, dict_create_hashed_expandable) (dict_create_linear, dict_create_linear_expandable, dict_free) (dict_add_symbol, dict_add_pending, dict_size, dict_empty): Make functions static. --- gdb/ChangeLog | 12 ++++++ gdb/dictionary.c | 97 ++++++++++++------------------------------------ 2 files changed, 35 insertions(+), 74 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 27e08d30cf..32c58ed1e1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,15 @@ +YYYY-MM-DD Keith Seitz <keiths@redhat.com> + + PR gdb/23712 + * dictionary.c (pending_to_vector): Remove. + (dict_create_hashed_1, dict_create_linear_1, dict_add_pending_1): + Remove _1 suffix, replacing functions of the same name. Update + all callers. + (dict_create_hashed, dict_create_hashed_expandable) + (dict_create_linear, dict_create_linear_expandable, dict_free) + (dict_add_symbol, dict_add_pending, dict_size, dict_empty): + Make functions static. + YYYY-MM-DD Keith Seitz <keiths@redhat.com> PR gdb/23712 diff --git a/gdb/dictionary.c b/gdb/dictionary.c index 83d8126682..2aedc480e9 100644 --- a/gdb/dictionary.c +++ b/gdb/dictionary.c @@ -342,31 +342,14 @@ static void insert_symbol_hashed (struct dictionary *dict, static void expand_hashtable (struct dictionary *dict); -/* A function to convert a linked list into a vector. */ - -static std::vector<symbol *> -pending_to_vector (const struct pending *symbol_list) -{ - std::vector<symbol *> symlist; - - for (const struct pending *list_counter = symbol_list; - list_counter != nullptr; list_counter = list_counter->next) - { - for (int i = list_counter->nsyms - 1; i >= 0; --i) - symlist.push_back (list_counter->symbol[i]); - } - - return symlist; -} - /* The creation functions. */ -/* A function to transition dict_create_hashed to new API. */ +/* Create a hashed dictionary of a given language. */ static struct dictionary * -dict_create_hashed_1 (struct obstack *obstack, - enum language language, - const std::vector<symbol *> &symbol_list) +dict_create_hashed (struct obstack *obstack, + enum language language, + const std::vector<symbol *> &symbol_list) { /* Allocate the dictionary. */ struct dictionary *retval = XOBNEW (obstack, struct dictionary); @@ -388,21 +371,9 @@ dict_create_hashed_1 (struct obstack *obstack, return retval; } -/* See dictionary.h. */ - -struct dictionary * -dict_create_hashed (struct obstack *obstack, - enum language language, - const struct pending *symbol_list) -{ - std::vector<symbol *> symlist = pending_to_vector (symbol_list); - - return dict_create_hashed_1 (obstack, language, symlist); -} +/* Create an expandable hashed dictionary of a given language. */ -/* See dictionary.h. */ - -extern struct dictionary * +static struct dictionary * dict_create_hashed_expandable (enum language language) { struct dictionary *retval = XNEW (struct dictionary); @@ -417,12 +388,12 @@ dict_create_hashed_expandable (enum language language) return retval; } -/* A function to transition dict_create_linear to new API. */ +/* Create a linear dictionary of a given language. */ static struct dictionary * -dict_create_linear_1 (struct obstack *obstack, - enum language language, - const std::vector<symbol *> &symbol_list) +dict_create_linear (struct obstack *obstack, + enum language language, + const std::vector<symbol *> &symbol_list) { struct dictionary *retval = XOBNEW (obstack, struct dictionary); DICT_VECTOR (retval) = &dict_linear_vector; @@ -442,21 +413,9 @@ dict_create_linear_1 (struct obstack *obstack, return retval; } -/* See dictionary.h. */ - -struct dictionary * -dict_create_linear (struct obstack *obstack, - enum language language, - const struct pending *symbol_list) -{ - std::vector<symbol *> symlist = pending_to_vector (symbol_list); - - return dict_create_linear_1 (obstack, language, symlist); -} - -/* See dictionary.h. */ +/* Create an expandable linear dictionary of a given language. */ -struct dictionary * +static struct dictionary * dict_create_linear_expandable (enum language language) { struct dictionary *retval = XNEW (struct dictionary); @@ -476,7 +435,7 @@ dict_create_linear_expandable (enum language language) /* Free the memory used by a dictionary that's not on an obstack. (If any.) */ -void +static void dict_free (struct dictionary *dict) { (DICT_VECTOR (dict))->free (dict); @@ -484,34 +443,24 @@ dict_free (struct dictionary *dict) /* Add SYM to DICT. DICT had better be expandable. */ -void +static void dict_add_symbol (struct dictionary *dict, struct symbol *sym) { (DICT_VECTOR (dict))->add_symbol (dict, sym); } -/* A function to transition dict_add_pending to new API. */ +/* Utility to add a list of symbols to a dictionary. + DICT must be an expandable dictionary. */ static void -dict_add_pending_1 (struct dictionary *dict, - const std::vector<symbol *> &symbol_list) +dict_add_pending (struct dictionary *dict, + const std::vector<symbol *> &symbol_list) { /* Preserve ordering by reversing the list. */ for (auto sym = symbol_list.rbegin (); sym != symbol_list.rend (); ++sym) dict_add_symbol (dict, *sym); } -/* Utility to add a list of symbols to a dictionary. - DICT must be an expandable dictionary. */ - -void -dict_add_pending (struct dictionary *dict, const struct pending *symbol_list) -{ - std::vector<symbol *> symlist = pending_to_vector (symbol_list); - - dict_add_pending_1 (dict, symlist); -} - /* Initialize ITERATOR to point at the first symbol in DICT, and return that first symbol, or NULL if DICT is empty. */ @@ -548,7 +497,7 @@ dict_iter_match_next (const lookup_name_info &name, ->iter_match_next (name, iterator); } -int +static int dict_size (const struct dictionary *dict) { return (DICT_VECTOR (dict))->size (dict); @@ -560,7 +509,7 @@ dict_size (const struct dictionary *dict) /* Test to see if DICT is empty. */ -int +static int dict_empty (struct dictionary *dict) { struct dict_iterator iter; @@ -1019,7 +968,7 @@ mdict_create_hashed (struct obstack *obstack, std::vector<symbol *> symlist = pair.second; retval->dictionaries[idx++] - = dict_create_hashed_1 (obstack, language, symlist); + = dict_create_hashed (obstack, language, symlist); } return retval; @@ -1064,7 +1013,7 @@ mdict_create_linear (struct obstack *obstack, std::vector<symbol *> symlist = pair.second; retval->dictionaries[idx++] - = dict_create_linear_1 (obstack, language, symlist); + = dict_create_linear (obstack, language, symlist); } return retval; @@ -1210,7 +1159,7 @@ mdict_add_pending (struct multidictionary *mdict, dict = create_new_language_dictionary (mdict, language); } - dict_add_pending_1 (dict, symlist); + dict_add_pending (dict, symlist); } } -- 2.17.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/5] gdb/23712: Cleanup/Remove temporary dictionary functions 2018-11-09 2:28 ` [PATCH 3/5] gdb/23712: Cleanup/Remove temporary dictionary functions Keith Seitz @ 2018-11-30 21:26 ` Tom Tromey 0 siblings, 0 replies; 15+ messages in thread From: Tom Tromey @ 2018-11-30 21:26 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> Now that multidictionary's are being used, there is no longer any need Keith> to retain the four temporary functions introduced in the beginning of Keith> this series. Keith> This patch removes them. Thanks, this is ok. Tom ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/5] gdb/23712: Use new multidictionary API 2018-11-09 2:28 [PATCH 1/5] gdb/23712: Introduce multidictionary's Keith Seitz 2018-11-09 2:28 ` [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list Keith Seitz 2018-11-09 2:28 ` [PATCH 3/5] gdb/23712: Cleanup/Remove temporary dictionary functions Keith Seitz @ 2018-11-09 2:28 ` Keith Seitz 2018-11-30 21:22 ` Tom Tromey 2018-11-09 2:34 ` [PATCH 5/5] gdb/23712: Test case for multidictionary Keith Seitz 2018-12-16 17:01 ` [PATCH 1/5] gdb/23712: Introduce multidictionary's Tom Tromey 4 siblings, 1 reply; 15+ messages in thread From: Keith Seitz @ 2018-11-09 2:28 UTC (permalink / raw) To: gdb-patches This patch builds on the previous by enabling the `new' multidictionary API. A lot of the hunks are simply textual replacements of "dict_" with "mdict_" and similar transformations. A word of warning, even with the use of multidictionaries, the code still does not satisfactorily fix the reported problems with gdb/23712 (or gdb/23010). We still have additional changes to make before that happens. gdb/ChangeLog: PR gdb/23712 * dictionary.h (struct dictionary): Replace declaration with multidictionary. (dict_create_hashed, dict_create_hashed_expandable) (dict_create_linear, dict_create_linear_expandable) (dict_free, dict_add_symbol, dict_add_pending, dict_empty) (dict_iterator_first, dict_iterator_next, dict_iter_match_first) (dict_iter_match_next, dict_size): Rename to "mdict_" versions taking multidictionary argument. [ALL_DICT_SYMBOLS]: Update for multidictionary. * block.h (struct block) <dict>: Change to multidictionary and rename `multidict'. * block.c, buildsym.c, jit.c, mdebugread.c, objfiles.c, symmisc.c: Update all dictionary references to multidictionary. --- gdb/ChangeLog | 17 +++++++ gdb/block.c | 31 ++++++------ gdb/block.h | 8 ++-- gdb/buildsym.c | 28 ++++++----- gdb/dictionary.h | 119 ++++++++++++++++++++++++----------------------- gdb/jit.c | 10 ++-- gdb/mdebugread.c | 8 ++-- gdb/objfiles.c | 4 +- gdb/symmisc.c | 6 +-- 9 files changed, 124 insertions(+), 107 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index ccf5dbdf88..27e08d30cf 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,20 @@ +YYYY-MM-DD Keith Seitz <keiths@redhat.com> + + PR gdb/23712 + * dictionary.h: Replace declaration of dictionary with + multidictionary. + (dict_create_hashed, dict_create_hashed_expandable) + (dict_create_linear, dict_create_linear_expandable) + (dict_free, dict_add_symbol, dict_add_pending, dict_empty) + (dict_iterator_first, dict_iterator_next, dict_iter_match_first) + (dict_iter_match_next, dict_size): Rename to "mdict_" versions + taking multidictionary argument. + [ALL_DICT_SYMBOLS]: Update for multidictionary. + * block.h (struct block) <dict>: Change to multidictionary + and rename `multidict'. + * block.c, buildsym.c, jit.c, mdebugread.c, objfiles.c, + symmisc.c: Update all dictionary references to multidictionary. + YYYY-MM-DD Keith Seitz <keiths@redhat.com> PR gdb/23712 diff --git a/gdb/block.c b/gdb/block.c index 85e6c618d7..f0dbfbec89 100644 --- a/gdb/block.c +++ b/gdb/block.c @@ -387,9 +387,9 @@ block_global_block (const struct block *block) zero/NULL. This is useful for creating "dummy" blocks that don't correspond to actual source files. - Warning: it sets the block's BLOCK_DICT to NULL, which isn't a + Warning: it sets the block's BLOCK_MULTIDICT to NULL, which isn't a valid value. If you really don't want the block to have a - dictionary, then you should subsequently set its BLOCK_DICT to + dictionary, then you should subsequently set its BLOCK_MULTIDICT to dict_create_linear (obstack, NULL). */ struct block * @@ -544,10 +544,11 @@ block_iterator_step (struct block_iterator *iterator, int first) block = BLOCKVECTOR_BLOCK (COMPUNIT_BLOCKVECTOR (cust), iterator->which); - sym = dict_iterator_first (BLOCK_DICT (block), &iterator->dict_iter); + sym = mdict_iterator_first (BLOCK_MULTIDICT (block), + &iterator->mdict_iter); } else - sym = dict_iterator_next (&iterator->dict_iter); + sym = mdict_iterator_next (&iterator->mdict_iter); if (sym != NULL) return sym; @@ -569,7 +570,7 @@ block_iterator_first (const struct block *block, initialize_block_iterator (block, iterator); if (iterator->which == FIRST_LOCAL_BLOCK) - return dict_iterator_first (block->dict, &iterator->dict_iter); + return mdict_iterator_first (block->multidict, &iterator->mdict_iter); return block_iterator_step (iterator, 1); } @@ -580,7 +581,7 @@ struct symbol * block_iterator_next (struct block_iterator *iterator) { if (iterator->which == FIRST_LOCAL_BLOCK) - return dict_iterator_next (&iterator->dict_iter); + return mdict_iterator_next (&iterator->mdict_iter); return block_iterator_step (iterator, 0); } @@ -612,11 +613,11 @@ block_iter_match_step (struct block_iterator *iterator, block = BLOCKVECTOR_BLOCK (COMPUNIT_BLOCKVECTOR (cust), iterator->which); - sym = dict_iter_match_first (BLOCK_DICT (block), name, - &iterator->dict_iter); + sym = mdict_iter_match_first (BLOCK_MULTIDICT (block), name, + &iterator->mdict_iter); } else - sym = dict_iter_match_next (name, &iterator->dict_iter); + sym = mdict_iter_match_next (name, &iterator->mdict_iter); if (sym != NULL) return sym; @@ -639,7 +640,8 @@ block_iter_match_first (const struct block *block, initialize_block_iterator (block, iterator); if (iterator->which == FIRST_LOCAL_BLOCK) - return dict_iter_match_first (block->dict, name, &iterator->dict_iter); + return mdict_iter_match_first (block->multidict, name, + &iterator->mdict_iter); return block_iter_match_step (iterator, name, 1); } @@ -651,7 +653,7 @@ block_iter_match_next (const lookup_name_info &name, struct block_iterator *iterator) { if (iterator->which == FIRST_LOCAL_BLOCK) - return dict_iter_match_next (name, &iterator->dict_iter); + return mdict_iter_match_next (name, &iterator->mdict_iter); return block_iter_match_step (iterator, name, 0); } @@ -731,7 +733,7 @@ block_lookup_symbol_primary (const struct block *block, const char *name, const domain_enum domain) { struct symbol *sym, *other; - struct dict_iterator dict_iter; + struct mdict_iterator mdict_iter; lookup_name_info lookup_name (name, symbol_name_match_type::FULL); @@ -740,9 +742,10 @@ block_lookup_symbol_primary (const struct block *block, const char *name, || BLOCK_SUPERBLOCK (BLOCK_SUPERBLOCK (block)) == NULL); other = NULL; - for (sym = dict_iter_match_first (block->dict, lookup_name, &dict_iter); + for (sym + = mdict_iter_match_first (block->multidict, lookup_name, &mdict_iter); sym != NULL; - sym = dict_iter_match_next (lookup_name, &dict_iter)) + sym = mdict_iter_match_next (lookup_name, &mdict_iter)) { if (SYMBOL_DOMAIN (sym) == domain) return sym; diff --git a/gdb/block.h b/gdb/block.h index 0050eacf8d..0c78a3af46 100644 --- a/gdb/block.h +++ b/gdb/block.h @@ -111,7 +111,7 @@ struct block /* This is used to store the symbols in the block. */ - struct dictionary *dict; + struct multidictionary *multidict; /* Contains information about namespace-related info relevant to this block: using directives and the current namespace scope. */ @@ -143,7 +143,7 @@ struct global_block #define BLOCK_END(bl) (bl)->endaddr #define BLOCK_FUNCTION(bl) (bl)->function #define BLOCK_SUPERBLOCK(bl) (bl)->superblock -#define BLOCK_DICT(bl) (bl)->dict +#define BLOCK_MULTIDICT(bl) (bl)->multidict #define BLOCK_NAMESPACE(bl) (bl)->namespace_info /* Accessor for ranges field within block BL. */ @@ -298,9 +298,9 @@ struct block_iterator enum block_enum which; - /* The underlying dictionary iterator. */ + /* The underlying multidictionary iterator. */ - struct dict_iterator dict_iter; + struct mdict_iterator mdict_iter; }; /* Initialize ITERATOR to point at the first symbol in BLOCK, and diff --git a/gdb/buildsym.c b/gdb/buildsym.c index a9d2698584..a3a067cccf 100644 --- a/gdb/buildsym.c +++ b/gdb/buildsym.c @@ -227,22 +227,20 @@ buildsym_compunit::finish_block_internal if (symbol) { - BLOCK_DICT (block) - = dict_create_linear (&m_objfile->objfile_obstack, - m_language, *listhead); + BLOCK_MULTIDICT (block) + = mdict_create_linear (&m_objfile->objfile_obstack, *listhead); } else { if (expandable) { - BLOCK_DICT (block) = dict_create_hashed_expandable (m_language); - dict_add_pending (BLOCK_DICT (block), *listhead); + BLOCK_MULTIDICT (block) = mdict_create_hashed_expandable (m_language); + mdict_add_pending (BLOCK_MULTIDICT (block), *listhead); } else { - BLOCK_DICT (block) = - dict_create_hashed (&m_objfile->objfile_obstack, - m_language, *listhead); + BLOCK_MULTIDICT (block) = + mdict_create_hashed (&m_objfile->objfile_obstack, *listhead); } } @@ -254,7 +252,7 @@ buildsym_compunit::finish_block_internal if (symbol) { struct type *ftype = SYMBOL_TYPE (symbol); - struct dict_iterator iter; + struct mdict_iterator miter; SYMBOL_BLOCK_VALUE (symbol) = block; BLOCK_FUNCTION (block) = symbol; @@ -268,7 +266,7 @@ buildsym_compunit::finish_block_internal /* Here we want to directly access the dictionary, because we haven't fully initialized the block yet. */ - ALL_DICT_SYMBOLS (BLOCK_DICT (block), iter, sym) + ALL_DICT_SYMBOLS (BLOCK_MULTIDICT (block), miter, sym) { if (SYMBOL_IS_ARGUMENT (sym)) nparams++; @@ -282,7 +280,7 @@ buildsym_compunit::finish_block_internal iparams = 0; /* Here we want to directly access the dictionary, because we haven't fully initialized the block yet. */ - ALL_DICT_SYMBOLS (BLOCK_DICT (block), iter, sym) + ALL_DICT_SYMBOLS (BLOCK_MULTIDICT (block), miter, sym) { if (iparams == nparams) break; @@ -1067,7 +1065,7 @@ buildsym_compunit::end_symtab_with_blockvector (struct block *static_block, { struct block *block = BLOCKVECTOR_BLOCK (blockvector, block_i); struct symbol *sym; - struct dict_iterator iter; + struct mdict_iterator miter; /* Inlined functions may have symbols not in the global or static symbol lists. */ @@ -1078,7 +1076,7 @@ buildsym_compunit::end_symtab_with_blockvector (struct block *static_block, /* Note that we only want to fix up symbols from the local blocks, not blocks coming from included symtabs. That is why we use ALL_DICT_SYMBOLS here and not ALL_BLOCK_SYMBOLS. */ - ALL_DICT_SYMBOLS (BLOCK_DICT (block), iter, sym) + ALL_DICT_SYMBOLS (BLOCK_MULTIDICT (block), miter, sym) if (symbol_symtab (sym) == NULL) symbol_set_symtab (sym, symtab); } @@ -1212,7 +1210,7 @@ buildsym_compunit::augment_type_symtab () to the primary symtab. */ set_missing_symtab (m_file_symbols, cust); - dict_add_pending (BLOCK_DICT (block), m_file_symbols); + mdict_add_pending (BLOCK_MULTIDICT (block), m_file_symbols); } if (m_global_symbols != NULL) @@ -1223,7 +1221,7 @@ buildsym_compunit::augment_type_symtab () to the primary symtab. */ set_missing_symtab (m_global_symbols, cust); - dict_add_pending (BLOCK_DICT (block), + mdict_add_pending (BLOCK_MULTIDICT (block), m_global_symbols); } } diff --git a/gdb/dictionary.h b/gdb/dictionary.h index d9c4ad491c..ef99a65099 100644 --- a/gdb/dictionary.h +++ b/gdb/dictionary.h @@ -25,10 +25,10 @@ #include "symfile.h" -/* An opaque type for dictionaries; only dictionary.c should know - about its innards. */ +/* An opaque type for multi-language dictionaries; only dictionary.c should + know about its innards. */ -struct dictionary; +struct multidictionary; /* Other types needed for declarations. */ @@ -38,65 +38,64 @@ struct pending; struct language_defn; /* The creation functions for various implementations of - dictionaries. */ + multi-language dictionaries. */ -/* Create a dictionary of symbols of language LANGUAGE implemented via +/* Create a multi-language dictionary of symbols implemented via a fixed-size hashtable. All memory it uses is allocated on OBSTACK; the environment is initialized from SYMBOL_LIST. */ -extern struct dictionary *dict_create_hashed (struct obstack *obstack, - enum language language, - const struct pending - *symbol_list); +extern struct multidictionary * + mdict_create_hashed (struct obstack *obstack, + const struct pending *symbol_list); -/* Create a dictionary of symbols of language LANGUAGE, implemented - via a hashtable that grows as necessary. The dictionary is - initially empty; to add symbols to it, call dict_add_symbol(). - Call dict_free() when you're done with it. */ +/* Create a multi-language dictionary of symbols, implemented + via a hashtable that grows as necessary. The initial dictionary of + LANGUAGE is empty; to add symbols to it, call mdict_add_symbol(). + Call mdict_free() when you're done with it. */ -extern struct dictionary * - dict_create_hashed_expandable (enum language language); +extern struct multidictionary * + mdict_create_hashed_expandable (enum language language); -/* Create a dictionary of symbols of language LANGUAGE, implemented +/* Create a multi-language dictionary of symbols, implemented via a fixed-size array. All memory it uses is allocated on OBSTACK; the environment is initialized from the SYMBOL_LIST. The symbols are ordered in the same order that they're found in SYMBOL_LIST. */ -extern struct dictionary *dict_create_linear (struct obstack *obstack, - enum language language, - const struct pending - *symbol_list); +extern struct multidictionary * + mdict_create_linear (struct obstack *obstack, + const struct pending *symbol_list); -/* Create a dictionary of symbols of language LANGUAGE, implemented - via an array that grows as necessary. The dictionary is initially - empty; to add symbols to it, call dict_add_symbol(). Call - dict_free() when you're done with it. */ +/* Create a multi-language dictionary of symbols, implemented + via an array that grows as necessary. The multidictionary initially + contains a single empty dictionary of LANGUAGE; to add symbols to it, + call mdict_add_symbol(). Call mdict_free() when you're done with it. */ -extern struct dictionary * - dict_create_linear_expandable (enum language language); +extern struct multidictionary * + mdict_create_linear_expandable (enum language language); -/* The functions providing the interface to dictionaries. Note that - the most common parts of the interface, namely symbol lookup, are - only provided via iterator functions. */ +/* The functions providing the interface to multi-language dictionaries. + Note that the most common parts of the interface, namely symbol lookup, + are only provided via iterator functions. */ -/* Free the memory used by a dictionary that's not on an obstack. (If +/* Free the memory used by a multidictionary that's not on an obstack. (If any.) */ -extern void dict_free (struct dictionary *dict); +extern void mdict_free (struct multidictionary *mdict); -/* Add a symbol to an expandable dictionary. */ +/* Add a symbol to an expandable multidictionary. */ -extern void dict_add_symbol (struct dictionary *dict, struct symbol *sym); +extern void mdict_add_symbol (struct multidictionary *mdict, + struct symbol *sym); -/* Utility to add a list of symbols to a dictionary. */ +/* Utility to add a list of symbols to a multidictionary. */ -extern void dict_add_pending (struct dictionary *dict, - const struct pending *symbol_list); +extern void mdict_add_pending (struct multidictionary *mdict, + const struct pending *symbol_list); -/* Is the dictionary empty? */ +/* Is the multidictionary empty? */ -extern int dict_empty (struct dictionary *dict); +extern int mdict_empty (struct multidictionary *mdict); /* A type containing data that is used when iterating over all symbols in a dictionary. Don't ever look at its innards; this type would @@ -128,44 +127,46 @@ struct mdict_iterator unsigned short current_idx; }; -/* Initialize ITERATOR to point at the first symbol in DICT, and - return that first symbol, or NULL if DICT is empty. */ +/* Initialize ITERATOR to point at the first symbol in MDICT, and + return that first symbol, or NULL if MDICT is empty. */ -extern struct symbol *dict_iterator_first (const struct dictionary *dict, - struct dict_iterator *iterator); +extern struct symbol * + mdict_iterator_first (const struct multidictionary *mdict, + struct mdict_iterator *miterator); -/* Advance ITERATOR, and return the next symbol, or NULL if there are +/* Advance MITERATOR, and return the next symbol, or NULL if there are no more symbols. Don't call this if you've previously received - NULL from dict_iterator_first or dict_iterator_next on this + NULL from mdict_iterator_first or mdict_iterator_next on this iteration. */ -extern struct symbol *dict_iterator_next (struct dict_iterator *iterator); +extern struct symbol *mdict_iterator_next (struct mdict_iterator *miterator); -/* Initialize ITERATOR to point at the first symbol in DICT whose +/* Initialize MITERATOR to point at the first symbol in MDICT whose SYMBOL_SEARCH_NAME is NAME, as tested using COMPARE (which must use the same conventions as strcmp_iw and be compatible with any dictionary hashing function), and return that first symbol, or NULL if there are no such symbols. */ -extern struct symbol *dict_iter_match_first (const struct dictionary *dict, - const lookup_name_info &name, - struct dict_iterator *iterator); +extern struct symbol * + mdict_iter_match_first (const struct multidictionary *mdict, + const lookup_name_info &name, + struct mdict_iterator *miterator); -/* Advance ITERATOR to point at the next symbol in DICT whose +/* Advance MITERATOR to point at the next symbol in MDICT whose SYMBOL_SEARCH_NAME is NAME, as tested using COMPARE (see dict_iter_match_first), or NULL if there are no more such symbols. Don't call this if you've previously received NULL from - dict_iterator_match_first or dict_iterator_match_next on this - iteration. And don't call it unless ITERATOR was created by a - previous call to dict_iter_match_first with the same NAME and COMPARE. */ + mdict_iterator_match_first or mdict_iterator_match_next on this + iteration. And don't call it unless MITERATOR was created by a + previous call to mdict_iter_match_first with the same NAME and COMPARE. */ -extern struct symbol *dict_iter_match_next (const lookup_name_info &name, - struct dict_iterator *iterator); +extern struct symbol *mdict_iter_match_next (const lookup_name_info &name, + struct mdict_iterator *miterator); -/* Return some notion of the size of the dictionary: the number of +/* Return some notion of the size of the multidictionary: the number of symbols if we have that, the number of hash buckets otherwise. */ -extern int dict_size (const struct dictionary *dict); +extern int mdict_size (const struct multidictionary *mdict); /* Macro to loop through all symbols in a dictionary DICT, in no particular order. ITER is a struct dict_iterator (NOTE: __not__ a @@ -175,8 +176,8 @@ extern int dict_size (const struct dictionary *dict); early by a break if you desire. */ #define ALL_DICT_SYMBOLS(dict, iter, sym) \ - for ((sym) = dict_iterator_first ((dict), &(iter)); \ + for ((sym) = mdict_iterator_first ((dict), &(iter)); \ (sym); \ - (sym) = dict_iterator_next (&(iter))) + (sym) = mdict_iterator_next (&(iter))) #endif /* DICTIONARY_H */ diff --git a/gdb/jit.c b/gdb/jit.c index e6b3cc25ca..ea067c7b33 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -651,14 +651,12 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile) size_t blockvector_size; CORE_ADDR begin, end; struct blockvector *bv; - enum language language; actual_nblocks = FIRST_LOCAL_BLOCK + stab->nblocks; cust = allocate_compunit_symtab (objfile, stab->file_name); allocate_symtab (cust, stab->file_name); add_compunit_symtab_to_objfile (cust); - language = compunit_language (cust); /* JIT compilers compile in memory. */ COMPUNIT_DIRNAME (cust) = NULL; @@ -702,8 +700,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile) TARGET_CHAR_BIT, "void"); - BLOCK_DICT (new_block) = dict_create_linear (&objfile->objfile_obstack, - language, NULL); + BLOCK_MULTIDICT (new_block) + = mdict_create_linear (&objfile->objfile_obstack, NULL); /* The address range. */ BLOCK_START (new_block) = (CORE_ADDR) gdb_block_iter->begin; BLOCK_END (new_block) = (CORE_ADDR) gdb_block_iter->end; @@ -740,8 +738,8 @@ finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile) new_block = (i == GLOBAL_BLOCK ? allocate_global_block (&objfile->objfile_obstack) : allocate_block (&objfile->objfile_obstack)); - BLOCK_DICT (new_block) = dict_create_linear (&objfile->objfile_obstack, - language, NULL); + BLOCK_MULTIDICT (new_block) + = mdict_create_linear (&objfile->objfile_obstack, NULL); BLOCK_SUPERBLOCK (new_block) = block_iter; block_iter = new_block; diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c index 26687f6c8d..54dba80449 100644 --- a/gdb/mdebugread.c +++ b/gdb/mdebugread.c @@ -4502,7 +4502,7 @@ static void add_symbol (struct symbol *s, struct symtab *symtab, struct block *b) { symbol_set_symtab (s, symtab); - dict_add_symbol (BLOCK_DICT (b), s); + mdict_add_symbol (BLOCK_MULTIDICT (b), s); } /* Add a new block B to a symtab S. */ @@ -4730,7 +4730,7 @@ new_bvect (int nblocks) } /* Allocate and zero a new block of language LANGUAGE, and set its - BLOCK_DICT. If function is non-zero, assume the block is + BLOCK_MULTIDICT. If function is non-zero, assume the block is associated to a function, and make sure that the symbols are stored linearly; otherwise, store them hashed. */ @@ -4743,9 +4743,9 @@ new_block (enum block_type type, enum language language) struct block *retval = XCNEW (struct block); if (type == FUNCTION_BLOCK) - BLOCK_DICT (retval) = dict_create_linear_expandable (language); + BLOCK_MULTIDICT (retval) = mdict_create_linear_expandable (language); else - BLOCK_DICT (retval) = dict_create_hashed_expandable (language); + BLOCK_MULTIDICT (retval) = mdict_create_hashed_expandable (language); return retval; } diff --git a/gdb/objfiles.c b/gdb/objfiles.c index a9b8fa7c58..5e2fc826d5 100644 --- a/gdb/objfiles.c +++ b/gdb/objfiles.c @@ -822,7 +822,7 @@ objfile_relocate1 (struct objfile *objfile, { struct block *b; struct symbol *sym; - struct dict_iterator iter; + struct mdict_iterator miter; b = BLOCKVECTOR_BLOCK (bv, i); BLOCK_START (b) += ANOFFSET (delta, block_line_section); @@ -838,7 +838,7 @@ objfile_relocate1 (struct objfile *objfile, /* We only want to iterate over the local symbols, not any symbols in included symtabs. */ - ALL_DICT_SYMBOLS (BLOCK_DICT (b), iter, sym) + ALL_DICT_SYMBOLS (BLOCK_MULTIDICT (b), miter, sym) { relocate_one_symbol (sym, objfile, delta); } diff --git a/gdb/symmisc.c b/gdb/symmisc.c index d30a35481e..83bd6464da 100644 --- a/gdb/symmisc.c +++ b/gdb/symmisc.c @@ -275,7 +275,7 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile) struct objfile *objfile = SYMTAB_OBJFILE (symtab); struct gdbarch *gdbarch = get_objfile_arch (objfile); int i; - struct dict_iterator iter; + struct mdict_iterator miter; int len; struct linetable *l; const struct blockvector *bv; @@ -331,7 +331,7 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile) even if we're using a hashtable, but nothing else but this message wants it. */ fprintf_filtered (outfile, ", %d syms/buckets in ", - dict_size (BLOCK_DICT (b))); + mdict_size (BLOCK_MULTIDICT (b))); fputs_filtered (paddress (gdbarch, BLOCK_START (b)), outfile); fprintf_filtered (outfile, ".."); fputs_filtered (paddress (gdbarch, BLOCK_END (b)), outfile); @@ -349,7 +349,7 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile) /* Now print each symbol in this block (in no particular order, if we're using a hashtable). Note that we only want this block, not any blocks from included symtabs. */ - ALL_DICT_SYMBOLS (BLOCK_DICT (b), iter, sym) + ALL_DICT_SYMBOLS (BLOCK_MULTIDICT (b), miter, sym) { TRY { -- 2.17.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/5] gdb/23712: Use new multidictionary API 2018-11-09 2:28 ` [PATCH 2/5] gdb/23712: Use new multidictionary API Keith Seitz @ 2018-11-30 21:22 ` Tom Tromey 0 siblings, 0 replies; 15+ messages in thread From: Tom Tromey @ 2018-11-30 21:22 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> This patch builds on the previous by enabling the `new' multidictionary Keith> API. A lot of the hunks are simply textual replacements of "dict_" Keith> with "mdict_" and similar transformations. Keith> A word of warning, even with the use of multidictionaries, the code Keith> still does not satisfactorily fix the reported problems with gdb/23712 Keith> (or gdb/23010). We still have additional changes to make before that Keith> happens. This all seems pretty mechanical AFAICT, and so I think it is ok. Thanks. Tom ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 5/5] gdb/23712: Test case for multidictionary 2018-11-09 2:28 [PATCH 1/5] gdb/23712: Introduce multidictionary's Keith Seitz ` (2 preceding siblings ...) 2018-11-09 2:28 ` [PATCH 2/5] gdb/23712: Use new multidictionary API Keith Seitz @ 2018-11-09 2:34 ` Keith Seitz 2018-11-30 20:59 ` Tom Tromey 2018-12-16 17:01 ` [PATCH 1/5] gdb/23712: Introduce multidictionary's Tom Tromey 4 siblings, 1 reply; 15+ messages in thread From: Keith Seitz @ 2018-11-09 2:34 UTC (permalink / raw) To: gdb-patches This is a test derived from one of the reproducers in symtab/23010. The DIE tree used here is typical of compilations with LTO, where an artificial parent DIE of language C99 imports DIEs of other languages. testsuite/ChangeLog: PR gdb/23712 * gdb.dwarf2/multidictionary.exp: New file. --- gdb/testsuite/ChangeLog | 5 + gdb/testsuite/gdb.dwarf2/multidictionary.exp | 157 +++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 gdb/testsuite/gdb.dwarf2/multidictionary.exp diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index e0bb8382aa..abc13754be 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,8 @@ +YYYY-MM-DD Keith Seitz <keiths@redhat.com> + + PR gdb/23712 + * gdb.dwarf2/multidictionary.exp: New file. + 2018-11-08 Jan Beulich <jbeulich@suse.com> * testsuite/gdb.arch/i386-avx512.c, diff --git a/gdb/testsuite/gdb.dwarf2/multidictionary.exp b/gdb/testsuite/gdb.dwarf2/multidictionary.exp new file mode 100644 index 0000000000..5e49327a4b --- /dev/null +++ b/gdb/testsuite/gdb.dwarf2/multidictionary.exp @@ -0,0 +1,157 @@ +# Copyright 2018 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# A test of multi-language dictionaries, a solution to symtab/23010 et al. + +load_lib dwarf.exp + +# This test can only be run on targets which support DWARF. +if {![dwarf2_support]} { + return 0 +} + +standard_testfile main.c .S + +# Create the DWARF. This is derived from the reproducer in the bug +# mentioned above. This DIE tree is typical of compilations wtih +# LTO enabled. + +set asm_file [standard_output_file $srcfile2] +Dwarf::assemble $asm_file { + declare_labels D45d9 D5079 D5080 D50a9 D50af D5ab2 D5ac2 D5ace D5acf + declare_labels D2135f D2216a D22171 D226c4 D226ca D244ca \ + D245da D245e6 + declare_labels D41c21 D42025 D42045 D42038 D42045 D420b5 + + cu {} { + D45d9: compile_unit { + {language @DW_LANG_C_plus_plus} + {name "SerialPortUtils.cpp"} + } { + D5079: base_type { + {byte_size 1 sdata} + {encoding @DW_ATE_unsigned} + {name "char"} + } + + D5080: const_type { + {type :$D5079} + } + + D50a9: pointer_type { + {byte_size 4 sdata} + {type :$D5080} + } + + D50af: const_type { + {type :$D50a9} + } + + D5ab2: subprogram { + {external 1 flag} + {linkage_name "_Z18SerialSyncWriteStrPKc"} + } { + D5ac2: formal_parameter { + {name "msg"} + {type :$D50af} + } + D5ace: lexical_block {} { + D5acf: DW_TAG_variable { + {name "p"} + {type :$D50a9} + } + } + } + } + } + + cu {} { + D2135f: compile_unit { + {language @DW_LANG_C_plus_plus} + {name "Main.cpp"} + } { + D2216a: base_type { + {byte_size 1 sdata} + {encoding @DW_ATE_unsigned_char} + {name "char"} + } + + D22171: const_type { + {type :$D2216a} + } + + D226c4: pointer_type { + {byte_size 4 sdata} + {type :$D22171} + } + + D226ca: const_type { + {type :$D226c4} + } + + D245da: subprogram { + {name "PrintPanicMsg"} + } { + D245e6: formal_parameter { + {name "msg"} + {type :$D226ca} + } + } + } + } + + cu {} { + D41c21: compile_unit { + {language @DW_LANG_C99} + {name "<artificial>"} + } { + D42025: subprogram { + {abstract_origin %$D245da} + {low_pc 0x80b60 addr} + {high_pc 0x6c data4} + } { + D42038: formal_parameter { + {abstract_origin %$D245e6} + } + + D42045: inlined_subroutine { + {abstract_origin %$D5ab2} + {low_pc 0x8060 addr} + {high_pc 0xc data4} + } { + D420b5: formal_parameter { + {abstract_origin %$D5ac2} + } + } + } + } + } +} + +# We force the DIEs above to be read in via "--readnow". +set saved_gdbflags $GDBFLAGS +append GDBFLAGS " --readnow" + +if {[prepare_for_testing "failed to prepare" $testfile \ + "${asm_file} ${srcfile}" {}]} { + return -1 +} +set GDBFLAGS $saved_gdbflags + +# All we need to do is check whether GDB is alive. Without +# multidictionaries, it will either crash, assert, or throw an +# internal_error. +gdb_test "p 1" "= 1" "GDB is alive" + -- 2.17.2 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 5/5] gdb/23712: Test case for multidictionary 2018-11-09 2:34 ` [PATCH 5/5] gdb/23712: Test case for multidictionary Keith Seitz @ 2018-11-30 20:59 ` Tom Tromey 2020-06-02 12:26 ` [committed][gdb/testsuite] Fix scrolling in gdb.dwarf2/multidictionary.exp Tom de Vries 0 siblings, 1 reply; 15+ messages in thread From: Tom Tromey @ 2018-11-30 20:59 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> This is a test derived from one of the reproducers in symtab/23010. Keith> The DIE tree used here is typical of compilations with LTO, where an Keith> artificial parent DIE of language C99 imports DIEs of other languages. Keith> testsuite/ChangeLog: Keith> PR gdb/23712 Keith> * gdb.dwarf2/multidictionary.exp: New file. Thanks. This is great. Keith> +# We force the DIEs above to be read in via "--readnow". Keith> +set saved_gdbflags $GDBFLAGS Keith> +append GDBFLAGS " --readnow" Keith> + Keith> +if {[prepare_for_testing "failed to prepare" $testfile \ Keith> + "${asm_file} ${srcfile}" {}]} { Keith> + return -1 Keith> +} Keith> +set GDBFLAGS $saved_gdbflags I recently learned about gdb_spawn_with_cmdline_opts -- see gdb.base/warning.exp for an example. I think that's preferable to saving and resetting GDBFLAGS. This is ok with that change. Tom ^ permalink raw reply [flat|nested] 15+ messages in thread
* [committed][gdb/testsuite] Fix scrolling in gdb.dwarf2/multidictionary.exp 2018-11-30 20:59 ` Tom Tromey @ 2020-06-02 12:26 ` Tom de Vries 0 siblings, 0 replies; 15+ messages in thread From: Tom de Vries @ 2020-06-02 12:26 UTC (permalink / raw) To: Tom Tromey, Keith Seitz; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 1320 bytes --] [ was: Re: [PATCH 5/5] gdb/23712: Test case for multidictionary ] On 30-11-2018 21:59, Tom Tromey wrote: >>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: > > Keith> This is a test derived from one of the reproducers in symtab/23010. > Keith> The DIE tree used here is typical of compilations with LTO, where an > Keith> artificial parent DIE of language C99 imports DIEs of other languages. > > Keith> testsuite/ChangeLog: > > Keith> PR gdb/23712 > Keith> * gdb.dwarf2/multidictionary.exp: New file. > > Thanks. This is great. > > Keith> +# We force the DIEs above to be read in via "--readnow". > Keith> +set saved_gdbflags $GDBFLAGS > Keith> +append GDBFLAGS " --readnow" > Keith> + > Keith> +if {[prepare_for_testing "failed to prepare" $testfile \ > Keith> + "${asm_file} ${srcfile}" {}]} { > Keith> + return -1 > Keith> +} > Keith> +set GDBFLAGS $saved_gdbflags > > I recently learned about gdb_spawn_with_cmdline_opts -- see > gdb.base/warning.exp for an example. I think that's preferable to > saving and resetting GDBFLAGS. > I ran into another problem in this test-case due to using gdb_spawn_with_cmdline_opts. I've fixed this by removing the use of gdb_spawn_with_cmdline_opts, and temporarily modifying GDBFLAGS instead (using save_vars to restore the value). Thanks, - Tom [-- Attachment #2: 0001-gdb-testsuite-Fix-scrolling-in-gdb.dwarf2-multidictionary.exp.patch --] [-- Type: text/x-patch, Size: 2171 bytes --] [gdb/testsuite] Fix scrolling in gdb.dwarf2/multidictionary.exp Consider a gdb_load patch to call the gdb_file_cmd twice: ... proc gdb_load { arg } { if { $arg != "" } { + set res [gdb_file_cmd $arg] + if { $res != 0 } { + return $res + } return [gdb_file_cmd $arg] } return 0 } ... With this patch, I run into: ... (gdb) kill^M The program is not being run.^M (gdb) ^M</outputs/gdb.dwarf2/multidictionary/multidictionary^M <.dwarf2/multidictionary/multidictionary"? (y or n) ERROR: Couldn't load outputs/gdb.dwarf2/multidictionary/multidictionary \ into gdb (timeout). p 1^M Please answer y or n.^M <.dwarf2/multidictionary/multidictionary"? (y or n) n^M Not confirmed.^M (gdb) UNRESOLVED: gdb.dwarf2/multidictionary.exp: GDB is alive \ (got interactive prompt) ... The problem is that the second file command results in a prompt, which is normally handled by gdb_file_cmd, but not recognized because the initial part of the prompt is scrolled out. This in turn is caused by using gdb_spawn_with_cmdline_opts without a subsequent "set width 0". Fix this by avoiding gdb_spawn_with_cmdline_opts, and forcing -readline by temporarily modifying GDBFLAGS instead. Tested on x86_64-linux. gdb/testsuite/ChangeLog: 2020-06-02 Tom de Vries <tdevries@suse.de> * gdb.dwarf2/multidictionary.exp: Don't use gdb_spawn_with_cmdline_opts. --- gdb/testsuite/gdb.dwarf2/multidictionary.exp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/gdb/testsuite/gdb.dwarf2/multidictionary.exp b/gdb/testsuite/gdb.dwarf2/multidictionary.exp index 01e5a0de45..45ba1ed99b 100644 --- a/gdb/testsuite/gdb.dwarf2/multidictionary.exp +++ b/gdb/testsuite/gdb.dwarf2/multidictionary.exp @@ -147,12 +147,9 @@ if {[build_executable $testfile.exp $testfile [list $asm_file $srcfile] {}] \ } # We force the DIEs above to be read in via "-readnow". -gdb_spawn_with_cmdline_opts "-readnow" -set test "initial prompt" -gdb_test_multiple "" $test { - -re ".*$gdb_prompt $" { - pass "$test" - } +save_vars { GDBFLAGS } { + set GDBFLAGS "$GDBFLAGS -readnow" + clean_restart } gdb_load $binfile ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb/23712: Introduce multidictionary's 2018-11-09 2:28 [PATCH 1/5] gdb/23712: Introduce multidictionary's Keith Seitz ` (3 preceding siblings ...) 2018-11-09 2:34 ` [PATCH 5/5] gdb/23712: Test case for multidictionary Keith Seitz @ 2018-12-16 17:01 ` Tom Tromey 2018-12-18 17:28 ` Keith Seitz 4 siblings, 1 reply; 15+ messages in thread From: Tom Tromey @ 2018-12-16 17:01 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: Keith> gdb/23712 is a new manifestation of the now-infamous (at least to me) Keith> symtab/23010 assertion failure (DICT_LANGUAGE == SYMBOL_LANGAUGE). Keith> I've done exhaustive performance testing with this approach, and I've Keith> attempted to minimize the overhead for the (overwhelmingly) most common Keith> one-language scenario. I'm not super concerned about this. I wonder a bit more about the memory overhead, though. Keith> [I've tested only GDB and firefox with -readnow.] In general my view is that -readnow performance just isn't important, since no users should ever do that, and CU expansion is rarely noticeable anyhow. (And, if CU expansion performance is a problem, I'll note there are some relatively simple ways to greatly speed it up that we have never seriously pursued.) Keith> +struct multidictionary Keith> +{ Keith> + /* An array of dictionaries, one per language. All dictionaries Keith> + must be of the same type. This should be free'd for expandable Keith> + dictionary types. */ Keith> + struct dictionary **dictionaries; Keith> + Keith> + /* The number of language dictionaries currently allocated. Keith> + Only used for expandable dictionaries. */ Keith> + unsigned short n_allocated_dictionaries; Keith> +}; I think this is the main possibly contentious bit of this series. I wondered when reading this series why it isn't possible to just put symbols of multiple languages into a single hash table. That would seem to make the series as a whole much simpler: no mass renaming, no need to store multiple dictionaries in a block, etc. Maybe one problem is that when searching you may only have the searched-for symbol name; so you'd have to maybe keep a bitset of all the languages in a dictionary and then do multiple hash lookups? But still that seems better. Don't rush off to change anything yet! I'd rather we discuss it before asking for more work. Also I'd appreciate opinions from anybody else listening. thanks, Tom ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb/23712: Introduce multidictionary's 2018-12-16 17:01 ` [PATCH 1/5] gdb/23712: Introduce multidictionary's Tom Tromey @ 2018-12-18 17:28 ` Keith Seitz 2018-12-19 11:42 ` Richard Biener 2019-01-02 16:41 ` Tom Tromey 0 siblings, 2 replies; 15+ messages in thread From: Keith Seitz @ 2018-12-18 17:28 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches Hi, Tom, On 12/16/18 9:01 AM, Tom Tromey wrote: > > I wonder a bit more about the memory overhead, though. I see that I didn't include this information in my original submission. I apologize for that because, of course, our own gdb.perf has this information. For completeness, I've copied the vmsize results that I collected during my original testing. Difference to unpatched master (at the time of submission -- multiply by 100 for percentage), vmsize: gmonster1:gmonster-null-lookup 10000-cus 0.00132458225987 gmonster1:gmonster-pervasive-typedef vmsize 10000-cus -0.000998352718015 gmonster1:gmonster-print-cerr 10000-cus -0.018685850806828 gmonster1:gmonster-ptype-string 10000-cus 0.005297872904029 gmonster1:gmonster-runto-main 10000-cus 0.018689093578962 gmonster1:gmonster-select-file 10000-cus 0.007689710164794 gmonster2:gmonster-null-lookup 1000-sos 0.009959564169472 gmonster2:gmonster-pervasive-typedef 1000-sos 0.000 gmonster2:gmonster-print-cerr 1000-sos 0.002044195506858 gmonster2:gmonster-ptype-string 1000-sos -0.021910168309929 gmonster2:gmonster-runto-main 1000-sos 0.006132085113341 gmonster2:gmonster-select-file 1000-sos 0.038964767646938 Using various methods to check memory consumption (massif, smem), the average additional memory consumption this patch adds with firefox is approx 1.3%. That doesn't seem altogether too out-of-line with gdb.perf results. We can probably shave a bit off this by using a bitfield or char instead of an unsigned short, too. > Keith> +struct multidictionary > Keith> +{ > Keith> + /* An array of dictionaries, one per language. All dictionaries > Keith> + must be of the same type. This should be free'd for expandable > Keith> + dictionary types. */ > Keith> + struct dictionary **dictionaries; > Keith> + > Keith> + /* The number of language dictionaries currently allocated. > Keith> + Only used for expandable dictionaries. */ > Keith> + unsigned short n_allocated_dictionaries; > Keith> +}; > > I think this is the main possibly contentious bit of this series. Is it just the increase of memory usage introduced by this struct that causes you concern? > I wondered when reading this series why it isn't possible to just put > symbols of multiple languages into a single hash table. That would seem > to make the series as a whole much simpler: no mass renaming, no need to > store multiple dictionaries in a block, etc. > > Maybe one problem is that when searching you may only have the > searched-for symbol name; so you'd have to maybe keep a bitset of all > the languages in a dictionary and then do multiple hash lookups? But > still that seems better. Yeah, this is the main implementation problem, exemplified by iter_match_first_hashed: const language_defn *lang = DICT_LANGUAGE (dict); unsigned int hash_index = (name.search_name_hash (lang->la_language) % DICT_HASHED_NBUCKETS (dict)); symbol_name_matcher_ftype *matches_name = get_symbol_name_matcher (lang, name); /* Loop through the symbols in the given bucket, breaking when SYM first matches. If SYM never matches, it will be set to NULL; either way, we have the right return value. */ for (sym = DICT_HASHED_BUCKET (dict, hash_index); sym != NULL; sym = sym->hash_next) { /* Warning: the order of arguments to compare matters! */ if (matches_name (SYMBOL_SEARCH_NAME (sym), name, NULL)) break; } Some of this is fairly trivial to fixup to permit multiple languages in a dictionary, e.g., moving the get_symbol_name_matcher into the loop. It adds cost for the loop, but I have not attempted to quantify that. The bigger problem is computing the hash_index for the bucket we want to loop over... With no language definition associated with the dictionary, we will have to loop over /all/ buckets, computing a hash for the first symbol in the bucket until we find a matching bucket, if any. An alternative is storing the hash in each bucket to facilitate this, but that would consume even more memory. This sounded incredibly wasteful to me when I wrote this patch. Keith ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb/23712: Introduce multidictionary's 2018-12-18 17:28 ` Keith Seitz @ 2018-12-19 11:42 ` Richard Biener 2019-01-02 16:41 ` Tom Tromey 1 sibling, 0 replies; 15+ messages in thread From: Richard Biener @ 2018-12-19 11:42 UTC (permalink / raw) To: Keith Seitz; +Cc: Tom Tromey, gdb-patches On Tue, 18 Dec 2018, Keith Seitz wrote: > Hi, Tom, > > On 12/16/18 9:01 AM, Tom Tromey wrote: > > > > I wonder a bit more about the memory overhead, though. > > I see that I didn't include this information in my original submission. > I apologize for that because, of course, our own gdb.perf has this > information. > > For completeness, I've copied the vmsize results that I collected during > my original testing. > > Difference to unpatched master (at the time of submission -- multiply > by 100 for percentage), vmsize: > > gmonster1:gmonster-null-lookup 10000-cus 0.00132458225987 > gmonster1:gmonster-pervasive-typedef vmsize 10000-cus -0.000998352718015 > gmonster1:gmonster-print-cerr 10000-cus -0.018685850806828 > gmonster1:gmonster-ptype-string 10000-cus 0.005297872904029 > gmonster1:gmonster-runto-main 10000-cus 0.018689093578962 > gmonster1:gmonster-select-file 10000-cus 0.007689710164794 > gmonster2:gmonster-null-lookup 1000-sos 0.009959564169472 > gmonster2:gmonster-pervasive-typedef 1000-sos 0.000 > gmonster2:gmonster-print-cerr 1000-sos 0.002044195506858 > gmonster2:gmonster-ptype-string 1000-sos -0.021910168309929 > gmonster2:gmonster-runto-main 1000-sos 0.006132085113341 > gmonster2:gmonster-select-file 1000-sos 0.038964767646938 > > Using various methods to check memory consumption (massif, smem), the average > additional memory consumption this patch adds with firefox is approx 1.3%. > That doesn't seem altogether too out-of-line with gdb.perf results. We can > probably shave a bit off this by using a bitfield or char instead of an > unsigned short, too. > > > Keith> +struct multidictionary > > Keith> +{ > > Keith> + /* An array of dictionaries, one per language. All dictionaries > > Keith> + must be of the same type. This should be free'd for expandable > > Keith> + dictionary types. */ > > Keith> + struct dictionary **dictionaries; > > Keith> + > > Keith> + /* The number of language dictionaries currently allocated. > > Keith> + Only used for expandable dictionaries. */ > > Keith> + unsigned short n_allocated_dictionaries; > > Keith> +}; > > > > I think this is the main possibly contentious bit of this series. > > Is it just the increase of memory usage introduced by this struct that causes you > concern? > > > I wondered when reading this series why it isn't possible to just put > > symbols of multiple languages into a single hash table. That would seem > > to make the series as a whole much simpler: no mass renaming, no need to > > store multiple dictionaries in a block, etc. > > > > Maybe one problem is that when searching you may only have the > > searched-for symbol name; so you'd have to maybe keep a bitset of all > > the languages in a dictionary and then do multiple hash lookups? But > > still that seems better. > > Yeah, this is the main implementation problem, exemplified by > iter_match_first_hashed: > > const language_defn *lang = DICT_LANGUAGE (dict); > unsigned int hash_index = (name.search_name_hash (lang->la_language) > % DICT_HASHED_NBUCKETS (dict)); > symbol_name_matcher_ftype *matches_name > = get_symbol_name_matcher (lang, name); > > /* Loop through the symbols in the given bucket, breaking when SYM > first matches. If SYM never matches, it will be set to NULL; > either way, we have the right return value. */ > > for (sym = DICT_HASHED_BUCKET (dict, hash_index); > sym != NULL; > sym = sym->hash_next) > { > /* Warning: the order of arguments to compare matters! */ > if (matches_name (SYMBOL_SEARCH_NAME (sym), name, NULL)) > break; > } > > Some of this is fairly trivial to fixup to permit multiple languages in > a dictionary, e.g., moving the get_symbol_name_matcher into the loop. It > adds cost for the loop, but I have not attempted to quantify that. > > The bigger problem is computing the hash_index for the bucket we want to > loop over... With no language definition associated with the dictionary, > we will have to loop over /all/ buckets, computing a hash for the first > symbol in the bucket until we find a matching bucket, if any. An alternative > is storing the hash in each bucket to facilitate this, but that would consume > even more memory. > > This sounded incredibly wasteful to me when I wrote this patch. I wonder whether we have a "context" language for 'name' that already determines what hash function and what matcher we should use? That is, in gdb user expressions already have to be written in the language currently active, so even for a "C" struct symbol x I have to write x%f when in fortran mode and x.f doesn't work. Note that during poking on the bug myself I figured that what language a symbol ends up associated with is also inconsitent dependend on who creates the symbol. Richard. > Keith > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb/23712: Introduce multidictionary's 2018-12-18 17:28 ` Keith Seitz 2018-12-19 11:42 ` Richard Biener @ 2019-01-02 16:41 ` Tom Tromey 2019-01-10 22:10 ` Keith Seitz 1 sibling, 1 reply; 15+ messages in thread From: Tom Tromey @ 2019-01-02 16:41 UTC (permalink / raw) To: Keith Seitz; +Cc: Tom Tromey, gdb-patches >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: [... size cost of the change...] Keith> This sounded incredibly wasteful to me when I wrote this patch. I have been thinking about this patch over the break, and I don't have any more objections. It fixes a serious bug and is not overly expensive; and furthermore I think it isn't any more complicated than the alternative. So, this is ok. thanks, Tom ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/5] gdb/23712: Introduce multidictionary's 2019-01-02 16:41 ` Tom Tromey @ 2019-01-10 22:10 ` Keith Seitz 0 siblings, 0 replies; 15+ messages in thread From: Keith Seitz @ 2019-01-10 22:10 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 1/2/19 8:41 AM, Tom Tromey wrote: >>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes: > > [... size cost of the change...] > Keith> This sounded incredibly wasteful to me when I wrote this patch. > > I have been thinking about this patch over the break, and I don't have > any more objections. It fixes a serious bug and is not overly > expensive; and furthermore I think it isn't any more complicated than > the alternative. So, this is ok. > I know this hasn't been an easy process for anyone, including users, and it has obviously been a more-than-expected agonizing review process for you. So a big thank you to everyone affected by this bug for their patience and perseverance. Pursuant to previous comments, I've updated the ChangeLog entries to also mention symtab/23010 and changed the final patch (the test) to use gdb_spawn_with_cmdline_opts. I've verified that the test fails before this patch and passes afterwards. I believe that was all that was requested. I've now pushed this series. Let's hope that's the last we see of this for a little while, at least! ;-) Thank you again! Keith ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-06-02 12:26 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-09 2:28 [PATCH 1/5] gdb/23712: Introduce multidictionary's Keith Seitz 2018-11-09 2:28 ` [PATCH 4/5] gdb/23712: Remove dw2_add_symbol_to_list Keith Seitz 2018-11-30 20:58 ` Tom Tromey 2018-11-09 2:28 ` [PATCH 3/5] gdb/23712: Cleanup/Remove temporary dictionary functions Keith Seitz 2018-11-30 21:26 ` Tom Tromey 2018-11-09 2:28 ` [PATCH 2/5] gdb/23712: Use new multidictionary API Keith Seitz 2018-11-30 21:22 ` Tom Tromey 2018-11-09 2:34 ` [PATCH 5/5] gdb/23712: Test case for multidictionary Keith Seitz 2018-11-30 20:59 ` Tom Tromey 2020-06-02 12:26 ` [committed][gdb/testsuite] Fix scrolling in gdb.dwarf2/multidictionary.exp Tom de Vries 2018-12-16 17:01 ` [PATCH 1/5] gdb/23712: Introduce multidictionary's Tom Tromey 2018-12-18 17:28 ` Keith Seitz 2018-12-19 11:42 ` Richard Biener 2019-01-02 16:41 ` Tom Tromey 2019-01-10 22:10 ` Keith Seitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).