From patchwork Wed Sep 3 20:36:40 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 2646 Received: (qmail 1581 invoked by alias); 3 Sep 2014 20:36:48 -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 1567 invoked by uid 89); 3 Sep 2014 20:36:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, UNSUBSCRIBE_BODY autolearn=no 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 03 Sep 2014 20:36:44 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s83KafZK024365 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 3 Sep 2014 16:36:41 -0400 Received: from localhost (dhcp-10-15-16-169.yyz.redhat.com [10.15.16.169]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s83Kae73016686 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Wed, 3 Sep 2014 16:36:41 -0400 From: Sergio Durigan Junior To: Doug Evans Cc: Jan Kratochvil , gdb-patches@sourceware.org Subject: Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class References: <87a9btqhe3.fsf@fleche.redhat.com> <87y4xwqn71.fsf@fleche.redhat.com> <20140708153221.GA15767@host2.jankratochvil.net> <87iolow0ad.fsf@redhat.com> <87a96ikmqf.fsf@redhat.com> X-URL: http://www.redhat.com Date: Wed, 03 Sep 2014 16:36:40 -0400 Message-ID: <87egvsfnl3.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes On Wednesday, September 03 2014, Doug Evans wrote: > Hi. Hi. > I've spent some more time looking at the patch. Thanks. > I have one style nit and one request. > > Style nit: I see lots of places that do "if (!ptr)". > Convention is to write this as "if (ptr == NULL)". I know. However, this code already existed; I am only moving parts of it. I was in doubt if it would be good to rewrite those bad style checks, but decided to keep them. Anyway, I will rewrite before I push. > Request: > The "why" of things is explained sufficiently well in your > original email: > https://sourceware.org/ml/gdb-patches/2014-03/msg00301.html > And while I can appreciate this text being added to the commit message, > I don't want to have to read commit logs to have a basic understanding > of the "why" of the code. [I'm all for more deeper understanding > being deferred to commit logs as appropriate, but I should be able > to get a basic understanding from the code itself.] > > Can you add something descriptive to the code? Fair enough. > For example, how about something like this? > > --- python/py-cmd.c= 2014-09-02 20:28:57.838267408 -0700 > +++ python/py-cmd.c 2014-09-02 20:59:40.026083464 -0700 > @@ -219,6 +219,14 @@ cmdpy_function (struct cmd_list_element > returns that variable, without actually calling the Python method > again. This saves us one Python method call. > > + The reason for this two step dance is that we need to know the set > + of "brkchars" to use early on, before we actually try to perform the > + completion. But if a Python command supplies a "complete" method then > + we have to call that method first: it may return as its result the kind > + of completion to perform and that will in turn specify which brkchars > + to use. IOW, we need the result of the "complete" method before we > + actually perform the completion. > + > It is important to mention that this function is built on the > assumption that the calls to cmdpy_completer_handle_brkchars and > cmdpy_completer will be subsequent with nothing intervening. This Yeah, looks good, I will add this. > I have no more comments so LGTM with those changes. Thanks, this is what I pushed. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 80961d6..d416623 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,25 @@ +2014-09-03 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 (completer_ftype_void): New typedef. + (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_helper): New function. + (cmdpy_completer_handle_brkchars): New function. + (cmdpy_completer): Adjust to use cmdpy_completer_helper. + (cmdpy_init): Set completer_handle_brkchars to + cmdpy_completer_handle_brkchars. + 2014-09-03 Gary Benson * nat/x86-dregs.h (ALL_DEBUG_REGISTERS): Renamed as... diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c index c409d9c..7bc51a1 100644 --- a/gdb/cli/cli-decode.c +++ b/gdb/cli/cli-decode.c @@ -161,6 +161,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, + completer_ftype_void *completer_handle_brkchars) +{ + cmd->completer_handle_brkchars = completer_handle_brkchars; +} + /* 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 @@ -236,6 +245,7 @@ add_cmd (const char *name, enum command_class class, cmd_cfunc_ftype *fun, 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 865d4a0..5920559 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). */ + + completer_ftype_void *completer_handle_brkchars; + /* 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 4ac3bfb..3b96212 100644 --- a/gdb/command.h +++ b/gdb/command.h @@ -159,8 +159,16 @@ extern void set_cmd_sfunc (struct cmd_list_element *cmd, typedef VEC (char_ptr) *completer_ftype (struct cmd_list_element *, const char *, const char *); +typedef void completer_ftype_void (struct cmd_list_element *, + const char *, const char *); + 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 *, + completer_ftype_void *); + /* 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 44920dd..67d7f45 100644 --- a/gdb/completer.c +++ b/gdb/completer.c @@ -449,6 +449,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. @@ -677,6 +692,9 @@ complete_line_internal (const char *text, p--) ; } + if (reason == handle_brkchars + && c->completer_handle_brkchars != NULL) + (*c->completer_handle_brkchars) (c, p, word); if (reason != handle_brkchars && c->completer != NULL) list = (*c->completer) (c, p, word); } @@ -750,6 +768,9 @@ complete_line_internal (const char *text, p--) ; } + if (reason == handle_brkchars + && c->completer_handle_brkchars != NULL) + (*c->completer_handle_brkchars) (c, p, word); if (reason != handle_brkchars && c->completer != NULL) list = (*c->completer) (c, p, word); } diff --git a/gdb/completer.h b/gdb/completer.h index 7aa0f3b..bc7ed96 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, const 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 21f2a20..8bc4bf7 100644 --- a/gdb/python/py-cmd.c +++ b/gdb/python/py-cmd.c @@ -208,45 +208,163 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty) do_cleanups (cleanup); } +/* Helper function for the Python command completers (both "pure" + completer and brkchar handler). This function takes COMMAND, TEXT + and WORD and tries to call the Python method for completion with + these arguments. It also takes HANDLE_BRKCHARS_P, an argument to + identify whether it is being called from the brkchar handler or + from the "pure" completer. In the first case, it effectively calls + the Python method for completion, and records the PyObject in a + static variable (used as a "cache"). In the second case, it just + returns that variable, without actually calling the Python method + again. This saves us one Python method call. + + The reason for this two step dance is that we need to know the set + of "brkchars" to use early on, before we actually try to perform + the completion. But if a Python command supplies a "complete" + method then we have to call that method first: it may return as its + result the kind of completion to perform and that will in turn + specify which brkchars to use. IOW, we need the result of the + "complete" method before we actually perform the completion. + + It is important to mention that this function is built on the + assumption that the calls to cmdpy_completer_handle_brkchars and + cmdpy_completer will be subsequent with nothing intervening. This + is true for our completer mechanism. + + This function returns the PyObject representing the Python method + call. */ + +static PyObject * +cmdpy_completer_helper (struct cmd_list_element *command, + const char *text, const char *word, + int handle_brkchars_p) +{ + cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command); + PyObject *textobj, *wordobj; + /* This static variable will server as a "cache" for us, in order to + store the PyObject that results from calling the Python + function. */ + static PyObject *resultobj = NULL; + + if (handle_brkchars_p) + { + /* If we were called to handle brkchars, it means this is the + first function call of two that will happen in a row. + Therefore, we need to call the completer ourselves, and cache + the return value in the static variable RESULTOBJ. Then, in + the second call, we can just use the value of RESULTOBJ to do + our job. */ + if (resultobj != NULL) + Py_DECREF (resultobj); + + resultobj = NULL; + if (obj == NULL) + error (_("Invalid invocation of Python command object.")); + if (!PyObject_HasAttr ((PyObject *) obj, complete_cst)) + { + /* If there is no complete method, don't error. */ + return NULL; + } + + textobj = PyUnicode_Decode (text, strlen (text), host_charset (), NULL); + if (textobj == NULL) + error (_("Could not convert argument to Python string.")); + wordobj = PyUnicode_Decode (word, sizeof (word), host_charset (), NULL); + if (wordobj == NULL) + { + Py_DECREF (textobj); + 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 (); + } + + Py_XINCREF (resultobj); + } + + return resultobj; +} + +/* Python function called to determine the break characters of a + certain completer. We are 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, + const char *text, const char *word) +{ + PyObject *resultobj = NULL; + struct cleanup *cleanup; + + cleanup = ensure_python_env (get_current_arch (), current_language); + + /* Calling our helper to obtain the PyObject of the Python + function. */ + resultobj = cmdpy_completer_helper (command, text, word, 1); + + /* Check if there was an error. */ + if (resultobj == NULL) + 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: + + /* We do not call Py_XDECREF here because RESULTOBJ will be used in + the subsequent call to cmdpy_completer function. */ + do_cleanups (cleanup); +} + /* Called by gdb for command completion. */ static VEC (char_ptr) * cmdpy_completer (struct cmd_list_element *command, const char *text, const char *word) { - cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command); - PyObject *textobj, *wordobj, *resultobj = NULL; + PyObject *resultobj = NULL; VEC (char_ptr) *result = NULL; 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 -- instead, just - say that there are no completions. */ - goto done; - } + /* Calling our helper to obtain the PyObject of the Python + function. */ + resultobj = cmdpy_completer_helper (command, text, word, 0); - textobj = PyUnicode_Decode (text, strlen (text), host_charset (), NULL); - if (! textobj) - error (_("Could not convert argument to Python string.")); - wordobj = PyUnicode_Decode (word, strlen (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 the result object of calling the Python function is NULL, it + means that there was an error. In this case, just give up and + return NULL. */ + if (resultobj == NULL) + goto done; result = NULL; if (PyInt_Check (resultobj)) @@ -302,7 +420,6 @@ cmdpy_completer (struct cmd_list_element *command, done: - Py_XDECREF (resultobj); do_cleanups (cleanup); return result; @@ -548,6 +665,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/ChangeLog b/gdb/testsuite/ChangeLog index d996b7c..d6db24b 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,9 @@ +2014-09-03 Sergio Durigan Junior + + PR python/16699 + * gdb.python/py-completion.exp: New file. + * gdb.python/py-completion.py: Likewise. + 2014-08-28 Doug Evans * gdb.arch/amd64-pseudo.c (main): Rewrite to better specify when diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp new file mode 100644 index 0000000..a3bac8b --- /dev/null +++ b/gdb/testsuite/gdb.python/py-completion.exp @@ -0,0 +1,69 @@ +# 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" + +# Create a temporary directory +set testdir "[standard_output_file "py-completion-testdir"]/" +set testdir_regex [string_to_regexp $testdir] +set testdir_complete [standard_output_file "py-completion-test"] +file mkdir $testdir + +# This one should always pass. +send_gdb "completefileinit ${testdir_complete}\t" +gdb_test_multiple "" "completefileinit completion" { + -re "^completefileinit ${testdir_regex}$" { + pass "completefileinit completion" + } +} + +# Just discarding whatever we typed. +gdb_test " " ".*" "discard #1" + +# This is the problematic one. +send_gdb "completefilemethod ${testdir_complete}\t" +gdb_test_multiple "" "completefilemethod completion" { + -re "^completefilemethod ${testdir_regex} $" { + fail "completefilemethod completion (completed filename as wrong command arg)" + } + -re "^completefilemethod ${testdir_regex}$" { + pass "completefilemethod completion" + } +} + +# Discarding again +gdb_test " " ".*" "discard #2" + +# Another problematic +set completion_regex "[string_to_regexp [standard_output_file "py-completion-t"]]" +send_gdb "completefilecommandcond [standard_output_file "py-completion-t\t"]" +gdb_test_multiple "" "completefilecommandcond completion" { + -re "^completefilecommandcond ${testdir}$" { + fail "completefilecommandcond completion (completed filename instead of command)" + } + -re "^completefilecommandcond ${completion_regex}\007$" { + pass "completefilecommandcond 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..23592d0 --- /dev/null +++ b/gdb/testsuite/gdb.python/py-completion.py @@ -0,0 +1,58 @@ +# 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 CompleteFileInit(gdb.Command): + def __init__(self): + gdb.Command.__init__(self,'completefileinit',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME) + + def invoke(self,argument,from_tty): + raise gdb.GdbError('not implemented') + +class CompleteFileMethod(gdb.Command): + def __init__(self): + gdb.Command.__init__(self,'completefilemethod',gdb.COMMAND_USER) + + def invoke(self,argument,from_tty): + raise gdb.GdbError('not implemented') + + def complete(self,text,word): + return gdb.COMPLETE_FILENAME + +class CompleteFileCommandCond(gdb.Command): + def __init__(self): + gdb.Command.__init__(self,'completefilecommandcond',gdb.COMMAND_USER) + + def invoke(self,argument,from_tty): + raise gdb.GdbError('not implemented') + + def complete(self,text,word): + # This is a test made to know if the command + # completion still works fine. When the user asks to + # complete something like "completefilecommandcond + # /path/to/py-completion-t", it should not complete to + # "/path/to/py-completion-test/", but instead just + # wait for input. + if "py-completion-t" in text: + return gdb.COMPLETE_COMMAND + else: + return gdb.COMPLETE_FILENAME + +CompleteFileInit() +CompleteFileMethod() +CompleteFileCommandCond()