Re:Re:Re:Re: [PATCH] I'm debugging https://github.com/helix-editor/helix.git@63dcaae1b9083396fb3faaef9eaa2421f7e48fb9, which is a editor implemented with rust lang. When I type gdb command below: (gdb) b pars gdb dumped. I got: m_match = 0x7fffd8173cc7 "parse::h3bbecc5bbd82b347" m_ignored_ranges = { first = 0x7fffd8173cbb "<impl str>::parse::h3bbecc5bbd82b347", second = 0x7fffd8173cc5 "::parse::h3bbecc5bbd82b347" }

[Andrew Burgess <aburgess@redhat.com>]

Zheng  <linuxmaker@163.com> writes:

> At 2023-01-13 03:06:02, "Andrew Burgess" <aburgess@redhat.com> wrote:
>>Zheng <linuxmaker@163.com> writes:
>>
>>> At 2023-01-06 23:53:48, "Andrew Burgess" <aburgess@redhat.com> wrote:
>>>>linuxmaker via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>>
>>>>> At 2023-01-06 04:24:42, "Tom Tromey" <tom@tromey.com> wrote:
>>>>>>>>>>> "Zheng" == Zheng Zhan Liang via Gdb-patches <gdb-patches@sourceware.org> writes:
>>>>>>
>>>>>>Zheng> From: Zheng Zhan <zzlossdev@163.com>
>>>>>>
>>>>>>Hi.  Thank you for the patch.
>>>>>>
>>>>>>The text all seemed to end up in the Subject line.  Probably you need
>>>>>>another newline after the first line of the commit message.
>>>>>>
>>>>>>Zheng> --- a/gdb/completer.h
>>>>>>Zheng> +++ b/gdb/completer.h
>>>>>>Zheng> @@ -163,8 +163,11 @@ class completion_match_for_lcd
>>>>>>Zheng>  	const char *prev = m_match;
>>>>>>Zheng>  	for (const auto &range : m_ignored_ranges)
>>>>>>Zheng>  	  {
>>>>>>Zheng> -	    m_finished_storage.append (prev, range.first);
>>>>>>Zheng> -	    prev = range.second;
>>>>>>Zheng> +            if (prev < range.first)
>>>>>>Zheng> +              {
>>>>>>Zheng> +                m_finished_storage.append (prev, range.first);
>>>>>>Zheng> +                prev = range.second;
>>>>>>Zheng> +              }
>>>>>>
>>>>>>Is there any way to construct a test case for this?
>>>>> seems pretty common in rust. you can test it like below:
>>>>> fn main() {
>>>>>     let four: u8 = "4".parse().unwrap();
>>>>>     println!("{}", four);
>>>>> }
>>>>> (gdb) b pars[TAB]
>>>>
>>>>I haven't reviewed the fix.  But in case this makes it easier for
>>> I've done a little more digging. And It looks like `strncmp_iw_with_mode` cached
>>> the last `mark_ignored_range` setting,  then crap the current one.
>>> I paste a newer patch at the bottom, and it seems work for me.
>>
>>Thanks, I was also looking at the same area of code, wondering if that
>>would be a better place for the fix.
>>
>>I'd actually like to propose that the fix be moved one frame up the
>>stack to cp_symbol_name_matches_1, my reasons are explained in the patch
>>below.
>>
>>Additionally, I've now managed to reproduce the crash when completing a
>>C++ symbol, which means we have a much more reliable test - it doesn't
>>depend on symbols being pulled in from the rust runtime, which might
>>change in the future.
>>
>>The patch below is my proposal for what we should merge.
>>
>>All feedback welcome.
>>
>>Thanks,
>>Andrew
>>
>>--
>>
>>commit 467065c7a25a6fc87fab6c73ac6d7d509aae81b6
>>Author: Andrew Burgess <aburgess@redhat.com>
>>Date:   Fri Jan 6 15:50:26 2023 +0000
>>
>>    gdb: fix crash during command completion
>>    
>>    In some cases GDB will fail when attempting to complete a command that
>>    involves a rust symbol, the failure can manifest as a crash.
>>    
>>    The problem is caused by the completion_match_for_lcd object being
>>    left containing invalid data during calls to cp_symbol_name_matches_1.
>>    
>>    The first question to address is why we are calling a C++ support
>>    function when handling a rust symbol.  That's due to GDB's auto
>>    language detection for msymbols, in some cases GDB can't tell if a
>>    symbol is a rust symbol, or a C++ symbol.
>>    
>>    The test application contains symbols for functions which are
>>    statically linked in from various rust support libraries.  There's no
>>    DWARF for these symbols, so all GDB has is the msymbols built from the
>>    ELF symbol table.
>>    
>>    Here's the problematic symbol that leads to our crash:
>>    
>>        mangled: _ZN4core3str21_$LT$impl$u20$str$GT$5parse17h5111d2d6a50d22bdE
>>      demangled: core::str::<impl str>::parse
>>    
>>    As an msymbol this is initially created with language auto, then GDB
>>    eventually calls symbol_find_demangled_name, which loops over all
>>    languages calling language_defn::sniff_from_mangled_name, the first
>>    language that can demangle the symbol gets assigned as the language
>>    for that symbol.
>>    
>>    Unfortunately, there's overlap in the mangled symbol names,
>>    some (legacy) rust symbols can be demangled as both rust and C++, see
>>    cplus_demangle in libiberty/cplus-dem.c where this is mentioned.
>>    
>>    And so, because we check the C++ language before we check for rust,
>>    then the msymbol is (incorrectly) given the C++ language.
>>    
>>    Now it's true that is some cases we might be able to figure out that a
>>    demangled symbol is not actually a valid C++ symbol, for example, in
>>    our case, the construct '::<impl str>::' is not, I believe, valid in a
>>    C++ symbol, we could look for ':<' and '>:' and refuse to accept this
>>    as a C++ symbol.
>>    
>>    However, I'm not sure it is always possible to tell that a demangled
>>    symbol is rust or C++, so, I think, we have to accept that some times
>>    we will get this language detection wrong.
>>    
>>    If we accept that we can't fix the symbol language detection 100% of
>>    the time, then we should make sure that GDB doesn't crash when it gets
>>    the language wrong, that is what this commit addresses.
>>    
>>    In our test case the user tries to complete a symbol name like this:
>>    
>>      (gdb) complete break pars
>>    
>>    This results in GDB trying to find all symbols that match 'pars',
>>    eventually we consider our problematic symbol, and we end up with a
>>    call stack that looks like this:
>>    
>>      #0  0x0000000000f3c6bd in strncmp_iw_with_mode
>>      #1  0x0000000000706d8d in cp_symbol_name_matches_1
>>      #2  0x0000000000706fa4 in cp_symbol_name_matches
>>      #3  0x0000000000df3c45 in compare_symbol_name
>>      #4  0x0000000000df3c91 in completion_list_add_name
>>      #5  0x0000000000df3f1d in completion_list_add_msymbol
>>      #6  0x0000000000df4c94 in default_collect_symbol_completion_matches_break_on
>>      #7  0x0000000000658c08 in language_defn::collect_symbol_completion_matches
>>      #8  0x0000000000df54c9 in collect_symbol_completion_matches
>>      #9  0x00000000009d98fb in linespec_complete_function
>>      #10 0x00000000009d99f0 in complete_linespec_component
>>      #11 0x00000000009da200 in linespec_complete
>>      #12 0x00000000006e4132 in complete_address_and_linespec_locations
>>      #13 0x00000000006e4ac3 in location_completer
>>    
>>    In cp_symbol_name_matches_1 we enter a loop, this loop repeatedly
>>    tries to match the demangled problematic symbol name against the user
>>    supplied text ('pars').  Each time around the loop another component
>>    of the symbol name is stripped off, thus, we check 'pars' against
>>    these options:
>>    
>>      core::str::<impl str>::parse
>>      str::<impl str>::parse
>>      <impl str>::parse
>>      parse
>>    
>>    As soon as we get a match the cp_symbol_name_matches_1 exits its loop
>>    and returns.  In our case, when we're looking for 'pars', the match
>>    occurs on the last iteration of the loop, when we are comparing to
>>    'parse'.
>>    
>>    Now the problem here is that cp_symbol_name_matches_1 uses the
>>    strncmp_iw_with_mode, and inside strncmp_iw_with_mode we allow for
>>    skipping over template parameters.  This allows GDB to match the
>>    symbol name 'foo<int>(int,int)' if the user supplies 'foo(int,'.
>>    Inside strncmp_iw_with_mode GDB will record any template arguments
>>    that it has skipped over inside the completion_match_for_lcd object
>>    that is passed in as an argument.
>>    
>>    And so, when GDB tries to match against '<impl str>::parse', the first
>>    thing it sees is '<impl str>', GDB assumes this is a template argument
>>    and records this as a skipped region within the
>>    completion_match_for_lcd object.  After '<impl str>' GDB sees a ':'
>>    character, which doesn't match with the 'pars' the user supplied, so
>>    strncmp_iw_with_mode returns a value indicating a non-match.  GDB then
>>    removes the '<impl str>' component from the symbol name and tries
>>    again, this time comparing to 'parse', which does match.
>>    
>>    Having found a match, then in cp_symbol_name_matches_1 we record the
>>    match string, and the full symbol name within the
>>    completion_match_result object, and return.
>>    
>>    The problem here is that the skipped region, the '<impl str>' that we
>>    recorded in the penultimate loop iteration was never discarded, its
>>    still there in our returned result.
>>    
>>    If we look at what the pointers held in the completion_match_result
>>    that cp_symbol_name_matches_1 returns, this is what we see:
>>    
>>      core::str::<impl str>::parse
>>      |          \________/  |
>>      |               |      '--- completion match string
>>      |               '---skip range
>>      '--- full symbol name
>>    
>>    When GDB calls completion_match_for_lcd::finish, GDB tries to create a
>>    string using the completion match string (parse), but excluding the
>>    skip range, as the stored skip range is before the start of the
>>    completion match string, then GDB tries to do some weird string
>>    creation, which will cause GDB to crash.
>>    
>>    The reason we don't often see this problem in C++ is that for C++
>>    symbols there is always some non-template text before the template
>>    argument.  This non-template text means GDB is likely to either match
>>    the symbol, or reject the symbol without storing a skip range.
>>    
>>    However, notice, I did say, we don't often see this problem.  Once I
>>    understood the issue, I was able to reproduce the crash using a pure
>>    C++ example:
>>    
>>      template<typename S>
>>      struct foo
>>      {
>>        template<typename T>
>>        foo (int p1, T a)
>>        {
>>          s = 0;
>>        }
>>    
>>        S s;
>>      };
>>    
>>      int
>>      main ()
>>      {
>>        foo<int> obj (2.3, 0);
>>        return 0;
>>      }
>>    
>>    Then in GDB:
>>    
>>      (gdb) complete break foo(int
>>    
>>    The problem here is that the C++ symbol for the constructor looks like
>>    this:
>>    
>>      foo<int>::foo<double>(int, double)
>>    
>>    When GDB enters cp_symbol_name_matches_1 the symbols it examines are:
>>    
>>      foo<int>::foo<double>(int, double)
>>      foo<double>(int, double)
>>    
>>    The first iteration of the loop will match the 'foo', then add the
>>    '<int>' template argument will be added as a skip range.  When GDB
>>    find the ':' after the '<int>' the first iteration of the loop fails
>>    to match, GDB removes the 'foo<int>::' component, and starts the
>>    second iteration of the loop.
>>    
>>    Again, GDB matches the 'foo', and now adds '<double>' as a skip
>>    region.  After that the '(int' successfully matches, and so the second
>>    iteration of the loop succeeds, but, once again we left the '<int>' in
>>    place as a skip region, even though this occurs before the start of
>>    our match string, and this will cause GDB to crash.
>>    
>>    This problem was reported to the mailing list, and a solution
>>    discussed in this thread:
>>    
>>      https://sourceware.org/pipermail/gdb-patches/2023-January/195166.html
>>    
>>    The solution proposed here is similar to one proposed by the original
>>    bug reported, but implemented in a different location within GDB.
>>    Instead of placing the fix in strncmp_iw_with_mode, I place the fix in
>>    cp_symbol_name_matches_1.  I believe this is a better location as it
>>    is this function that implements the loop, and it is this loop, which
>>    repeatedly calls strncmp_iw_with_mode, that should be resetting the
>>    result object state (I believe).
>>    
>>    What I have done is add an assert to strncmp_iw_with_mode that the
>>    incoming result object is empty.
>>    
>>    I've also added some other asserts in related code, in
>>    completion_match_for_lcd::mark_ignored_range, I make some basic
>>    assertions about the incoming range pointers, and in
>>    completion_match_for_lcd::finish I also make some assertions about how
>>    the skip ranges relate to the match pointer.
>>    
>>    There's two new tests.  The original rust example that was used in the
>>    initial bug report, and a C++ test.  The rust example depends on which
>>    symbols are pulled in from the rust libraries, so it is possible that,
>>    at some future date, the problematic symbol will disappear from this
>>    test program.  The C++ test should be more reliable, as this only
>>    depends on symbols from within the C++ source code.
>>    
>>    Co-Authored-By:  Zheng Zhan <zzlossdev@163.com>
>>
>>diff --git a/gdb/completer.h b/gdb/completer.h
>>index 8b4ad8ec4d4..67d2fbf9d38 100644
>>--- a/gdb/completer.h
>>+++ b/gdb/completer.h
>>@@ -145,7 +145,12 @@ class completion_match_for_lcd
>> 
>>   /* Mark the range between [BEGIN, END) as ignored.  */
>>   void mark_ignored_range (const char *begin, const char *end)
>>-  { m_ignored_ranges.emplace_back (begin, end); }
>>+  {
>>+    gdb_assert (begin < end);
>>+    gdb_assert (m_ignored_ranges.empty ()
>>+		|| m_ignored_ranges.back ().second < begin);
>>+    m_ignored_ranges.emplace_back (begin, end);
>>+  }
>> 
>>   /* Get the resulting LCD, after a successful match.  If there are
>>      ignored ranges, then this builds a new string with the ignored
>>@@ -160,9 +165,14 @@ class completion_match_for_lcd
>>       {
>> 	m_finished_storage.clear ();
>> 
>>+	gdb_assert (m_ignored_ranges.back ().second
>>+		    <= (m_match + strlen (m_match)));
>>+
>> 	const char *prev = m_match;
>> 	for (const auto &range : m_ignored_ranges)
>> 	  {
>>+	    gdb_assert (prev < range.first);
>>+	    gdb_assert (range.second > range.first);
>> 	    m_finished_storage.append (prev, range.first);
>> 	    prev = range.second;
>> 	  }
>>@@ -179,6 +189,13 @@ class completion_match_for_lcd
>>     m_ignored_ranges.clear ();
>>   }
>> 
>>+  /* Return true if this object has had no match data set since its
>>+     creation, or the last call to clear.  */
>>+  bool empty () const
>>+  {
>>+    return m_match == nullptr && m_ignored_ranges.empty ();
>>+  }
>>+
>> private:
>>   /* The completion match result for LCD.  This is usually either a
>>      pointer into to a substring within a symbol's name, or to the
>>diff --git a/gdb/cp-support.c b/gdb/cp-support.c
>>index b7ed2101b78..fb5758ff914 100644
>>--- a/gdb/cp-support.c
>>+++ b/gdb/cp-support.c
>>@@ -1773,6 +1773,8 @@ cp_symbol_name_matches_1 (const char *symbol_search_name,
>>   completion_match_for_lcd *match_for_lcd
>>     = (comp_match_res != NULL ? &comp_match_res->match_for_lcd : NULL);
>> 
>>+  gdb_assert (match_for_lcd == nullptr || match_for_lcd->empty ());
>>+
>>   while (true)
>>     {
>>       if (strncmp_iw_with_mode (sname, lookup_name, lookup_name_len,
>>@@ -1806,6 +1808,11 @@ cp_symbol_name_matches_1 (const char *symbol_search_name,
>> 	  return true;
>> 	}
>> 
>>+      /* Clear match_for_lcd so the next strncmp_iw_with_mode call starts
>>+	 from scratch.  */
>>+      if (match_for_lcd != nullptr)
>>+	match_for_lcd->clear ();
> Well done. I have a question here, what if the `match_for_lcd` comes from the final call of
> `strncmp_iw_with_mode` and what's it contain matters?

Within cp_symbol_name_matches_1, if the strnmcp_iw_with_mode call finds
a match then we always return before the new call to ::clear.  We only
call clear if strnmcp_iw_with_mode fails to find a match and we then
move onto the next part of the match.

>                                                         Currently, I find two more references to
> `strnmcp_iw_with_mode`. Are we going to fix each one outsidely?

I don't believe that these calls need fixing.  The problem with
cp_symbol_name_matches_1 is that the strnmcp_iw_with_mode call is in a
loop, we effectively have multiple calls to cp_symbol_name_matches_1,
and we only want the results from the last successful call.

But in case this ever changes, I added an assert to
strnmcp_iw_with_mode, this will trigger if we ever call that function
and the completion_match_for_lcd already has some results.

Thanks,
Andrew

>
> Thanks,
> Zheng
>>+
>>       unsigned int len = cp_find_first_component (sname);
>> 
>>       if (sname[len] == '\0')
>>diff --git a/gdb/testsuite/gdb.cp/cpcompletion.exp b/gdb/testsuite/gdb.cp/cpcompletion.exp
>>index 931f376a23d..48692dbc1f8 100644
>>--- a/gdb/testsuite/gdb.cp/cpcompletion.exp
>>+++ b/gdb/testsuite/gdb.cp/cpcompletion.exp
>>@@ -135,3 +135,5 @@ with_test_prefix "expression with namespace" {
>>     # Add a disambiguating character and we get a unique completion.
>>     test_gdb_complete_unique "p Test_NS::f" "p Test_NS::foo"
>> }
>>+
>>+test_gdb_complete_unique "break baz(int" "break baz(int, double)"
>>diff --git a/gdb/testsuite/gdb.cp/pr9594.cc b/gdb/testsuite/gdb.cp/pr9594.cc
>>index 54ddaafc0ca..b854d810b11 100644
>>--- a/gdb/testsuite/gdb.cp/pr9594.cc
>>+++ b/gdb/testsuite/gdb.cp/pr9594.cc
>>@@ -52,8 +52,31 @@ int qux;
>> 
>> } /* namespace Test_NS */
>> 
>>+/* The important thing with class baz is that both the class and the
>>+   constructor must have a template argument, we need the symbol to look
>>+   like:
>>+
>>+   baz<TYPE_1>::baz<TYPE_2>(int,....whatever...)
>>+
>>+   It doesn't really matter if TYPE_1 and TYPE_2 are the same or different,
>>+   but we create them differently in this test as it makes debugging GDB
>>+   slightly easier.  */
>>+
>>+template<typename S>
>>+struct baz
>>+{
>>+  template<typename T>
>>+  baz (int p1, T a)
>>+  {
>>+    s = 0;
>>+  }
>>+
>>+  S s;
>>+};
>>+
>> int main ()
>> {
>>+  baz<int> obj (2.3, 0.1);
>>   // Anonymous struct with method.
>>   struct {
>>     int get() { return 5; }
>>diff --git a/gdb/testsuite/gdb.rust/completion.exp b/gdb/testsuite/gdb.rust/completion.exp
>>new file mode 100644
>>index 00000000000..00923db14a6
>>--- /dev/null
>>+++ b/gdb/testsuite/gdb.rust/completion.exp
>>@@ -0,0 +1,34 @@
>>+# Copyright (C) 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/>.
>>+
>>+# Test a completion case that was causing GDB to crash.
>>+
>>+load_lib rust-support.exp
>>+if {[skip_rust_tests]} {
>>+    return
>>+}
>>+
>>+standard_testfile .rs
>>+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug rust}]} {
>>+    return -1
>>+}
>>+
>>+set line [gdb_get_line_number "set breakpoint here"]
>>+if {![runto ${srcfile}:$line]} {
>>+    untested "could not run to breakpoint"
>>+    return -1
>>+}
>>+
>>+gdb_test "complete break pars" ".*"
>>diff --git a/gdb/testsuite/gdb.rust/completion.rs b/gdb/testsuite/gdb.rust/completion.rs
>>new file mode 100644
>>index 00000000000..8396e3f4086
>>--- /dev/null
>>+++ b/gdb/testsuite/gdb.rust/completion.rs
>>@@ -0,0 +1,19 @@
>>+// Copyright (C) 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/>.
>>+
>>+fn main() {
>>+    let four: u8 = "4".parse().unwrap();
>>+    println!("{}", four);	// set breakpoint here
>>+}
>>diff --git a/gdb/utils.c b/gdb/utils.c
>>index 734c5bf7f70..ab14e186821 100644
>>--- a/gdb/utils.c
>>+++ b/gdb/utils.c
>>@@ -2139,6 +2139,8 @@ strncmp_iw_with_mode (const char *string1, const char *string2,
>> 			|| language == language_rust
>> 			|| language == language_fortran);
>> 
>>+  gdb_assert (match_for_lcd == nullptr || match_for_lcd->empty ());
>>+
>>   while (1)
>>     {
>>       if (skip_spaces