This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] fix some bugs in python completion code


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 --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);

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]