diff mbox

fix some bugs in python completion code

Message ID 545D458E.1010405@yahoo.fr
State New
Headers show

Commit Message

'Timothy Arceri' via Patchwork Forward Nov. 7, 2014, 10:19 p.m. UTC 
The attached patch fixes several issues with auto-completion for gdb 
python plugins. I cc-ed sergiodj because he contributed most of the 
stuff this is touching.


First issue: sizeof/strlen confusion.

The first issue is a simple confusion between sizeof and strlen when 
converting the current word to Unicode. This causes the python plugin to 
only see those first bytes that fit into the size of a pointer and 
returns junks if the word happens to be shorter.

This wasn't picked up in the testsuite because we never use the `word` 
argument and always rely on `text`.


Second issue: incorrect value passed to python as word argument.

The second issue this patch addresses is harder to understand. The way 
auto-completion works requires to first determine the break-characters 
that will be used and only then run the actual completion code. Because 
a completer implemented in python can request that an existing gdb 
completer be used instead, we don't know for sure which characters 
should be considered breaking before running the actual completion. To 
handle this the python completer is called in the very beginning and its 
result cached for when the actual completion should be returned.

The issue is that the python completer is given the arguments received 
by the function that is supposed to set the breaking characters. At that 
point the `word` parameter isn't supposed to be used and is always the 
same as `text`. This is normal because it can't yet be known. Because of 
this the python completer sees the wrong value for `word`. Actually it 
always receives the same value as for `text` (basically the whole line).

To fix this we choose a set of default breaking characters that will be 
used id they are not overwritten which happens if another completer ends 
up being used. Since we set those default breaking characters we are 
able to anticipate and compute the word our-self so that we are able to 
pass it on to python.

This wasn't picked up in the testsuite because we never use the `word` 
argument and always rely on `text`.


Third issue: case-specific duplicate code not repeated enough times.

There is some code handling the specific cases of filename and location 
completion that changes the `text` pointer before calling those 
completers. It is repeated twice, in both cases when a direct call to a 
completer is about to be made. The problem is this isn't done in the 
case it is the python completer delegating to one of those. The proposed 
patch moves this code to the filename and location completers 
themselves. This removes the duplicate code and ensures this should work 
no matter how the completers are called.

This can be tested using testsuite/gdb.python/py-completion.py and the 
completefilemethod testcase. Completing the first argument as a file 
works fine but the second argument fails to auto-complete. This is 
because the filename completer received the whole line instead of just 
the last word in this case. With the patch this works fine.



Using python2.7 or python3.4 this patch doesn't break any of the tests 
in the testsuite/gdb.python;


ChangeLog:

2014-11-07  Wannes Rombouts  <wapiflapi@yahoo.fr>

	* gdb/completer.c (refactor): Move code preparing for
	completers to the completers themselves to ensure it is
	always called.
	* gdb/python/py-cmd.c (cmdpy_completer_helper): Fix bug
	caused by call to sizeof instead of strlen.
	(cmdpy_completer_handle_brkchars): Compute current word
	and pass it to python as specified in the documentation.



Please criticize if there are issues with the way I fixed this!

Wannes `wapiflapi` Rombouts

Ps: This is my first time contributing and I hope I did everything right!
diff mbox

Patch

