From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A2CAE3858C66 for ; Thu, 12 Jan 2023 19:06:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A2CAE3858C66 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1673550369; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=LVybr5tnl+haUZmQh2exxKdkogUQRat4SCwRZTQsEI8=; b=Hlhc6TCvizs43AEP3mq7zNh4Tx+Pm4AyKlynyg/3QHtf2PYLEKaHTizQnjtEsIPwvQD3zP tmbJzfwZx8+0GCrn8jHaZ3Oz9YY4B/X0r4V7iK5Ex9PPRv5pkS5fWt3ZhwNErB/sbWyENg 5y8dG9nMZpqwMzDmm75WBmfYmsZllAM= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-96-zMPRIcuaNAGMUlAnm8aBWA-1; Thu, 12 Jan 2023 14:06:06 -0500 X-MC-Unique: zMPRIcuaNAGMUlAnm8aBWA-1 Received: by mail-wr1-f71.google.com with SMTP id u4-20020adfc644000000b002ad64393461so3761562wrg.7 for ; Thu, 12 Jan 2023 11:06:05 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LVybr5tnl+haUZmQh2exxKdkogUQRat4SCwRZTQsEI8=; b=ptjwlg94DPqEiQxPK9FuqX3bCRlJcT3Yl/D+X1AYf1gNVwDoMHj0H0VPg/8gK/9fWw gLVWsSIe2j6NGeo6T/xNZZxPpoI7l+g1+wkGrBeRx681uzOgB1+KQKa4ZQmSzgKNOY+m vrWhZKVuox+tGrNBQ9HLTQ5AMn510Nsz02Fww5vo6+E1uT/DhFOxLoOauCMWMy+NhzC2 gEvI7nlmP0SMaiDnwWpdUkk1QgBHMKu41xFTt5vCFDHmM3ICRDiwnjThJyqQ8AntVDcM gv3IxnI2tCIK26yeKwb68/57gVVE7iEmwRW/3OeyjqMuTUnSpmxZhVfJoXsdeUpImS2W 7Ofw== X-Gm-Message-State: AFqh2kpcflQKcSAbiqhJB/3tJW06BEIyMjHL/wCYDSDRBa+KK8lqEE26 anqfVKh2EBqNG2Eb6RSSY7K5P9+xMNnhnA8yPy4bS/RSSejxV7Ycazd/b/CtcfCFbtdwIy7V22R K5ieQ7PmH4nXynNWOc8Q8YA== X-Received: by 2002:a05:600c:2e44:b0:3d3:4406:8a3c with SMTP id q4-20020a05600c2e4400b003d344068a3cmr55712523wmf.32.1673550364654; Thu, 12 Jan 2023 11:06:04 -0800 (PST) X-Google-Smtp-Source: AMrXdXvGpmp8BztlItDERkUTwlK+wODKzDR4D2lrxCXVtI7MYMrtZJhIsfSIReidAjxZujFoqQ1NQg== X-Received: by 2002:a05:600c:2e44:b0:3d3:4406:8a3c with SMTP id q4-20020a05600c2e4400b003d344068a3cmr55712500wmf.32.1673550364298; Thu, 12 Jan 2023 11:06:04 -0800 (PST) Received: from localhost (92.40.218.34.threembb.co.uk. [92.40.218.34]) by smtp.gmail.com with ESMTPSA id q1-20020a1ce901000000b003b3307fb98fsm21864045wmc.24.2023.01.12.11.06.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Jan 2023 11:06:03 -0800 (PST) From: Andrew Burgess To: Zheng Cc: Tom Tromey , Zheng Zhan Liang via Gdb-patches , Zheng Zhan Subject: 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 "::parse::h3bbecc5bbd82b347", second = 0x7fffd8173cc5 "::parse::h3bbecc5bbd82b347" } In-Reply-To: <52ab60b3.2c1b.1858b741466.Coremail.linuxmaker@163.com> References: <20230102124334.120786-1-linuxmaker@163.com> <875ydkd911.fsf@tromey.com> <548304b6.2be2.185850681f6.Coremail.linuxmaker@163.com> <87lemfy7zn.fsf@redhat.com> <52ab60b3.2c1b.1858b741466.Coremail.linuxmaker@163.com> Date: Thu, 12 Jan 2023 19:06:02 +0000 Message-ID: <875ydbh8th.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Zheng writes: > At 2023-01-06 23:53:48, "Andrew Burgess" wrote: >>linuxmaker via Gdb-patches writes: >> >>> At 2023-01-06 04:24:42, "Tom Tromey" wrote: >>>>>>>>> "Zheng" == Zheng Zhan Liang via Gdb-patches writes: >>>> >>>>Zheng> From: Zheng Zhan >>>> >>>>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 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::::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 '::::' 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::::parse str::::parse ::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)' 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 '::parse', the first thing it sees is '', GDB assumes this is a template argument and records this as a skipped region within the completion_match_for_lcd object. After '' 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 '' 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 '' 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::::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 struct foo { template foo (int p1, T a) { s = 0; } S s; }; int main () { foo 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::foo(int, double) When GDB enters cp_symbol_name_matches_1 the symbols it examines are: foo::foo(int, double) foo(int, double) The first iteration of the loop will match the 'foo', then add the '' template argument will be added as a skip range. When GDB find the ':' after the '' the first iteration of the loop fails to match, GDB removes the 'foo::' component, and starts the second iteration of the loop. Again, GDB matches the 'foo', and now adds '' 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 '' 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 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 (); + 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::baz(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 +struct baz +{ + template + baz (int p1, T a) + { + s = 0; + } + + S s; +}; + int main () { + baz 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 . + +# 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 . + +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