From patchwork Tue Sep 1 07:35:02 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pierre-Marie de Rodat X-Patchwork-Id: 8541 Received: (qmail 17236 invoked by alias); 1 Sep 2015 07:35:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 17225 invoked by uid 89); 1 Sep 2015 07:35:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 required=5.0 tests=AWL, BAYES_50, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: smtp.eu.adacore.com Received: from mel.act-europe.fr (HELO smtp.eu.adacore.com) (194.98.77.210) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 01 Sep 2015 07:35:05 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id D250728F2DFE; Tue, 1 Sep 2015 09:35:02 +0200 (CEST) Received: from smtp.eu.adacore.com ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4PlcIccvFHxp; Tue, 1 Sep 2015 09:35:02 +0200 (CEST) Received: from [10.10.1.112] (cacatoes.act-europe.fr [10.10.1.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.eu.adacore.com (Postfix) with ESMTPSA id 9C18C28F2DF9; Tue, 1 Sep 2015 09:35:02 +0200 (CEST) Subject: Re: [PATCH] [Ada] Fix completion for multiple function matches To: Joel Brobecker References: <1441034710-31810-1-git-send-email-derodat@adacore.com> <20150831172516.GA3356@adacore.com> Cc: gdb-patches@sourceware.org From: Pierre-Marie de Rodat Message-ID: <55E55526.2000303@adacore.com> Date: Tue, 1 Sep 2015 09:35:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <20150831172516.GA3356@adacore.com> X-IsSubscribed: yes On 08/31/2015 07:25 PM, Joel Brobecker wrote: > Overall, the patch looks like definite progress as is, and I'd like > to see it pushed to avoid both the unsollicited MCQ and the ensuing > SEGV. However, I'm wondering if it might still be leaving the hole > in the completion. > > In particular, what happens to the completion of "p ambig" if > you add a second function called, say "Aux_Pck.Ambiguous_Prog"? > In other words, I am wondering if the fact that we're returning > zero when parsing for completion might not unexpectedly filter out > some matches. Regardless of the answer, that's probably a worthwhile > adjustment to the tests you propose. I adjusted the testcase in the way I think you meant: I added the Ambiguous_Prog procedure to Aux_Pck and added a test to ask the completion of "p ambig": GDB correctly reports two possible completions: "p ambiguous_func" and "p ambiguous_proc". This looks independent of function resolution in ada-lang, and that is fortunate in our case. :-) Is this what you had in mind? > So, here's the plan I propose: We adjust the very minor suggestions > I made below, adjust the testcase as suggested above, do a quick > second round of review and then push the patch. If if turns out > there is a hole (I am not sure), then we can put that on the list > of things to improve and decide when we want to look at it. Sure: here's the updated patch, re-regtested on x86_64-linux. >> + /* If we got multiple matches, ask the user which one to use. Don't do this >> + interactive thing during completion, though: it is irritating to get a >> + prompt in this context. */ > > I wouldn't say "irritating" as, to me, this is a bug, not an > inconvenience. So, I would change the comment to say something like: > > /* If we got multiple matches, ask the user which one to use. Don't do this > interactive thing during completion, though, as the purpose of the > completion is providing a list of all possible matches. Prompting the > user to filter it down would be completely unexpected in this case. */ Done. >> +# Usually, parsing a function name that is ambiguous yields a menu through >> +# which users can select a specific function. This should not happen during >> +# completion, though. > > Missing second space after a period. Done. >> +test_gdb_complete "ambig" \ >> + "p ambiguous_func" >> +test_gdb_complete "ambiguous_func" \ >> + "p ambiguous_func" >> diff --git a/gdb/testsuite/gdb.ada/complete/aux_pck.adb b/gdb/testsuite/gdb.ada/complete/aux_pck.adb >> new file mode 100644 >> index 0000000..18e060e >> --- /dev/null >> +++ b/gdb/testsuite/gdb.ada/complete/aux_pck.adb >> @@ -0,0 +1,23 @@ >> +-- Copyright 2008-2015 Free Software Foundation, Inc. > > I *think* you want to only list 2015 in this case. Unless I missed > the fact that you copied this file from elsewhere... Copy-paste error, indeed. ;-) Done. >> diff --git a/gdb/testsuite/gdb.ada/complete/aux_pck.ads b/gdb/testsuite/gdb.ada/complete/aux_pck.ads >> new file mode 100644 >> index 0000000..36fdb6c >> --- /dev/null >> +++ b/gdb/testsuite/gdb.ada/complete/aux_pck.ads >> @@ -0,0 +1,20 @@ >> +-- Copyright 2008-2015 Free Software Foundation, Inc. > > Same here. Likewise. Thank you for reviewing! From 1ce5e145bd6147507fcae4fb614b6cb4fdefd2bc Mon Sep 17 00:00:00 2001 From: Pierre-Marie de Rodat Date: Mon, 31 Aug 2015 16:04:07 +0200 Subject: [PATCH] [Ada] Fix completion for multiple function matches Before this change, trying to complete an expression ending with an ambiguous function name (i.e. for which there are multiple matches) would display a menu with a prompt for the user to pick one. For instance: (gdb) p funcMultiple matches for func [0] cancel [1] pack2.func at pack2.adb:5 [2] pack.func at pack.adb:5 > This is not user friendly and actually triggered a segmentation fault after the user did pick one. It is not clear whether the segmentation fault needs a separate fix, but this is the only known case which exhibits it at the moment, and this case must be fixed itself. The problem lies in ada-lang.c (ada_resolve_function): when we got multiple matches, we should not display the menu if we are in completion mode. This patch adjusts the corresponding condition accordingly. gdb/ChangeLog: * ada-lang.c (ada_resolve_function): Do not ask the user what match to use when in completion mode. gdb/testsuite/ChangeLog: * gdb.ada/complete.exp: Add "pck.ambiguous_func" to the relevant expected outputs. Add two testcases for completing ambiguous functions. * gdb.ada/complete/aux_pck.adb: New file. * gdb.ada/complete/aux_pck.ads: New file. * gdb.ada/complete/foo.adb: Pull Aux_Pck and call the two Ambiguous_Func functions. * gdb.ada/complete/pck.ads: Add an Ambiguous_Func function. * gdb.ada/complete/pck.adb: Likewise. Tested on x86_64-linux, no regression. --- gdb/ada-lang.c | 6 +++++- gdb/testsuite/gdb.ada/complete.exp | 13 +++++++++++++ gdb/testsuite/gdb.ada/complete/aux_pck.adb | 28 ++++++++++++++++++++++++++++ gdb/testsuite/gdb.ada/complete/aux_pck.ads | 21 +++++++++++++++++++++ gdb/testsuite/gdb.ada/complete/foo.adb | 6 +++++- gdb/testsuite/gdb.ada/complete/pck.adb | 5 +++++ gdb/testsuite/gdb.ada/complete/pck.ads | 2 ++ 7 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 gdb/testsuite/gdb.ada/complete/aux_pck.adb create mode 100644 gdb/testsuite/gdb.ada/complete/aux_pck.ads diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index 7e6b6dc..a7809ff 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -3655,9 +3655,13 @@ ada_resolve_function (struct block_symbol syms[], } } + /* If we got multiple matches, ask the user which one to use. Don't do this + interactive thing during completion, though, as the purpose of the + completion is providing a list of all possible matches. Prompting the + user to filter it down would be completely unexpected in this case. */ if (m == 0) return -1; - else if (m > 1) + else if (m > 1 && !parse_completion) { printf_filtered (_("Multiple matches for %s\n"), name); user_select_syms (syms, m, 1); diff --git a/gdb/testsuite/gdb.ada/complete.exp b/gdb/testsuite/gdb.ada/complete.exp index 9919bdf..0c4f03f 100644 --- a/gdb/testsuite/gdb.ada/complete.exp +++ b/gdb/testsuite/gdb.ada/complete.exp @@ -140,6 +140,7 @@ test_gdb_complete "external_ident" \ test_gdb_complete "pck" \ [multi_line "(p pck\\.ad\[sb\])?" \ "(p pck\\.ad\[sb\])?" \ + "p pck.ambiguous_func" \ "p pck.external_identical_one" \ "p pck.inner.inside_variable" \ "p pck.local_identical_one" \ @@ -151,6 +152,7 @@ test_gdb_complete "pck" \ test_gdb_complete "pck." \ [multi_line "(p pck\\.ad\[sb\])?" \ "(p pck\\.ad\[sb\])?" \ + "p pck.ambiguous_func" \ "p pck.external_identical_one" \ "p pck.inner.inside_variable" \ "p pck.local_identical_one" \ @@ -181,3 +183,14 @@ if { [readline_is_used] } { } } } + +# Usually, parsing a function name that is ambiguous yields a menu through +# which users can select a specific function. This should not happen during +# completion, though. +test_gdb_complete "ambig" \ + [multi_line "p ambiguous_func" \ + "p ambiguous_proc" ] +test_gdb_complete "ambiguous_f" \ + "p ambiguous_func" +test_gdb_complete "ambiguous_func" \ + "p ambiguous_func" diff --git a/gdb/testsuite/gdb.ada/complete/aux_pck.adb b/gdb/testsuite/gdb.ada/complete/aux_pck.adb new file mode 100644 index 0000000..b15912a --- /dev/null +++ b/gdb/testsuite/gdb.ada/complete/aux_pck.adb @@ -0,0 +1,28 @@ +-- Copyright 2015 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 . + +package body Aux_Pck is + + procedure Ambiguous_Func is + begin + null; + end Ambiguous_Func; + + procedure Ambiguous_Proc is + begin + null; + end Ambiguous_Proc; + +end Aux_Pck; diff --git a/gdb/testsuite/gdb.ada/complete/aux_pck.ads b/gdb/testsuite/gdb.ada/complete/aux_pck.ads new file mode 100644 index 0000000..f3476c6 --- /dev/null +++ b/gdb/testsuite/gdb.ada/complete/aux_pck.ads @@ -0,0 +1,21 @@ +-- Copyright 2015 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 . + +package Aux_Pck is + + procedure Ambiguous_Func; + procedure Ambiguous_Proc; + +end Aux_Pck; diff --git a/gdb/testsuite/gdb.ada/complete/foo.adb b/gdb/testsuite/gdb.ada/complete/foo.adb index 58d0ee3..98f1c1e 100644 --- a/gdb/testsuite/gdb.ada/complete/foo.adb +++ b/gdb/testsuite/gdb.ada/complete/foo.adb @@ -13,7 +13,8 @@ -- You should have received a copy of the GNU General Public License -- along with this program. If not, see . -with Pck; use Pck; +with Aux_Pck; use Aux_Pck; +with Pck; use Pck; procedure Foo is Some_Local_Variable : Integer := 1; @@ -21,5 +22,8 @@ procedure Foo is begin My_Global_Variable := Some_Local_Variable + 1; -- START Proc (External_Identical_Two); + Aux_Pck.Ambiguous_Func; + Aux_Pck.Ambiguous_Proc; + Pck.Ambiguous_Func; end Foo; diff --git a/gdb/testsuite/gdb.ada/complete/pck.adb b/gdb/testsuite/gdb.ada/complete/pck.adb index 51dd725..10680dc 100644 --- a/gdb/testsuite/gdb.ada/complete/pck.adb +++ b/gdb/testsuite/gdb.ada/complete/pck.adb @@ -21,4 +21,9 @@ package body Pck is Inner.Inside_Variable := Not_In_Scope + I; end Proc; + procedure Ambiguous_Func is + begin + null; + end Ambiguous_Func; + end Pck; diff --git a/gdb/testsuite/gdb.ada/complete/pck.ads b/gdb/testsuite/gdb.ada/complete/pck.ads index ab2c47b..e85f566 100644 --- a/gdb/testsuite/gdb.ada/complete/pck.ads +++ b/gdb/testsuite/gdb.ada/complete/pck.ads @@ -31,4 +31,6 @@ package Pck is procedure Proc (I : Integer); + procedure Ambiguous_Func; + end Pck; -- 2.5.1