* [PATCH v2 0/4] Fix regression in new DWARF reader @ 2023-01-10 18:33 Tom Tromey 2023-01-10 18:33 ` [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each Tom Tromey ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Tom Tromey @ 2023-01-10 18:33 UTC (permalink / raw) To: gdb-patches Here's v2 of the series to fix a regression introduced by the new DWARF reader. v1 was here: https://sourceware.org/pipermail/gdb-patches/2022-December/194795.html In this version, I added some unit tests in patch #1 to try to demonstrate that patch #2 is safe. Because parallel_for_each is now so complicated, this turns out to be non-obvious, and in fact I found another bug (not affecting patch #2 -- the bug was that an empty task could still be submitted -- but an empty task does not yield an empty result) in it while working on this. Let me know what you think. This is blocking GDB 13. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each 2023-01-10 18:33 [PATCH v2 0/4] Fix regression in new DWARF reader Tom Tromey @ 2023-01-10 18:33 ` Tom Tromey 2023-01-14 6:03 ` Joel Brobecker 2023-01-10 18:33 ` [PATCH v2 2/4] Don't erase empty indices in DWARF reader Tom Tromey ` (2 subsequent siblings) 3 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-01-10 18:33 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey I found that parallel_for_each would submit empty tasks to the thread pool. For example, this can happen if the number of tasks is smaller than the number of available threads. In the DWARF reader, this resulted in the cooked index containing empty sub-indices. This patch arranges to instead shrink the result vector and process the trailing entries in the calling thread. --- gdb/unittests/parallel-for-selftests.c | 39 ++++++++++++++++++++++++++ gdbsupport/parallel-for.h | 30 ++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/gdb/unittests/parallel-for-selftests.c b/gdb/unittests/parallel-for-selftests.c index 3162db18df1..15a095ae62b 100644 --- a/gdb/unittests/parallel-for-selftests.c +++ b/gdb/unittests/parallel-for-selftests.c @@ -149,6 +149,45 @@ TEST (int n_threads) SELF_CHECK (counter == NUMBER); #undef NUMBER + + /* Check that if there are fewer tasks than threads, then we won't + end up with a null result. */ + std::vector<std::unique_ptr<int>> intresults; + std::atomic<bool> any_empty_tasks (false); + + FOR_EACH (1, 0, 1, + [&] (int start, int end) + { + if (start == end) + any_empty_tasks = true; + return std::unique_ptr<int> (new int (end - start)); + }); + SELF_CHECK (!any_empty_tasks); + SELF_CHECK (std::all_of (intresults.begin (), + intresults.end (), + [] (const std::unique_ptr<int> &entry) + { + return entry != nullptr; + })); + + /* The same but using the task size parameter. */ + intresults.clear (); + any_empty_tasks = false; + FOR_EACH (1, 0, 1, + [&] (int start, int end) + { + if (start == end) + any_empty_tasks = true; + return std::unique_ptr<int> (new int (end - start)); + }, + task_size_one); + SELF_CHECK (!any_empty_tasks); + SELF_CHECK (std::all_of (intresults.begin (), + intresults.end (), + [] (const std::unique_ptr<int> &entry) + { + return entry != nullptr; + })); } #endif /* FOR_EACH */ diff --git a/gdbsupport/parallel-for.h b/gdbsupport/parallel-for.h index b565676a0d0..de9ebb15746 100644 --- a/gdbsupport/parallel-for.h +++ b/gdbsupport/parallel-for.h @@ -70,6 +70,12 @@ struct par_for_accumulator return result; } + /* Resize the results to N. */ + void resize (size_t n) + { + m_futures.resize (n); + } + private: /* A vector of futures coming from the tasks run in the @@ -108,6 +114,12 @@ struct par_for_accumulator<void> } } + /* Resize the results to N. */ + void resize (size_t n) + { + m_futures.resize (n); + } + private: std::vector<gdb::future<void>> m_futures; @@ -232,6 +244,24 @@ parallel_for_each (unsigned n, RandomIt first, RandomIt last, end = j; remaining_size -= chunk_size; } + + /* This case means we don't have enough elements to really + distribute them. Rather than ever submit a task that does + nothing, we short-circuit here. */ + if (first == end) + end = last; + + if (end == last) + { + /* We're about to dispatch the last batch of elements, which + we normally process in the main thread. So just truncate + the result list here. This avoids submitting empty tasks + to the thread pool. */ + count = i; + results.resize (count); + break; + } + if (parallel_for_each_debug) { debug_printf (_("Parallel for: elements on worker thread %i\t: %zu"), -- 2.38.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each 2023-01-10 18:33 ` [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each Tom Tromey @ 2023-01-14 6:03 ` Joel Brobecker 0 siblings, 0 replies; 19+ messages in thread From: Joel Brobecker @ 2023-01-14 6:03 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey, Joel Brobecker Hi Tom, On Tue, Jan 10, 2023 at 11:33:35AM -0700, Tom Tromey via Gdb-patches wrote: > I found that parallel_for_each would submit empty tasks to the thread > pool. For example, this can happen if the number of tasks is smaller > than the number of available threads. In the DWARF reader, this > resulted in the cooked index containing empty sub-indices. This patch > arranges to instead shrink the result vector and process the trailing > entries in the calling thread. Thanks for the updated patch, and the added test. This patch looks OK to me. > --- > gdb/unittests/parallel-for-selftests.c | 39 ++++++++++++++++++++++++++ > gdbsupport/parallel-for.h | 30 ++++++++++++++++++++ > 2 files changed, 69 insertions(+) > > diff --git a/gdb/unittests/parallel-for-selftests.c b/gdb/unittests/parallel-for-selftests.c > index 3162db18df1..15a095ae62b 100644 > --- a/gdb/unittests/parallel-for-selftests.c > +++ b/gdb/unittests/parallel-for-selftests.c > @@ -149,6 +149,45 @@ TEST (int n_threads) > SELF_CHECK (counter == NUMBER); > > #undef NUMBER > + > + /* Check that if there are fewer tasks than threads, then we won't > + end up with a null result. */ > + std::vector<std::unique_ptr<int>> intresults; > + std::atomic<bool> any_empty_tasks (false); > + > + FOR_EACH (1, 0, 1, > + [&] (int start, int end) > + { > + if (start == end) > + any_empty_tasks = true; > + return std::unique_ptr<int> (new int (end - start)); > + }); > + SELF_CHECK (!any_empty_tasks); > + SELF_CHECK (std::all_of (intresults.begin (), > + intresults.end (), > + [] (const std::unique_ptr<int> &entry) > + { > + return entry != nullptr; > + })); > + > + /* The same but using the task size parameter. */ > + intresults.clear (); > + any_empty_tasks = false; > + FOR_EACH (1, 0, 1, > + [&] (int start, int end) > + { > + if (start == end) > + any_empty_tasks = true; > + return std::unique_ptr<int> (new int (end - start)); > + }, > + task_size_one); > + SELF_CHECK (!any_empty_tasks); > + SELF_CHECK (std::all_of (intresults.begin (), > + intresults.end (), > + [] (const std::unique_ptr<int> &entry) > + { > + return entry != nullptr; > + })); > } > > #endif /* FOR_EACH */ > diff --git a/gdbsupport/parallel-for.h b/gdbsupport/parallel-for.h > index b565676a0d0..de9ebb15746 100644 > --- a/gdbsupport/parallel-for.h > +++ b/gdbsupport/parallel-for.h > @@ -70,6 +70,12 @@ struct par_for_accumulator > return result; > } > > + /* Resize the results to N. */ > + void resize (size_t n) > + { > + m_futures.resize (n); > + } > + > private: > > /* A vector of futures coming from the tasks run in the > @@ -108,6 +114,12 @@ struct par_for_accumulator<void> > } > } > > + /* Resize the results to N. */ > + void resize (size_t n) > + { > + m_futures.resize (n); > + } > + > private: > > std::vector<gdb::future<void>> m_futures; > @@ -232,6 +244,24 @@ parallel_for_each (unsigned n, RandomIt first, RandomIt last, > end = j; > remaining_size -= chunk_size; > } > + > + /* This case means we don't have enough elements to really > + distribute them. Rather than ever submit a task that does > + nothing, we short-circuit here. */ > + if (first == end) > + end = last; > + > + if (end == last) > + { > + /* We're about to dispatch the last batch of elements, which > + we normally process in the main thread. So just truncate > + the result list here. This avoids submitting empty tasks > + to the thread pool. */ > + count = i; > + results.resize (count); > + break; > + } > + > if (parallel_for_each_debug) > { > debug_printf (_("Parallel for: elements on worker thread %i\t: %zu"), > -- > 2.38.1 > -- Joel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 2/4] Don't erase empty indices in DWARF reader 2023-01-10 18:33 [PATCH v2 0/4] Fix regression in new DWARF reader Tom Tromey 2023-01-10 18:33 ` [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each Tom Tromey @ 2023-01-10 18:33 ` Tom Tromey 2023-01-14 6:05 ` Joel Brobecker 2023-01-10 18:33 ` [PATCH v2 3/4] Move hash_entry and eq_entry into cooked_index::do_finalize Tom Tromey 2023-01-10 18:33 ` [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader Tom Tromey 3 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-01-10 18:33 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey The DWARF reader has some code to remove empty indices. However, I think this code has been obsolete since some earlier changes to parallel_for_each. This patch removes this code. --- gdb/dwarf2/read.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 851852404eb..c3f246fedf7 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -7170,16 +7170,6 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile) print_tu_stats (per_objfile); indexes.push_back (index_storage.release ()); - /* Remove any NULL entries. This might happen if parallel-for - decides to throttle the number of threads that were used. */ - indexes.erase - (std::remove_if (indexes.begin (), - indexes.end (), - [] (const std::unique_ptr<cooked_index> &entry) - { - return entry == nullptr; - }), - indexes.end ()); indexes.shrink_to_fit (); cooked_index_vector *vec = new cooked_index_vector (std::move (indexes)); -- 2.38.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/4] Don't erase empty indices in DWARF reader 2023-01-10 18:33 ` [PATCH v2 2/4] Don't erase empty indices in DWARF reader Tom Tromey @ 2023-01-14 6:05 ` Joel Brobecker 2023-01-17 13:53 ` Tom Tromey 0 siblings, 1 reply; 19+ messages in thread From: Joel Brobecker @ 2023-01-14 6:05 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey, Joel Brobecker On Tue, Jan 10, 2023 at 11:33:36AM -0700, Tom Tromey via Gdb-patches wrote: > The DWARF reader has some code to remove empty indices. However, I > think this code has been obsolete since some earlier changes to > parallel_for_each. This patch removes this code. I don't think this code needs a re-review, since it's already been approved by me, and I think also by Andrew, but just in case, OK for me :). > --- > gdb/dwarf2/read.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 851852404eb..c3f246fedf7 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -7170,16 +7170,6 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile) > print_tu_stats (per_objfile); > > indexes.push_back (index_storage.release ()); > - /* Remove any NULL entries. This might happen if parallel-for > - decides to throttle the number of threads that were used. */ > - indexes.erase > - (std::remove_if (indexes.begin (), > - indexes.end (), > - [] (const std::unique_ptr<cooked_index> &entry) > - { > - return entry == nullptr; > - }), > - indexes.end ()); > indexes.shrink_to_fit (); > > cooked_index_vector *vec = new cooked_index_vector (std::move (indexes)); > -- > 2.38.1 > -- Joel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/4] Don't erase empty indices in DWARF reader 2023-01-14 6:05 ` Joel Brobecker @ 2023-01-17 13:53 ` Tom Tromey 0 siblings, 0 replies; 19+ messages in thread From: Tom Tromey @ 2023-01-17 13:53 UTC (permalink / raw) To: Joel Brobecker; +Cc: Tom Tromey via Gdb-patches, Tom Tromey >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes: >> The DWARF reader has some code to remove empty indices. However, I >> think this code has been obsolete since some earlier changes to >> parallel_for_each. This patch removes this code. Joel> I don't think this code needs a re-review, since it's already been Joel> approved by me, and I think also by Andrew, but just in case, Joel> OK for me :). Thanks. I think somewhere we discussed the possibility of not merging this patch to gdb-13. I'm planning to leave this one out there, as it's safe to do so. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/4] Move hash_entry and eq_entry into cooked_index::do_finalize 2023-01-10 18:33 [PATCH v2 0/4] Fix regression in new DWARF reader Tom Tromey 2023-01-10 18:33 ` [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each Tom Tromey 2023-01-10 18:33 ` [PATCH v2 2/4] Don't erase empty indices in DWARF reader Tom Tromey @ 2023-01-10 18:33 ` Tom Tromey 2023-01-14 6:06 ` Joel Brobecker 2023-01-10 18:33 ` [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader Tom Tromey 3 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-01-10 18:33 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey I was briefly confused by the hash_entry and eq_entry functions in the cooked index. They are only needed in a single method, and that method already has a couple of local lambdas for a different hash table. So, it seemed cleaner to move these there as well. --- gdb/dwarf2/cooked-index.c | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c index 93ffd923c76..c711e3b9b2a 100644 --- a/gdb/dwarf2/cooked-index.c +++ b/gdb/dwarf2/cooked-index.c @@ -26,26 +26,6 @@ #include "split-name.h" #include <algorithm> -/* Hash function for cooked_index_entry. */ - -static hashval_t -hash_entry (const void *e) -{ - const cooked_index_entry *entry = (const cooked_index_entry *) e; - return dwarf5_djb_hash (entry->canonical); -} - -/* Equality function for cooked_index_entry. */ - -static int -eq_entry (const void *a, const void *b) -{ - const cooked_index_entry *ae = (const cooked_index_entry *) a; - const gdb::string_view *sv = (const gdb::string_view *) b; - return (strlen (ae->canonical) == sv->length () - && strncasecmp (ae->canonical, sv->data (), sv->length ()) == 0); -} - /* See cooked-index.h. */ const char * @@ -191,6 +171,20 @@ cooked_index::do_finalize () htab_up seen_names (htab_create_alloc (10, hash_name_ptr, eq_name_ptr, nullptr, xcalloc, xfree)); + auto hash_entry = [] (const void *e) + { + const cooked_index_entry *entry = (const cooked_index_entry *) e; + return dwarf5_djb_hash (entry->canonical); + }; + + auto eq_entry = [] (const void *a, const void *b) -> int + { + const cooked_index_entry *ae = (const cooked_index_entry *) a; + const gdb::string_view *sv = (const gdb::string_view *) b; + return (strlen (ae->canonical) == sv->length () + && strncasecmp (ae->canonical, sv->data (), sv->length ()) == 0); + }; + htab_up gnat_entries (htab_create_alloc (10, hash_entry, eq_entry, nullptr, xcalloc, xfree)); -- 2.38.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/4] Move hash_entry and eq_entry into cooked_index::do_finalize 2023-01-10 18:33 ` [PATCH v2 3/4] Move hash_entry and eq_entry into cooked_index::do_finalize Tom Tromey @ 2023-01-14 6:06 ` Joel Brobecker 0 siblings, 0 replies; 19+ messages in thread From: Joel Brobecker @ 2023-01-14 6:06 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey, Joel Brobecker > I was briefly confused by the hash_entry and eq_entry functions in the > cooked index. They are only needed in a single method, and that > method already has a couple of local lambdas for a different hash > table. So, it seemed cleaner to move these there as well. Still OK for me :-). > gdb/dwarf2/cooked-index.c | 34 ++++++++++++++-------------------- > 1 file changed, 14 insertions(+), 20 deletions(-) > > diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c > index 93ffd923c76..c711e3b9b2a 100644 > --- a/gdb/dwarf2/cooked-index.c > +++ b/gdb/dwarf2/cooked-index.c > @@ -26,26 +26,6 @@ > #include "split-name.h" > #include <algorithm> > > -/* Hash function for cooked_index_entry. */ > - > -static hashval_t > -hash_entry (const void *e) > -{ > - const cooked_index_entry *entry = (const cooked_index_entry *) e; > - return dwarf5_djb_hash (entry->canonical); > -} > - > -/* Equality function for cooked_index_entry. */ > - > -static int > -eq_entry (const void *a, const void *b) > -{ > - const cooked_index_entry *ae = (const cooked_index_entry *) a; > - const gdb::string_view *sv = (const gdb::string_view *) b; > - return (strlen (ae->canonical) == sv->length () > - && strncasecmp (ae->canonical, sv->data (), sv->length ()) == 0); > -} > - > /* See cooked-index.h. */ > > const char * > @@ -191,6 +171,20 @@ cooked_index::do_finalize () > htab_up seen_names (htab_create_alloc (10, hash_name_ptr, eq_name_ptr, > nullptr, xcalloc, xfree)); > > + auto hash_entry = [] (const void *e) > + { > + const cooked_index_entry *entry = (const cooked_index_entry *) e; > + return dwarf5_djb_hash (entry->canonical); > + }; > + > + auto eq_entry = [] (const void *a, const void *b) -> int > + { > + const cooked_index_entry *ae = (const cooked_index_entry *) a; > + const gdb::string_view *sv = (const gdb::string_view *) b; > + return (strlen (ae->canonical) == sv->length () > + && strncasecmp (ae->canonical, sv->data (), sv->length ()) == 0); > + }; > + > htab_up gnat_entries (htab_create_alloc (10, hash_entry, eq_entry, > nullptr, xcalloc, xfree)); > > -- > 2.38.1 > -- Joel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-10 18:33 [PATCH v2 0/4] Fix regression in new DWARF reader Tom Tromey ` (2 preceding siblings ...) 2023-01-10 18:33 ` [PATCH v2 3/4] Move hash_entry and eq_entry into cooked_index::do_finalize Tom Tromey @ 2023-01-10 18:33 ` Tom Tromey 2023-01-14 6:11 ` Joel Brobecker 2023-01-17 18:09 ` Simon Marchi 3 siblings, 2 replies; 19+ messages in thread From: Tom Tromey @ 2023-01-10 18:33 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey PR c++/29896 points out a regression in the new DWARF reader. It does not properly handle a case like "break fn", where "fn" is a template function. This happens because the new index uses strncasecmp to compare. However, to make this work correctly, we need a custom function that ignores template parameters. This patch adds a custom comparison function and fixes the bug. A new test case is included. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29896 --- gdb/dwarf2/cooked-index.c | 143 +++++++++++++++++++++++++---- gdb/dwarf2/cooked-index.h | 15 ++- gdb/dwarf2/read.c | 3 +- gdb/testsuite/gdb.cp/paramless.cc | 46 ++++++++++ gdb/testsuite/gdb.cp/paramless.exp | 41 +++++++++ 5 files changed, 226 insertions(+), 22 deletions(-) create mode 100644 gdb/testsuite/gdb.cp/paramless.cc create mode 100644 gdb/testsuite/gdb.cp/paramless.exp diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c index c711e3b9b2a..09b3fd70b26 100644 --- a/gdb/dwarf2/cooked-index.c +++ b/gdb/dwarf2/cooked-index.c @@ -25,6 +25,114 @@ #include "ada-lang.h" #include "split-name.h" #include <algorithm> +#include "safe-ctype.h" +#include "gdbsupport/selftest.h" + +/* See cooked-index.h. */ + +bool +cooked_index_entry::compare (const char *stra, const char *strb, + bool completing) +{ + /* If we've ever matched "<" in both strings, then we disable the + special template parameter handling. */ + bool seen_lt = false; + + while (*stra != '\0' + && *strb != '\0' + && (TOLOWER ((unsigned char) *stra) + == TOLOWER ((unsigned char ) *strb))) + { + if (*stra == '<') + seen_lt = true; + ++stra; + ++strb; + } + + unsigned c1 = TOLOWER ((unsigned char) *stra); + unsigned c2 = TOLOWER ((unsigned char) *strb); + + if (completing) + { + /* When completing, if one string ends earlier than the other, + consider them as equal. Also, completion mode ignores the + special '<' handling. */ + if (c1 == '\0' || c2 == '\0') + return false; + /* Fall through to the generic case. */ + } + else if (seen_lt) + { + /* Fall through to the generic case. */ + } + else if (c1 == '\0' || c1 == '<') + { + /* Maybe they both end at the same spot. */ + if (c2 == '\0' || c2 == '<') + return false; + /* First string ended earlier. */ + return true; + } + else if (c2 == '\0' || c2 == '<') + { + /* Second string ended earlier. */ + return false; + } + + return c1 < c2; +} + +#if GDB_SELF_TEST + +namespace { + +void +test_compare () +{ + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", false)); + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", false)); + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", true)); + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", true)); + + SELF_CHECK (cooked_index_entry::compare ("abcd", "ABCDE", false)); + SELF_CHECK (!cooked_index_entry::compare ("ABCDE", "abcd", false)); + SELF_CHECK (!cooked_index_entry::compare ("abcd", "ABCDE", true)); + SELF_CHECK (!cooked_index_entry::compare ("ABCDE", "abcd", true)); + + SELF_CHECK (!cooked_index_entry::compare ("name", "name<>", false)); + SELF_CHECK (!cooked_index_entry::compare ("name<>", "name", false)); + SELF_CHECK (!cooked_index_entry::compare ("name", "name<>", true)); + SELF_CHECK (!cooked_index_entry::compare ("name<>", "name", true)); + + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", false)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", false)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", true)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<ag>", true)); + + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", + "name<arg<more>>", false)); + + SELF_CHECK (!cooked_index_entry::compare ("name", "name<arg<more>>", false)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name", false)); + SELF_CHECK (cooked_index_entry::compare ("name<arg<", "name<arg<more>>", + false)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg<", + "name<arg<more>>", + true)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name<arg<", + false)); + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name<arg<", + true)); + + SELF_CHECK (cooked_index_entry::compare ("", "abcd", false)); + SELF_CHECK (!cooked_index_entry::compare ("", "abcd", true)); + SELF_CHECK (!cooked_index_entry::compare ("abcd", "", false)); + SELF_CHECK (!cooked_index_entry::compare ("abcd", "", true)); +} + +} /* anonymous namespace */ + +#endif /* GDB_SELF_TEST */ /* See cooked-index.h. */ @@ -247,30 +355,24 @@ cooked_index::do_finalize () /* See cooked-index.h. */ cooked_index::range -cooked_index::find (gdb::string_view name, bool completing) +cooked_index::find (const std::string &name, bool completing) { wait (); - auto lower = std::lower_bound (m_entries.begin (), m_entries.end (), - name, + auto lower = std::lower_bound (m_entries.begin (), m_entries.end (), name, [=] (const cooked_index_entry *entry, - const gdb::string_view &n) + const std::string &n) { - int cmp = strncasecmp (entry->canonical, n.data (), n.length ()); - if (cmp != 0 || completing) - return cmp < 0; - return strlen (entry->canonical) < n.length (); + return cooked_index_entry::compare (entry->canonical, n.c_str (), + completing); }); - auto upper = std::upper_bound (m_entries.begin (), m_entries.end (), - name, - [=] (const gdb::string_view &n, + auto upper = std::upper_bound (m_entries.begin (), m_entries.end (), name, + [=] (const std::string &n, const cooked_index_entry *entry) { - int cmp = strncasecmp (n.data (), entry->canonical, n.length ()); - if (cmp != 0 || completing) - return cmp < 0; - return n.length () < strlen (entry->canonical); + return cooked_index_entry::compare (n.c_str (), entry->canonical, + completing); }); return range (lower, upper); @@ -311,7 +413,7 @@ cooked_index_vector::get_addrmaps () /* See cooked-index.h. */ cooked_index_vector::range -cooked_index_vector::find (gdb::string_view name, bool completing) +cooked_index_vector::find (const std::string &name, bool completing) { std::vector<cooked_index::range> result_range; result_range.reserve (m_vector.size ()); @@ -339,3 +441,12 @@ cooked_index_vector::get_main () const return result; } + +void _initialize_cooked_index (); +void +_initialize_cooked_index () +{ +#if GDB_SELF_TEST + selftests::register_test ("cooked_index_entry::compare", test_compare); +#endif +} diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h index 607e07745f9..2b34a6534e8 100644 --- a/gdb/dwarf2/cooked-index.h +++ b/gdb/dwarf2/cooked-index.h @@ -143,11 +143,16 @@ struct cooked_index_entry : public allocate_on_obstack STORAGE. */ const char *full_name (struct obstack *storage) const; - /* Entries must be sorted case-insensitively; this compares two - entries. */ + /* Compare two strings, case-insensitively. Return true if STRA is + less than STRB. If one string has template parameters, but the + other does not, then they are considered to be equal; so for + example "t<x>" == "t<x>", "t<x>" < "t<y>", but "t" == "t<x>". */ + static bool compare (const char *stra, const char *strb, bool completing); + + /* Compare two entries by canonical name. */ bool operator< (const cooked_index_entry &other) const { - return strcasecmp (canonical, other.canonical) < 0; + return compare (canonical, other.canonical, false); } /* The name as it appears in DWARF. This always points into one of @@ -232,7 +237,7 @@ class cooked_index /* Look up an entry by name. Returns a range of all matching results. If COMPLETING is true, then a larger range, suitable for completion, will be returned. */ - range find (gdb::string_view name, bool completing); + range find (const std::string &name, bool completing); private: @@ -335,7 +340,7 @@ class cooked_index_vector : public dwarf_scanner_base /* Look up an entry by name. Returns a range of all matching results. If COMPLETING is true, then a larger range, suitable for completion, will be returned. */ - range find (gdb::string_view name, bool completing); + range find (const std::string &name, bool completing); /* Return a range of all the entries. */ range all_entries () diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index c3f246fedf7..44b54f77de9 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -18741,8 +18741,9 @@ cooked_index_functions::expand_symtabs_matching { std::vector<gdb::string_view> name_vec = lookup_name_without_params.split_name (lang); + std::string last_name = gdb::to_string (name_vec.back ()); - for (const cooked_index_entry *entry : table->find (name_vec.back (), + for (const cooked_index_entry *entry : table->find (last_name, completing)) { QUIT; diff --git a/gdb/testsuite/gdb.cp/paramless.cc b/gdb/testsuite/gdb.cp/paramless.cc new file mode 100644 index 00000000000..392b15f4256 --- /dev/null +++ b/gdb/testsuite/gdb.cp/paramless.cc @@ -0,0 +1,46 @@ +/* Test case for template breakpoint test. + + Copyright 2023 Free Software Foundation, Inc. + + This file is part of GDB. + + 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/>. */ + +template<typename T> +struct outer +{ + template<typename Q = int> + int fn (int x) + { + return x + 23; + } +}; + +template<typename T = int> +int toplev (int y) +{ + return y; +} + +outer<int> outer1; +outer<char> outer2; + +int main () +{ + int x1 = outer1.fn (0); + int x2 = outer2.fn<short> (-46); + int x3 = toplev<char> (0); + int x4 = toplev (0); + return x1 + x2; +} diff --git a/gdb/testsuite/gdb.cp/paramless.exp b/gdb/testsuite/gdb.cp/paramless.exp new file mode 100644 index 00000000000..4fc8fd9e015 --- /dev/null +++ b/gdb/testsuite/gdb.cp/paramless.exp @@ -0,0 +1,41 @@ +# Copyright 2023 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/>. + +# This file is part of the gdb testsuite. + +# Test template breakpoints without parameters. + +if { [skip_cplus_tests] } { continue } + +standard_testfile .cc + +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { + return -1 +} + +gdb_breakpoint "outer<int>::fn" message +delete_breakpoints + +gdb_breakpoint "outer<char>::fn<short>" message +delete_breakpoints + +gdb_test "break outer::fn" "Breakpoint $decimal at .*2 locations." +delete_breakpoints + +gdb_test "break toplev" "Breakpoint $decimal at .*2 locations." +delete_breakpoints + +gdb_breakpoint "toplev<char>" message +delete_breakpoints -- 2.38.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-10 18:33 ` [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader Tom Tromey @ 2023-01-14 6:11 ` Joel Brobecker 2023-01-17 13:54 ` Tom Tromey 2023-01-17 16:44 ` Tom de Vries 2023-01-17 18:09 ` Simon Marchi 1 sibling, 2 replies; 19+ messages in thread From: Joel Brobecker @ 2023-01-14 6:11 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: Tom Tromey, Joel Brobecker Hi Tom, On Tue, Jan 10, 2023 at 11:33:38AM -0700, Tom Tromey via Gdb-patches wrote: > PR c++/29896 points out a regression in the new DWARF reader. It does > not properly handle a case like "break fn", where "fn" is a template > function. > > This happens because the new index uses strncasecmp to compare. > However, to make this work correctly, we need a custom function that > ignores template parameters. > > This patch adds a custom comparison function and fixes the bug. A new > test case is included. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29896 Thanks for updating the copyright years. I believe it should be 2022-2023, though. I remember being told that the start of the copyright year range corresponds to the year the file was committed to medium. The patch is still OK for me with the minor correction above. > --- > gdb/dwarf2/cooked-index.c | 143 +++++++++++++++++++++++++---- > gdb/dwarf2/cooked-index.h | 15 ++- > gdb/dwarf2/read.c | 3 +- > gdb/testsuite/gdb.cp/paramless.cc | 46 ++++++++++ > gdb/testsuite/gdb.cp/paramless.exp | 41 +++++++++ > 5 files changed, 226 insertions(+), 22 deletions(-) > create mode 100644 gdb/testsuite/gdb.cp/paramless.cc > create mode 100644 gdb/testsuite/gdb.cp/paramless.exp > > diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c > index c711e3b9b2a..09b3fd70b26 100644 > --- a/gdb/dwarf2/cooked-index.c > +++ b/gdb/dwarf2/cooked-index.c > @@ -25,6 +25,114 @@ > #include "ada-lang.h" > #include "split-name.h" > #include <algorithm> > +#include "safe-ctype.h" > +#include "gdbsupport/selftest.h" > + > +/* See cooked-index.h. */ > + > +bool > +cooked_index_entry::compare (const char *stra, const char *strb, > + bool completing) > +{ > + /* If we've ever matched "<" in both strings, then we disable the > + special template parameter handling. */ > + bool seen_lt = false; > + > + while (*stra != '\0' > + && *strb != '\0' > + && (TOLOWER ((unsigned char) *stra) > + == TOLOWER ((unsigned char ) *strb))) > + { > + if (*stra == '<') > + seen_lt = true; > + ++stra; > + ++strb; > + } > + > + unsigned c1 = TOLOWER ((unsigned char) *stra); > + unsigned c2 = TOLOWER ((unsigned char) *strb); > + > + if (completing) > + { > + /* When completing, if one string ends earlier than the other, > + consider them as equal. Also, completion mode ignores the > + special '<' handling. */ > + if (c1 == '\0' || c2 == '\0') > + return false; > + /* Fall through to the generic case. */ > + } > + else if (seen_lt) > + { > + /* Fall through to the generic case. */ > + } > + else if (c1 == '\0' || c1 == '<') > + { > + /* Maybe they both end at the same spot. */ > + if (c2 == '\0' || c2 == '<') > + return false; > + /* First string ended earlier. */ > + return true; > + } > + else if (c2 == '\0' || c2 == '<') > + { > + /* Second string ended earlier. */ > + return false; > + } > + > + return c1 < c2; > +} > + > +#if GDB_SELF_TEST > + > +namespace { > + > +void > +test_compare () > +{ > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", false)); > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", false)); > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", true)); > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", true)); > + > + SELF_CHECK (cooked_index_entry::compare ("abcd", "ABCDE", false)); > + SELF_CHECK (!cooked_index_entry::compare ("ABCDE", "abcd", false)); > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "ABCDE", true)); > + SELF_CHECK (!cooked_index_entry::compare ("ABCDE", "abcd", true)); > + > + SELF_CHECK (!cooked_index_entry::compare ("name", "name<>", false)); > + SELF_CHECK (!cooked_index_entry::compare ("name<>", "name", false)); > + SELF_CHECK (!cooked_index_entry::compare ("name", "name<>", true)); > + SELF_CHECK (!cooked_index_entry::compare ("name<>", "name", true)); > + > + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", false)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", false)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", true)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<ag>", true)); > + > + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", > + "name<arg<more>>", false)); > + > + SELF_CHECK (!cooked_index_entry::compare ("name", "name<arg<more>>", false)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name", false)); > + SELF_CHECK (cooked_index_entry::compare ("name<arg<", "name<arg<more>>", > + false)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg<", > + "name<arg<more>>", > + true)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name<arg<", > + false)); > + SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name<arg<", > + true)); > + > + SELF_CHECK (cooked_index_entry::compare ("", "abcd", false)); > + SELF_CHECK (!cooked_index_entry::compare ("", "abcd", true)); > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "", false)); > + SELF_CHECK (!cooked_index_entry::compare ("abcd", "", true)); > +} > + > +} /* anonymous namespace */ > + > +#endif /* GDB_SELF_TEST */ > > /* See cooked-index.h. */ > > @@ -247,30 +355,24 @@ cooked_index::do_finalize () > /* See cooked-index.h. */ > > cooked_index::range > -cooked_index::find (gdb::string_view name, bool completing) > +cooked_index::find (const std::string &name, bool completing) > { > wait (); > > - auto lower = std::lower_bound (m_entries.begin (), m_entries.end (), > - name, > + auto lower = std::lower_bound (m_entries.begin (), m_entries.end (), name, > [=] (const cooked_index_entry *entry, > - const gdb::string_view &n) > + const std::string &n) > { > - int cmp = strncasecmp (entry->canonical, n.data (), n.length ()); > - if (cmp != 0 || completing) > - return cmp < 0; > - return strlen (entry->canonical) < n.length (); > + return cooked_index_entry::compare (entry->canonical, n.c_str (), > + completing); > }); > > - auto upper = std::upper_bound (m_entries.begin (), m_entries.end (), > - name, > - [=] (const gdb::string_view &n, > + auto upper = std::upper_bound (m_entries.begin (), m_entries.end (), name, > + [=] (const std::string &n, > const cooked_index_entry *entry) > { > - int cmp = strncasecmp (n.data (), entry->canonical, n.length ()); > - if (cmp != 0 || completing) > - return cmp < 0; > - return n.length () < strlen (entry->canonical); > + return cooked_index_entry::compare (n.c_str (), entry->canonical, > + completing); > }); > > return range (lower, upper); > @@ -311,7 +413,7 @@ cooked_index_vector::get_addrmaps () > /* See cooked-index.h. */ > > cooked_index_vector::range > -cooked_index_vector::find (gdb::string_view name, bool completing) > +cooked_index_vector::find (const std::string &name, bool completing) > { > std::vector<cooked_index::range> result_range; > result_range.reserve (m_vector.size ()); > @@ -339,3 +441,12 @@ cooked_index_vector::get_main () const > > return result; > } > + > +void _initialize_cooked_index (); > +void > +_initialize_cooked_index () > +{ > +#if GDB_SELF_TEST > + selftests::register_test ("cooked_index_entry::compare", test_compare); > +#endif > +} > diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h > index 607e07745f9..2b34a6534e8 100644 > --- a/gdb/dwarf2/cooked-index.h > +++ b/gdb/dwarf2/cooked-index.h > @@ -143,11 +143,16 @@ struct cooked_index_entry : public allocate_on_obstack > STORAGE. */ > const char *full_name (struct obstack *storage) const; > > - /* Entries must be sorted case-insensitively; this compares two > - entries. */ > + /* Compare two strings, case-insensitively. Return true if STRA is > + less than STRB. If one string has template parameters, but the > + other does not, then they are considered to be equal; so for > + example "t<x>" == "t<x>", "t<x>" < "t<y>", but "t" == "t<x>". */ > + static bool compare (const char *stra, const char *strb, bool completing); > + > + /* Compare two entries by canonical name. */ > bool operator< (const cooked_index_entry &other) const > { > - return strcasecmp (canonical, other.canonical) < 0; > + return compare (canonical, other.canonical, false); > } > > /* The name as it appears in DWARF. This always points into one of > @@ -232,7 +237,7 @@ class cooked_index > /* Look up an entry by name. Returns a range of all matching > results. If COMPLETING is true, then a larger range, suitable > for completion, will be returned. */ > - range find (gdb::string_view name, bool completing); > + range find (const std::string &name, bool completing); > > private: > > @@ -335,7 +340,7 @@ class cooked_index_vector : public dwarf_scanner_base > /* Look up an entry by name. Returns a range of all matching > results. If COMPLETING is true, then a larger range, suitable > for completion, will be returned. */ > - range find (gdb::string_view name, bool completing); > + range find (const std::string &name, bool completing); > > /* Return a range of all the entries. */ > range all_entries () > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index c3f246fedf7..44b54f77de9 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -18741,8 +18741,9 @@ cooked_index_functions::expand_symtabs_matching > { > std::vector<gdb::string_view> name_vec > = lookup_name_without_params.split_name (lang); > + std::string last_name = gdb::to_string (name_vec.back ()); > > - for (const cooked_index_entry *entry : table->find (name_vec.back (), > + for (const cooked_index_entry *entry : table->find (last_name, > completing)) > { > QUIT; > diff --git a/gdb/testsuite/gdb.cp/paramless.cc b/gdb/testsuite/gdb.cp/paramless.cc > new file mode 100644 > index 00000000000..392b15f4256 > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/paramless.cc > @@ -0,0 +1,46 @@ > +/* Test case for template breakpoint test. > + > + Copyright 2023 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + 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/>. */ > + > +template<typename T> > +struct outer > +{ > + template<typename Q = int> > + int fn (int x) > + { > + return x + 23; > + } > +}; > + > +template<typename T = int> > +int toplev (int y) > +{ > + return y; > +} > + > +outer<int> outer1; > +outer<char> outer2; > + > +int main () > +{ > + int x1 = outer1.fn (0); > + int x2 = outer2.fn<short> (-46); > + int x3 = toplev<char> (0); > + int x4 = toplev (0); > + return x1 + x2; > +} > diff --git a/gdb/testsuite/gdb.cp/paramless.exp b/gdb/testsuite/gdb.cp/paramless.exp > new file mode 100644 > index 00000000000..4fc8fd9e015 > --- /dev/null > +++ b/gdb/testsuite/gdb.cp/paramless.exp > @@ -0,0 +1,41 @@ > +# Copyright 2023 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/>. > + > +# This file is part of the gdb testsuite. > + > +# Test template breakpoints without parameters. > + > +if { [skip_cplus_tests] } { continue } > + > +standard_testfile .cc > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} { > + return -1 > +} > + > +gdb_breakpoint "outer<int>::fn" message > +delete_breakpoints > + > +gdb_breakpoint "outer<char>::fn<short>" message > +delete_breakpoints > + > +gdb_test "break outer::fn" "Breakpoint $decimal at .*2 locations." > +delete_breakpoints > + > +gdb_test "break toplev" "Breakpoint $decimal at .*2 locations." > +delete_breakpoints > + > +gdb_breakpoint "toplev<char>" message > +delete_breakpoints > -- > 2.38.1 > -- Joel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-14 6:11 ` Joel Brobecker @ 2023-01-17 13:54 ` Tom Tromey 2023-01-17 16:44 ` Tom de Vries 1 sibling, 0 replies; 19+ messages in thread From: Tom Tromey @ 2023-01-17 13:54 UTC (permalink / raw) To: Joel Brobecker; +Cc: Tom Tromey via Gdb-patches, Tom Tromey >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes: Joel> Thanks for updating the copyright years. Joel> I believe it should be 2022-2023, though. I remember being told that Joel> the start of the copyright year range corresponds to the year the file Joel> was committed to medium. Joel> The patch is still OK for me with the minor correction above. I made this change. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-14 6:11 ` Joel Brobecker 2023-01-17 13:54 ` Tom Tromey @ 2023-01-17 16:44 ` Tom de Vries 2023-01-17 18:46 ` Tom Tromey 1 sibling, 1 reply; 19+ messages in thread From: Tom de Vries @ 2023-01-17 16:44 UTC (permalink / raw) To: Joel Brobecker, Tom Tromey via Gdb-patches; +Cc: Tom Tromey On 1/14/23 07:11, Joel Brobecker via Gdb-patches wrote: > +if { [skip_cplus_tests] } { continue } This uses the old syntax, so we get: ... ERROR: tcl error sourcing src/gdb/testsuite/gdb.cp/paramless.exp. ERROR: invalid command name "skip_cplus_tests" while executing ... Thanks, - Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-17 16:44 ` Tom de Vries @ 2023-01-17 18:46 ` Tom Tromey 0 siblings, 0 replies; 19+ messages in thread From: Tom Tromey @ 2023-01-17 18:46 UTC (permalink / raw) To: Tom de Vries; +Cc: Joel Brobecker, Tom Tromey via Gdb-patches, Tom Tromey >>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes: Tom> On 1/14/23 07:11, Joel Brobecker via Gdb-patches wrote: >> +if { [skip_cplus_tests] } { continue } Tom> This uses the old syntax, so we get: Tom> ... Tom> ERROR: tcl error sourcing src/gdb/testsuite/gdb.cp/paramless.exp. Tom> ERROR: invalid command name "skip_cplus_tests" Tom> while executing Tom> ... Sorry about that. I see now that my test comparison script doesn't account for ERROR / UNRESOLVED :( I'm going to check in the appended. Tom commit fbd6525ba66d9c6c98cf57223d6ad09df1274d67 Author: Tom Tromey <tromey@adacore.com> Date: Tue Jan 17 11:45:40 2023 -0700 Use require in paramless.exp The new paramless.exp test was not converted to the new "require" approach. This patch fixes the problem. diff --git a/gdb/testsuite/gdb.cp/paramless.exp b/gdb/testsuite/gdb.cp/paramless.exp index 79529de49f5..579f363d6a0 100644 --- a/gdb/testsuite/gdb.cp/paramless.exp +++ b/gdb/testsuite/gdb.cp/paramless.exp @@ -17,7 +17,7 @@ # Test template breakpoints without parameters. -if { [skip_cplus_tests] } { continue } +require allow_cplus_tests standard_testfile .cc ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-10 18:33 ` [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader Tom Tromey 2023-01-14 6:11 ` Joel Brobecker @ 2023-01-17 18:09 ` Simon Marchi 2023-01-17 19:39 ` Tom Tromey 1 sibling, 1 reply; 19+ messages in thread From: Simon Marchi @ 2023-01-17 18:09 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 1/10/23 13:33, Tom Tromey via Gdb-patches wrote: > PR c++/29896 points out a regression in the new DWARF reader. It does > not properly handle a case like "break fn", where "fn" is a template > function. > > This happens because the new index uses strncasecmp to compare. > However, to make this work correctly, we need a custom function that > ignores template parameters. > > This patch adds a custom comparison function and fixes the bug. A new > test case is included. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29896 Starting with this patch, I get (using _GLIBCXX_DEBUG): $83 = {void (void)} 0x555555556a82 <flubber<short, int, short, int, short>()> (gdb) PASS: gdb.cp/cpexprs-debug-types.exp: print flubber<short, int, short, int, short> print policy1::function /usr/include/c++/11/bits/stl_algo.h:2105: In function: _FIter std::upper_bound(_FIter, _FIter, const _Tp&, _Compare) [with _FIter = gnu_debug::_Safe_iterator<gnu_cxx:: normal_iterator<cooked_index_entry**, std::vector<cooked_index_entry*, std::allocator<cooked_index_entry*> > >, std:: debug::vector<cooked_index_entry*>, std::random_access_iterator_tag>; _Tp = std::cxx11::basic_string<char>; _Compare = cooked_index::find(const string&, bool)::<lambda(const string&, const cooked_index_entry*)>] Error: elements in iterator range [first, last) are not partitioned by the predicate __comp and value __val. Objects involved in the operation: iterator "first" @ 0x7ffcb7cf5430 { type = gnu_cxx::normal_iterator<cooked_index_entry**, std::vector<cooked_index_entry*, std::allocator<cooked_index_entry*> > > (mutable iterator); state = dereferenceable (start-of-sequence); references sequence with type 'std::debug::vector<cooked_index_entry*, std::allocator<cooked_index_entry*> >' @ 0x611000098758 } iterator "last" @ 0x7ffcb7cf5480 { type = gnu_cxx::normal_iterator<cooked_index_entry**, std::vector<cooked_index_entry*, std::allocator<cooked_index_entry*> > > (mutable iterator); state = past-the-end; references sequence with type 'std::debug::vector<cooked_index_entry*, std::allocator<cooked_index_entry*> >' @ 0x611000098758 } Fatal signal: Aborted ----- Backtrace ----- 0x560e18f13e2f gdb_internal_backtrace_1 /home/smarchi/src/binutils-gdb/gdb/bt-utils.c:122 0x560e18f14325 _Z22gdb_internal_backtracev /home/smarchi/src/binutils-gdb/gdb/bt-utils.c:168 0x560e19b71251 handle_fatal_signal /home/smarchi/src/binutils-gdb/gdb/event-top.c:956 0x7f0ac36a251f ??? 0x7f0ac36f6a7c ??? 0x7f0ac36a2475 ??? 0x7f0ac36887f2 ??? 0x7f0ac4069240 ??? 0x560e1956f665 upper_bound<__gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<cooked_index_entry**, std::__cxx1998::vector<cooked_index_entry*, std::allocator<cooked_index_entry*> > >, std::__debug::vector<cooked_index_entry*>, std::random_access_iterator_tag>, std::__cxx11::basic_string<char>, cooked_index::find(const string&, bool)::<lambda(const string&, const cooked_index_entry*)> > /usr/include/c++/11/bits/stl_algo.h:2105 0x560e1956bd41 _ZN12cooked_index4findERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEb /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:376 0x560e1956d664 _ZN19cooked_index_vector4findERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEEb /home/smarchi/src/binutils-gdb/gdb/dwarf2/cooked-index.c:421 0x560e198a9748 _ZN22cooked_index_functions23expand_symtabs_matchingEP7objfileN3gdb13function_viewIFbPKcbEEEPK16lookup_name_infoNS3_IFbS5_EEENS3_IFbP15compunit_symtabEEE10enum_flagsI24block_search_flag_valuesE11domain_enum13search_domain /home/smarchi/src/binutils-gdb/gdb/dwarf2/read.c:18747 0x560e1adc12c6 _ZN7objfile13lookup_symbolE10block_enumPKc11domain_enum /home/smarchi/src/binutils-gdb/gdb/symfile-debug.c:276 0x560e1ae99109 lookup_symbol_via_quick_fns /home/smarchi/src/binutils-gdb/gdb/symtab.c:2414 0x560e1ae99c80 lookup_symbol_in_objfile /home/smarchi/src/binutils-gdb/gdb/symtab.c:2543 0x560e1ae9a10a operator() /home/smarchi/src/binutils-gdb/gdb/symtab.c:2589 0x560e1aecff72 operator() /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:305 0x560e1aed000c _FUN /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:299 0x560e1a5cb694 _ZNK3gdb13function_viewIFbP7objfileEEclES2_ /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/function-view.h:289 0x560e1ac74071 svr4_iterate_over_objfiles_in_search_order /home/smarchi/src/binutils-gdb/gdb/solib-svr4.c:3456 0x560e18b8b7a1 _Z45gdbarch_iterate_over_objfiles_in_search_orderP7gdbarchN3gdb13function_viewIFbP7objfileEEES4_ /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:4981 0x560e1ae9a8b3 lookup_global_or_static_symbol /home/smarchi/src/binutils-gdb/gdb/symtab.c:2586 0x560e1ae9ade2 _Z20lookup_global_symbolPKcPK5block11domain_enum /home/smarchi/src/binutils-gdb/gdb/symtab.c:2641 0x560e193f3f86 cp_basic_lookup_symbol /home/smarchi/src/binutils-gdb/gdb/cp-namespace.c:155 0x560e193fba75 cp_lookup_nested_symbol_1 /home/smarchi/src/binutils-gdb/gdb/cp-namespace.c:884 0x560e193fc85a _Z23cp_lookup_nested_symbolP4typePKcPK5block11domain_enum /home/smarchi/src/binutils-gdb/gdb/cp-namespace.c:975 0x560e1900fcb0 classify_inner_name /home/smarchi/src/binutils-gdb/gdb/c-exp.y:3172 0x560e1901136e c_yylex /home/smarchi/src/binutils-gdb/gdb/c-exp.y:3322 0x560e18fe729c _Z9c_yyparsev /home/smarchi/build/binutils-gdb/gdb/c-exp.c.tmp:2023 0x560e1901349a _Z7c_parseP12parser_state /home/smarchi/src/binutils-gdb/gdb/c-exp.y:3420 0x560e1a096ec5 _ZNK13language_defn6parserEP12parser_state /home/smarchi/src/binutils-gdb/gdb/language.c:618 0x560e1a637374 parse_exp_in_context /home/smarchi/src/binutils-gdb/gdb/parse.c:515 0x560e1a637b81 _Z16parse_expressionPKcP23innermost_block_trackerb /home/smarchi/src/binutils-gdb/gdb/parse.c:551 0x560e1a64ea7c process_print_command_args /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1305 0x560e1a64ec7c print_command_1 /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1319 0x560e1a64fb18 print_command /home/smarchi/src/binutils-gdb/gdb/printcmd.c:1452 Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-17 18:09 ` Simon Marchi @ 2023-01-17 19:39 ` Tom Tromey 2023-01-27 5:47 ` Simon Marchi 0 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-01-17 19:39 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, gdb-patches >>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: Simon> Starting with this patch, I get (using _GLIBCXX_DEBUG): [...] Ugh, sorry about that. I've reproduce it and I'm working on a fix. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-17 19:39 ` Tom Tromey @ 2023-01-27 5:47 ` Simon Marchi 2023-01-27 10:15 ` Andrew Burgess 0 siblings, 1 reply; 19+ messages in thread From: Simon Marchi @ 2023-01-27 5:47 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 1/17/23 14:39, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: > > Simon> Starting with this patch, I get (using _GLIBCXX_DEBUG): > [...] > > Ugh, sorry about that. > I've reproduce it and I'm working on a fix. > > Tom I started to look into this, using that other patch I sent that dumps the contents of the cooked index. You probably already know this, as you have been looking at the problem already, but it can perhaps help pothers. The specific instance I look at is: $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.cp/cpexprs/cpexprs -ex start -ex "print policy1::function" -batch When searching in the cooked index, the search string (canonicalized version of the input, I think) is: "policy<int, operation_1<void*> >::function" The problem is that the lower bound function used in cooked_index::find (cooked_index_entry::compare) is invalid for these two consecutive entries: [365] ((cooked_index_entry *) 0x621000115080) name: policy canonical: policy DWARF tag: DW_TAG_subprogram flags: 0x0 [] DIE offset: 0x3951 parent: ((cooked_index_entry *) 0x6210001137a0) [policy<int, operation_2<void*> >] [366] ((cooked_index_entry *) 0x621000113740) name: policy<int, operation_1<void*> > canonical: policy<int, operation_1<void*> > DWARF tag: DW_TAG_class_type flags: 0x0 [] DIE offset: 0x22a3 parent: ((cooked_index_entry *) 0) It returns that [365] greater or equal to our search string, but also that [366] is less than our search string. This is a contradiction, because elements are supposed to be sorted according to the comparison function. Simon ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-27 5:47 ` Simon Marchi @ 2023-01-27 10:15 ` Andrew Burgess 2023-01-27 14:30 ` Tom Tromey 0 siblings, 1 reply; 19+ messages in thread From: Andrew Burgess @ 2023-01-27 10:15 UTC (permalink / raw) To: Simon Marchi, Tom Tromey; +Cc: gdb-patches Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > On 1/17/23 14:39, Tom Tromey wrote: >>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes: >> >> Simon> Starting with this patch, I get (using _GLIBCXX_DEBUG): >> [...] >> >> Ugh, sorry about that. >> I've reproduce it and I'm working on a fix. >> >> Tom > > I started to look into this, using that other patch I sent that dumps > the contents of the cooked index. You probably already know this, as > you have been looking at the problem already, but it can perhaps help > pothers. The specific instance I look at is: > > $ ./gdb -nx -q --data-directory=data-directory testsuite/outputs/gdb.cp/cpexprs/cpexprs -ex start -ex "print policy1::function" -batch > > When searching in the cooked index, the search string (canonicalized > version of the input, I think) is: > > "policy<int, operation_1<void*> >::function" > > The problem is that the lower bound function used in > cooked_index::find (cooked_index_entry::compare) is invalid for these > two consecutive entries: > > [365] ((cooked_index_entry *) 0x621000115080) > name: policy > canonical: policy > DWARF tag: DW_TAG_subprogram > flags: 0x0 [] > DIE offset: 0x3951 > parent: ((cooked_index_entry *) 0x6210001137a0) [policy<int, operation_2<void*> >] > > [366] ((cooked_index_entry *) 0x621000113740) > name: policy<int, operation_1<void*> > > canonical: policy<int, operation_1<void*> > > DWARF tag: DW_TAG_class_type > flags: 0x0 [] > DIE offset: 0x22a3 > parent: ((cooked_index_entry *) 0) > > It returns that [365] greater or equal to our search string, but also > that [366] is less than our search string. This is a contradiction, > because elements are supposed to be sorted according to the comparison > function. I've been carrying a patch to work around this issue since the issue was introduced. I've included the patch below, but I don't see it as a permanent fix, though it might help some folk in the short term as a work around. The underlying problem is that we use a different sort predicate for the std::lower_bound and std::upper_bound calls (in some cases) than when we sorted this list. I'm not sure this can ever work correctly. The patch below replaces the lower/upper bound calls with a linear walk of the cooked_index, but this is pretty slow, so much so I had to extend a timeout in one test to avoid it timing out. I also started looking into this more seriously yesterday as I was getting fed up having to port this patch to all my dev branches. Thanks, Andrew --- commit 8094b570486c6a9af4b3223ac7dc42227064cd10 Author: Andrew Burgess <aburgess@redhat.com> Date: Thu Jan 19 18:16:07 2023 +0000 [wip] gdb: temporary fix for the cooked index issue Issue introduced in commit: commit ac37b79cc440e37fc704d425a6e450afb3c7ee89 Date: Wed Dec 14 14:37:41 2022 -0700 Fix parameter-less template regression in new DWARF reader Problem is a use of std::lower_bound with a comparison function that is different to the one used to sort the vector. This changes seems to slow down GDB a fair bit though, this can be seen by the need to adjust gdb.gdb/python-helper.exp. diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c index 09b3fd70b26..48f58f70b0f 100644 --- a/gdb/dwarf2/cooked-index.c +++ b/gdb/dwarf2/cooked-index.c @@ -30,7 +30,7 @@ /* See cooked-index.h. */ -bool +int cooked_index_entry::compare (const char *stra, const char *strb, bool completing) { @@ -58,7 +58,7 @@ cooked_index_entry::compare (const char *stra, const char *strb, consider them as equal. Also, completion mode ignores the special '<' handling. */ if (c1 == '\0' || c2 == '\0') - return false; + return 0; /* Fall through to the generic case. */ } else if (seen_lt) @@ -69,17 +69,31 @@ cooked_index_entry::compare (const char *stra, const char *strb, { /* Maybe they both end at the same spot. */ if (c2 == '\0' || c2 == '<') - return false; + return 0; /* First string ended earlier. */ - return true; + return -1; } else if (c2 == '\0' || c2 == '<') { /* Second string ended earlier. */ - return false; + return 1; } - return c1 < c2; + if (c1 < c2) + return -1; + else if (c1 == c2) + return 0; + else + return 1; +} + +/* See cooked-index.h. */ + +bool +cooked_index_entry::is_equal (const char *stra, const char *strb, + bool completing) +{ + return compare (stra, strb, completing) == 0; } #if GDB_SELF_TEST @@ -89,45 +103,45 @@ namespace { void test_compare () { - SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", false)); - SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", false)); - SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", true)); - SELF_CHECK (!cooked_index_entry::compare ("abcd", "abcd", true)); - - SELF_CHECK (cooked_index_entry::compare ("abcd", "ABCDE", false)); - SELF_CHECK (!cooked_index_entry::compare ("ABCDE", "abcd", false)); - SELF_CHECK (!cooked_index_entry::compare ("abcd", "ABCDE", true)); - SELF_CHECK (!cooked_index_entry::compare ("ABCDE", "abcd", true)); - - SELF_CHECK (!cooked_index_entry::compare ("name", "name<>", false)); - SELF_CHECK (!cooked_index_entry::compare ("name<>", "name", false)); - SELF_CHECK (!cooked_index_entry::compare ("name", "name<>", true)); - SELF_CHECK (!cooked_index_entry::compare ("name<>", "name", true)); - - SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", false)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", false)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<arg>", true)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg>", "name<ag>", true)); - - SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", - "name<arg<more>>", false)); - - SELF_CHECK (!cooked_index_entry::compare ("name", "name<arg<more>>", false)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name", false)); + SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd", true) == 0); + SELF_CHECK (cooked_index_entry::compare ("abcd", "abcd", true) == 0); + + SELF_CHECK (cooked_index_entry::compare ("abcd", "ABCDE", false) < 0); + SELF_CHECK (cooked_index_entry::compare ("ABCDE", "abcd", false) > 0); + SELF_CHECK (cooked_index_entry::compare ("abcd", "ABCDE", true) == 0); + SELF_CHECK (cooked_index_entry::compare ("ABCDE", "abcd", true) == 0); + + SELF_CHECK (cooked_index_entry::compare ("name", "name<>", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<>", "name", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("name", "name<>", true) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<>", "name", true) == 0); + + SELF_CHECK (cooked_index_entry::compare ("name<arg>", "name<arg>", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg>", "name<arg>", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg>", "name<arg>", true) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg>", "name<ag>", true) > 0); + + SELF_CHECK (cooked_index_entry::compare ("name<arg<more>>", + "name<arg<more>>", false) == 0); + + SELF_CHECK (cooked_index_entry::compare ("name", "name<arg<more>>", false) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg<more>>", "name", false) == 0); SELF_CHECK (cooked_index_entry::compare ("name<arg<", "name<arg<more>>", - false)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg<", - "name<arg<more>>", - true)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name<arg<", - false)); - SELF_CHECK (!cooked_index_entry::compare ("name<arg<more>>", "name<arg<", - true)); - - SELF_CHECK (cooked_index_entry::compare ("", "abcd", false)); - SELF_CHECK (!cooked_index_entry::compare ("", "abcd", true)); - SELF_CHECK (!cooked_index_entry::compare ("abcd", "", false)); - SELF_CHECK (!cooked_index_entry::compare ("abcd", "", true)); + false) < 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg<", + "name<arg<more>>", + true) == 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg<more>>", "name<arg<", + false) > 0); + SELF_CHECK (cooked_index_entry::compare ("name<arg<more>>", "name<arg<", + true) == 0); + + SELF_CHECK (cooked_index_entry::compare ("", "abcd", false) < 0); + SELF_CHECK (cooked_index_entry::compare ("", "abcd", true) == 0); + SELF_CHECK (cooked_index_entry::compare ("abcd", "", false) > 0); + SELF_CHECK (cooked_index_entry::compare ("abcd", "", true) == 0); } } /* anonymous namespace */ @@ -352,32 +366,6 @@ cooked_index::do_finalize () }); } -/* See cooked-index.h. */ - -cooked_index::range -cooked_index::find (const std::string &name, bool completing) -{ - wait (); - - auto lower = std::lower_bound (m_entries.begin (), m_entries.end (), name, - [=] (const cooked_index_entry *entry, - const std::string &n) - { - return cooked_index_entry::compare (entry->canonical, n.c_str (), - completing); - }); - - auto upper = std::upper_bound (m_entries.begin (), m_entries.end (), name, - [=] (const std::string &n, - const cooked_index_entry *entry) - { - return cooked_index_entry::compare (n.c_str (), entry->canonical, - completing); - }); - - return range (lower, upper); -} - cooked_index_vector::cooked_index_vector (vec_type &&vec) : m_vector (std::move (vec)) { @@ -417,8 +405,31 @@ cooked_index_vector::find (const std::string &name, bool completing) { std::vector<cooked_index::range> result_range; result_range.reserve (m_vector.size ()); - for (auto &entry : m_vector) - result_range.push_back (entry->find (name, completing)); + + for (auto &cooked_index : m_vector) + { + auto entry_range = cooked_index->all_entries (); + for (auto it = entry_range.begin (); + it < entry_range.end (); + ++it) + { + auto start_it = it; + while (it < entry_range.end () + && cooked_index_entry::is_equal ((*it)->canonical, + name.c_str (), + completing)) + ++it; + + if (start_it != it) + { + cooked_index::range r { start_it, it }; + result_range.push_back (r); + } + + if (it == entry_range.end ()) + break; + } + } return range (std::move (result_range)); } diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h index 55eaf9955ab..b1e4859f6aa 100644 --- a/gdb/dwarf2/cooked-index.h +++ b/gdb/dwarf2/cooked-index.h @@ -143,16 +143,25 @@ struct cooked_index_entry : public allocate_on_obstack STORAGE. */ const char *full_name (struct obstack *storage) const; + /* Compare two strings, case-insensitively. Return -1 if STRA is + less than STRB, 0 if STRA equals STRB, and 1 if STRA is greater than + STRB. + + If one string has template parameters, but the other does not, then + they are considered to be equal; so for example "t<x>" == "t<x>", + "t<x>" < "t<y>", but "t" == "t<x>". */ + static int compare (const char *stra, const char *strb, bool completing); + /* Compare two strings, case-insensitively. Return true if STRA is - less than STRB. If one string has template parameters, but the + equal to STRB. If one string has template parameters, but the other does not, then they are considered to be equal; so for example "t<x>" == "t<x>", "t<x>" < "t<y>", but "t" == "t<x>". */ - static bool compare (const char *stra, const char *strb, bool completing); + static bool is_equal (const char *stra, const char *strb, bool completing); /* Compare two entries by canonical name. */ bool operator< (const cooked_index_entry &other) const { - return compare (canonical, other.canonical, false); + return compare (canonical, other.canonical, false) < 0; } /* The name as it appears in DWARF. This always points into one of @@ -234,11 +243,6 @@ class cooked_index return { m_entries.begin (), m_entries.end () }; } - /* Look up an entry by name. Returns a range of all matching - results. If COMPLETING is true, then a larger range, suitable - for completion, will be returned. */ - range find (const std::string &name, bool completing); - private: /* Return the entry that is believed to represent the program's diff --git a/gdb/testsuite/gdb.gdb/python-helper.exp b/gdb/testsuite/gdb.gdb/python-helper.exp index 98f03ef456f..186ae4eed56 100644 --- a/gdb/testsuite/gdb.gdb/python-helper.exp +++ b/gdb/testsuite/gdb.gdb/python-helper.exp @@ -211,5 +211,7 @@ proc test_python_helper {} { return 0 } -# Use the self-test framework to run the test. -do_self_tests captured_main test_python_helper +with_timeout_factor 3 { + # Use the self-test framework to run the test. + do_self_tests captured_main test_python_helper +} ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-27 10:15 ` Andrew Burgess @ 2023-01-27 14:30 ` Tom Tromey 2023-01-27 19:57 ` Tom Tromey 0 siblings, 1 reply; 19+ messages in thread From: Tom Tromey @ 2023-01-27 14:30 UTC (permalink / raw) To: Andrew Burgess via Gdb-patches; +Cc: Simon Marchi, Tom Tromey, Andrew Burgess >>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes: Sorry I haven't gotten back to fixing this. I've been pretty busy with a project at work, my other patches have all been rote stuff. Andrew> The underlying problem is that we use a different sort predicate for the Andrew> std::lower_bound and std::upper_bound calls (in some cases) than when we Andrew> sorted this list. I'm not sure this can ever work correctly. Yeah, that was my conclusion as well. My plan is to sort more naively again (writing our own strcasecmp though to avoid the performance thing on Windows), then have either the 'find' method or the caller in read.c do an extra filtering. Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader 2023-01-27 14:30 ` Tom Tromey @ 2023-01-27 19:57 ` Tom Tromey 0 siblings, 0 replies; 19+ messages in thread From: Tom Tromey @ 2023-01-27 19:57 UTC (permalink / raw) To: Tom Tromey Cc: Andrew Burgess via Gdb-patches, Simon Marchi, Tom Tromey, Andrew Burgess Andrew> The underlying problem is that we use a different sort predicate for the Andrew> std::lower_bound and std::upper_bound calls (in some cases) than when we Andrew> sorted this list. I'm not sure this can ever work correctly. Tom> Yeah, that was my conclusion as well. Well, after talking to Simon on irc, I think my conclusion was wrong. I've sent a patch to fix the problem. Please take a look. thanks, Tom ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-01-27 19:57 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-10 18:33 [PATCH v2 0/4] Fix regression in new DWARF reader Tom Tromey 2023-01-10 18:33 ` [PATCH v2 1/4] Avoid submitting empty tasks in parallel_for_each Tom Tromey 2023-01-14 6:03 ` Joel Brobecker 2023-01-10 18:33 ` [PATCH v2 2/4] Don't erase empty indices in DWARF reader Tom Tromey 2023-01-14 6:05 ` Joel Brobecker 2023-01-17 13:53 ` Tom Tromey 2023-01-10 18:33 ` [PATCH v2 3/4] Move hash_entry and eq_entry into cooked_index::do_finalize Tom Tromey 2023-01-14 6:06 ` Joel Brobecker 2023-01-10 18:33 ` [PATCH v2 4/4] Fix parameter-less template regression in new DWARF reader Tom Tromey 2023-01-14 6:11 ` Joel Brobecker 2023-01-17 13:54 ` Tom Tromey 2023-01-17 16:44 ` Tom de Vries 2023-01-17 18:46 ` Tom Tromey 2023-01-17 18:09 ` Simon Marchi 2023-01-17 19:39 ` Tom Tromey 2023-01-27 5:47 ` Simon Marchi 2023-01-27 10:15 ` Andrew Burgess 2023-01-27 14:30 ` Tom Tromey 2023-01-27 19:57 ` Tom Tromey
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).