From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20481 invoked by alias); 22 Mar 2014 02:54:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 20469 invoked by uid 89); 22 Mar 2014 02:54:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 22 Mar 2014 02:54:42 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2M2se6E026207 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 21 Mar 2014 22:54:40 -0400 Received: from psique (ovpn-113-73.phx2.redhat.com [10.3.113.73]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s2M2sa1d027161 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 21 Mar 2014 22:54:38 -0400 From: Sergio Durigan Junior To: GDB Patches Cc: Phil Muldoon Subject: Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class References: X-URL: http://www.redhat.com Date: Sat, 22 Mar 2014 02:54:00 -0000 In-Reply-To: (Sergio Durigan Junior's message of "Wed, 12 Mar 2014 19:49:39 -0300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2014-03/txt/msg00545.txt.bz2 On Wednesday, March 12 2014, I wrote: > Hi, Ping. > This PR came from a Red Hat bug that was filed recently. I checked and > it still exists on HEAD, so here's a proposed fix. Although this is > marked as a Python backend bug, this is really about the completion > mechanism used by GDB. Since this code reminds me of my first attempt > to make a good noodle, it took me quite some time to fix it in a > non-intrusive way. > > The problem is triggered when one registers a completion method inside a > class in a Python script, rather than registering the command using a > completer class directly. For example, consider the following script: > > class MyFirstCommand(gdb.Command): > def __init__(self): > gdb.Command.__init__(self,'myfirstcommand',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME) > > def invoke(self,argument,from_tty): > raise gdb.GdbError('not implemented') > > class MySecondCommand(gdb.Command): > def __init__(self): > gdb.Command.__init__(self,'mysecondcommand',gdb.COMMAND_USER) > > def invoke(self,argument,from_tty): > raise gdb.GdbError('not implemented') > > def complete(self,text,word): > return gdb.COMPLETE_FILENAME > > MyFirstCommand () > MySecondCommand () > > When one loads this into GDB and tries to complete filenames for both > myfirstcommand and mysecondcommand, she gets: > > (gdb) myfirstcommand /hom > (gdb) myfirstcommand /home/ > ^ > ... > (gdb) mysecondcommand /hom > (gdb) mysecondcommand /home > ^ > > (The "^" marks the final position of the cursor after the TAB). > > So we see that myfirstcommand honors the COMPLETE_FILENAME class (as > specified in the command creation), but mysecondcommand does not. After > some investigation, I found that the problem lies with the set of word > break characters that is used for each case. The set should be the same > for both commands, but it is not. > > During the process of deciding which type of completion should be used, > the code in gdb/completer.c:complete_line_internal analyses the command > that requested the completion and tries to determine the type of > completion wanted by checking which completion function will be called > (e.g., filename_completer for filenames, location_completer for > locations, etc.). > > This all works fine for myfirstcommand, because immediately after the > command registration the Python backend already sets its completion > function to filename_completer (which then causes the > complete_line_internal function to choose the right set of word break > chars). However, for mysecondcommand, this decision is postponed to > when the completer function is evaluated, and the Python backend uses an > internal completer (called cmdpy_completer). complete_line_internal > doesn't know about this internal completer, and can't choose the right > set of word break chars in time, which then leads to a bad decision when > completing the "/hom" word. > > So, after a few attempts, I decided to create another callback in > "struct cmd_list_element" that will be responsible for handling the case > when there is an unknown completer function for complete_line_internal > to work with. So far, only the Python backend uses this callback, and > only when the user provides a completer method instead of registering > the command directly with a completer class. I think this is the best > option because it not very intrusive (all the other commands will still > work normally), but especially because the whole completion code is so > messy that it would be hard to fix this without having to redesign > things. > > I have regtested this on Fedora 18 x86_64, without regressions. I also > included a testcase. > > OK to apply? > > Thanks, > > -- > Sergio > > gdb/ > 2014-03-12 Sergio Durigan Junior > > PR python/16699 > * cli/cli-decode.c (set_cmd_completer_handle_brkchars): New > function. > (add_cmd): Set "completer_handle_brkchars" to NULL. > * cli/cli-decode.h (struct cmd_list_element) > : New field. > * command.h (set_cmd_completer_handle_brkchars): New prototype. > * completer.c (set_gdb_completion_word_break_characters): New > function. > (complete_line_internal): Call "completer_handle_brkchars" > callback from command. > * completer.h: Include "command.h". > (set_gdb_completion_word_break_characters): New prototype. > * python/py-cmd.c (cmdpy_completer_handle_brkchars): New function. > (cmdpy_init): Set completer_handle_brkchars to > cmdpy_completer_handle_brkchars. > > gdb/testsuite/ > 2014-03-12 Sergio Durigan Junior > > PR python/16699 > * gdb.python/py-completion.exp: New file. > * gdb.python/py-completion.py: Likewise. > > diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c > index d36ab4e..0c4d996 100644 > --- a/gdb/cli/cli-decode.c > +++ b/gdb/cli/cli-decode.c > @@ -164,6 +164,15 @@ set_cmd_completer (struct cmd_list_element *cmd, completer_ftype *completer) > cmd->completer = completer; /* Ok. */ > } > > +/* See definition in commands.h. */ > + > +void > +set_cmd_completer_handle_brkchars (struct cmd_list_element *cmd, > + void (*completer_handle_brkchars) (struct cmd_list_element *self)) > +{ > + cmd->completer_handle_brkchars = completer_handle_brkchars; /* Ok. */ > +} > + > /* Add element named NAME. > Space for NAME and DOC must be allocated by the caller. > CLASS is the top level category into which commands are broken down > @@ -239,6 +248,7 @@ add_cmd (const char *name, enum command_class class, void (*fun) (char *, int), > c->prefix = NULL; > c->abbrev_flag = 0; > set_cmd_completer (c, make_symbol_completion_list_fn); > + c->completer_handle_brkchars = NULL; > c->destroyer = NULL; > c->type = not_set_cmd; > c->var = NULL; > diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h > index c6edc87..a043734 100644 > --- a/gdb/cli/cli-decode.h > +++ b/gdb/cli/cli-decode.h > @@ -176,6 +176,15 @@ struct cmd_list_element > "baz/foo", return "baz/foobar". */ > completer_ftype *completer; > > + /* Handle the word break characters for this completer. Usually > + this function need not be defined, but for some types of > + completers (e.g., Python completers declared as methods inside > + a class) the word break chars may need to be redefined > + depending on the completer type (e.g., for filename > + completers). */ > + > + void (*completer_handle_brkchars) (struct cmd_list_element *self); > + > /* Destruction routine for this command. If non-NULL, this is > called when this command instance is destroyed. This may be > used to finalize the CONTEXT field, if needed. */ > diff --git a/gdb/command.h b/gdb/command.h > index a5040a4..058d133 100644 > --- a/gdb/command.h > +++ b/gdb/command.h > @@ -160,6 +160,12 @@ typedef VEC (char_ptr) *completer_ftype (struct cmd_list_element *, > > extern void set_cmd_completer (struct cmd_list_element *, completer_ftype *); > > +/* Set the completer_handle_brkchars callback. */ > + > +extern void set_cmd_completer_handle_brkchars (struct cmd_list_element *, > + void (*f) > + (struct cmd_list_element *)); > + > /* HACK: cagney/2002-02-23: Code, mostly in tracepoints.c, grubs > around in cmd objects to test the value of the commands sfunc(). */ > extern int cmd_cfunc_eq (struct cmd_list_element *cmd, > diff --git a/gdb/completer.c b/gdb/completer.c > index 94f70a9..a9809a2 100644 > --- a/gdb/completer.c > +++ b/gdb/completer.c > @@ -450,6 +450,21 @@ expression_completer (struct cmd_list_element *ignore, > return location_completer (ignore, p, word); > } > > +/* See definition in completer.h. */ > + > +void > +set_gdb_completion_word_break_characters (completer_ftype *fn) > +{ > + /* So far we are only interested in differentiating filename > + completers from everything else. */ > + if (fn == filename_completer) > + rl_completer_word_break_characters > + = gdb_completer_file_name_break_characters; > + else > + rl_completer_word_break_characters > + = gdb_completer_command_word_break_characters; > +} > + > /* Here are some useful test cases for completion. FIXME: These > should be put in the test suite. They should be tested with both > M-? and TAB. > @@ -481,7 +496,6 @@ typedef enum > } > complete_line_internal_reason; > > - > /* Internal function used to handle completions. > > > @@ -678,6 +692,9 @@ complete_line_internal (const char *text, > p--) > ; > } > + if (reason == handle_brkchars > + && c->completer_handle_brkchars != NULL) > + (*c->completer_handle_brkchars) (c); > if (reason != handle_brkchars && c->completer != NULL) > list = (*c->completer) (c, p, word); > } > @@ -751,6 +768,9 @@ complete_line_internal (const char *text, > p--) > ; > } > + if (reason == handle_brkchars > + && c->completer_handle_brkchars != NULL) > + (*c->completer_handle_brkchars) (c); > if (reason != handle_brkchars && c->completer != NULL) > list = (*c->completer) (c, p, word); > } > diff --git a/gdb/completer.h b/gdb/completer.h > index 5b90773..cb9c389 100644 > --- a/gdb/completer.h > +++ b/gdb/completer.h > @@ -18,6 +18,7 @@ > #define COMPLETER_H 1 > > #include "gdb_vecs.h" > +#include "command.h" > > extern VEC (char_ptr) *complete_line (const char *text, > char *line_buffer, > @@ -48,6 +49,13 @@ extern char *get_gdb_completer_quote_characters (void); > > extern char *gdb_completion_word_break_characters (void); > > +/* Set the word break characters array to the corresponding set of > + chars, based on FN. This function is useful for cases when the > + completer doesn't know the type of the completion until some > + calculation is done (e.g., for Python functions). */ > + > +extern void set_gdb_completion_word_break_characters (completer_ftype *fn); > + > /* Exported to linespec.c */ > > extern const char *skip_quoted_chars (const char *, const char *, > diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c > index c24bca7..df0aeed 100644 > --- a/gdb/python/py-cmd.c > +++ b/gdb/python/py-cmd.c > @@ -206,6 +206,79 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty) > do_cleanups (cleanup); > } > > +/* Python function called to determine the break characters of a > + certain completer. We use dummy variables for the "text" and > + "word" arguments for the completer, and call it. We're actually > + only interested in knowing if the completer registered by the user > + will return one of the integer codes (see COMPLETER_* symbols). */ > + > +static void > +cmdpy_completer_handle_brkchars (struct cmd_list_element *command) > +{ > + cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command); > + PyObject *textobj, *wordobj, *resultobj = NULL; > + const char dummy_text[] = "dummytext"; > + const char dummy_word[] = "dummyword"; > + struct cleanup *cleanup; > + > + cleanup = ensure_python_env (get_current_arch (), current_language); > + > + if (!obj) > + error (_("Invalid invocation of Python command object.")); > + if (!PyObject_HasAttr ((PyObject *) obj, complete_cst)) > + { > + /* If there is no complete method, don't error. */ > + goto done; > + } > + > + textobj = PyUnicode_Decode (dummy_text, sizeof (dummy_text), > + host_charset (), NULL); > + if (!textobj) > + error (_("Could not convert argument to Python string.")); > + wordobj = PyUnicode_Decode (dummy_word, sizeof (dummy_word), > + host_charset (), NULL); > + if (!wordobj) > + error (_("Could not convert argument to Python string.")); > + > + resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst, > + textobj, wordobj, NULL); > + Py_DECREF (textobj); > + Py_DECREF (wordobj); > + if (!resultobj) > + { > + /* Just swallow errors here. */ > + PyErr_Clear (); > + goto done; > + } > + > + if (PyInt_Check (resultobj)) > + { > + /* User code may also return one of the completion constants, > + thus requesting that sort of completion. We are only > + interested in this kind of return. */ > + long value; > + > + if (!gdb_py_int_as_long (resultobj, &value)) > + { > + /* Ignore. */ > + PyErr_Clear (); > + } > + else if (value >= 0 && value < (long) N_COMPLETERS) > + { > + /* This is the core of this function. Depending on which > + completer type the Python function returns, we have to > + adjust the break characters accordingly. */ > + set_gdb_completion_word_break_characters > + (completers[value].completer); > + } > + } > + > + done: > + > + Py_XDECREF (resultobj); > + do_cleanups (cleanup); > +} > + > /* Called by gdb for command completion. */ > > static VEC (char_ptr) * > @@ -546,6 +619,9 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw) > set_cmd_context (cmd, self); > set_cmd_completer (cmd, ((completetype == -1) ? cmdpy_completer > : completers[completetype].completer)); > + if (completetype == -1) > + set_cmd_completer_handle_brkchars (cmd, > + cmdpy_completer_handle_brkchars); > } > if (except.reason < 0) > { > diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp > new file mode 100644 > index 0000000..9f8a5b2 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-completion.exp > @@ -0,0 +1,49 @@ > +# Copyright (C) 2014 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 . > + > +set testfile "py-completion" > + > +load_lib gdb-python.exp > + > +gdb_exit > +gdb_start > + > +# Skip all tests if Python scripting is not enabled. > +if { [skip_python_tests] } { continue } > + > +gdb_test_no_output "source ${srcdir}/${subdir}/${testfile}.py" > + > +# This one should always pass. > +send_gdb "myfirstcommand /hom\t" > +gdb_test_multiple "" "myfirstcommand completion" { > + -re "^myfirstcommand /home/$" { > + pass "myfirstcommand completion" > + } > +} > + > +# Just discarding whatever we typed. > +send_gdb "\n" > +gdb_test ".*" > + > +# This is the problematic one. > +send_gdb "mysecondcommand /hom\t" > +gdb_test_multiple "" "mysecondcommand completion" { > + -re "^mysecondcommand /home $" { > + fail "mysecondcommand completion (did not correctly complete filename)" > + } > + -re "^mysecondcommand /home/$" { > + pass "mysecondcommand completion" > + } > +} > diff --git a/gdb/testsuite/gdb.python/py-completion.py b/gdb/testsuite/gdb.python/py-completion.py > new file mode 100644 > index 0000000..bf7f77b > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-completion.py > @@ -0,0 +1,38 @@ > +# Copyright (C) 2014 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 . > + > +# This testcase tests PR python/16699 > + > +import gdb > + > +class MyFirstCommand(gdb.Command): > + def __init__(self): > + gdb.Command.__init__(self,'myfirstcommand',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME) > + > + def invoke(self,argument,from_tty): > + raise gdb.GdbError('not implemented') > + > +class MySecondCommand(gdb.Command): > + def __init__(self): > + gdb.Command.__init__(self,'mysecondcommand',gdb.COMMAND_USER) > + > + def invoke(self,argument,from_tty): > + raise gdb.GdbError('not implemented') > + > + def complete(self,text,word): > + return gdb.COMPLETE_FILENAME > + > +MyFirstCommand() > +MySecondCommand() -- Sergio