diff --git a/gdb/completer.c b/gdb/completer.c
index a0f3fa3..95fb6d8 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -118,6 +118,23 @@  filename_completer (struct cmd_list_element *ignore,
   int subsequent_name;
   VEC (char_ptr) *return_val = NULL;
 
+  const char *start;
+
+  start = text;
+  /* Many commands which want to complete on
+     file names accept several file names, as
+     in "run foo bar >>baz".  So we don't want
+     to complete the entire text after the
+     command, just the last word.  To this
+     end, we need to find the beginning of the
+     file name by starting at `word' and going
+     backwards.  */
+  for (text = word;
+       text > start
+	 && strchr (gdb_completer_file_name_break_characters, text[-1]) == NULL;
+       text--)
+    ;
+
   subsequent_name = 0;
   while (1)
     {
@@ -197,6 +214,17 @@  location_completer (struct cmd_list_element *ignore,
   const char *orig_text = text;
   size_t text_len;
 
+  const char *start;
+
+  start = text;
+  /* Commands which complete on locations want to
+     see the entire argument.  */
+  for (text = word;
+       text > start
+	 && text[-1] != ' ' && text[-1] != '\t';
+       text--)
+    ;
+
   /* Do we have an unquoted colon, as in "break foo.c:bar"?  */
   for (p = text; *p != '\0'; ++p)
     {
@@ -659,42 +687,17 @@  complete_line_internal (const char *text,
 		  rl_completer_word_break_characters =
 		    gdb_completer_command_word_break_characters;
 		}
-	      else
+	      else if (reason == handle_brkchars)
 		{
-		  /* It is a normal command; what comes after it is
-		     completed by the command's completer function.  */
 		  if (c->completer == filename_completer)
-		    {
-		      /* Many commands which want to complete on
-			 file names accept several file names, as
-			 in "run foo bar >>baz".  So we don't want
-			 to complete the entire text after the
-			 command, just the last word.  To this
-			 end, we need to find the beginning of the
-			 file name by starting at `word' and going
-			 backwards.  */
-		      for (p = word;
-			   p > tmp_command
-			     && strchr (gdb_completer_file_name_break_characters, p[-1]) == NULL;
-			   p--)
-			;
-		      rl_completer_word_break_characters =
-			gdb_completer_file_name_break_characters;
-		    }
-		  else if (c->completer == location_completer)
-		    {
-		      /* Commands which complete on locations want to
-			 see the entire argument.  */
-		      for (p = word;
-			   p > tmp_command
-			     && p[-1] != ' ' && p[-1] != '\t';
-			   p--)
-			;
-		    }
-		  if (reason == handle_brkchars
-		      && c->completer_handle_brkchars != NULL)
+		    rl_completer_word_break_characters =
+		      gdb_completer_file_name_break_characters;
+		  if (c->completer_handle_brkchars != NULL)
 		    (*c->completer_handle_brkchars) (c, p, word);
-		  if (reason != handle_brkchars && c->completer != NULL)
+		}
+	      else if (reason == handle_completions)
+		{
+		  if (c->completer != NULL)
 		    list = (*c->completer) (c, p, word);
 		}
 	    }
@@ -743,34 +746,18 @@  complete_line_internal (const char *text,
 	      if (reason != handle_brkchars)
 		list = complete_on_enum (c->enums, p, word);
 	    }
-	  else
+
+	  else if (reason == handle_brkchars)
 	    {
-	      /* It is a normal command.  */
 	      if (c->completer == filename_completer)
-		{
-		  /* See the commentary above about the specifics
-		     of file-name completion.  */
-		  for (p = word;
-		       p > tmp_command
-			 && strchr (gdb_completer_file_name_break_characters, 
-				    p[-1]) == NULL;
-		       p--)
-		    ;
-		  rl_completer_word_break_characters =
-		    gdb_completer_file_name_break_characters;
-		}
-	      else if (c->completer == location_completer)
-		{
-		  for (p = word;
-		       p > tmp_command
-			 && p[-1] != ' ' && p[-1] != '\t';
-		       p--)
-		    ;
-		}
-	      if (reason == handle_brkchars
-		  && c->completer_handle_brkchars != NULL)
+		rl_completer_word_break_characters =
+		  gdb_completer_file_name_break_characters;
+	      if (c->completer_handle_brkchars != NULL)
 		(*c->completer_handle_brkchars) (c, p, word);
-	      if (reason != handle_brkchars && c->completer != NULL)
+	    }
+	  else if (reason == handle_completions)
+	    {
+	      if (c->completer != NULL)
 		list = (*c->completer) (c, p, word);
 	    }
 	}
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 5fc656e..6ebfa5b 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -28,6 +28,9 @@ 
 #include "completer.h"
 #include "language.h"
 
+/* Needed for rl_completer_word_break_characters */
+#include "readline/readline.h"
+
 /* Struct representing built-in completion types.  */
 struct cmdpy_completer
 {
@@ -269,7 +272,7 @@  cmdpy_completer_helper (struct cmd_list_element *command,
       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);
+      wordobj = PyUnicode_Decode (word, strlen (word), host_charset (), NULL);
       if (wordobj == NULL)
 	{
 	  Py_DECREF (textobj);
@@ -306,6 +309,28 @@  cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
 
   cleanup = ensure_python_env (get_current_arch (), current_language);
 
+  /* The following line sets the default break characters in case the
+     python complete method chooses to do the completion itself.
+     This is optional and can be left out to keep the language defaults.
+
+     TODO: We should let the user be able to choose and/or know this.
+
+     Because if a python completer chooses to do the completion instead
+     of relying on for example COMPLETE_EXPRESSION, chances are it is
+     doing something which expects more <shell> like arguments. We use
+     the same reduced set of breaking characters for this as for files.
+  */
+
+  set_gdb_completion_word_break_characters (filename_completer);
+
+  /* Because this is the first time we are called we received a dummy
+     value for word (we aren't supposed to need it). Since we use it
+     we need to compute its correct value ourselves. */
+  word = text + strlen (text);
+  while (word > text
+	 && strchr (rl_completer_word_break_characters, word[-1]) == NULL)
+    word--;
+
   /* Calling our helper to obtain the PyObject of the Python
      function.  */
   resultobj = cmdpy_completer_helper (command, text, word, 1);