From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17278 invoked by alias); 4 Apr 2014 20:41:53 -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 17265 invoked by uid 89); 4 Apr 2014 20:41:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS 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; Fri, 04 Apr 2014 20:41:50 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s34KfnPu013750 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 4 Apr 2014 16:41:49 -0400 Received: from psique (ovpn-113-33.phx2.redhat.com [10.3.113.33]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s34KfivA003993 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 4 Apr 2014 16:41:46 -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: Fri, 04 Apr 2014 20:41:00 -0000 In-Reply-To: (Sergio Durigan Junior's message of "Fri, 21 Mar 2014 23:54:35 -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-04/txt/msg00075.txt.bz2 On Friday, March 21 2014, I wrote: > On Wednesday, March 12 2014, I wrote: > >> Hi, > > Ping. Ping^2. >> 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 -- Sergio