[v2,4/4] Fix parameter-less template regression in new DWARF reader

  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
  

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" == 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
  

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
  

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
  

>>>>> "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
  

>>>>> "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
  

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
  

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
+}
  

>>>>> "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
  

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