[PATCH v3 02/19] Remove completion_tracker_t from the public completion API.

[Keith Seitz <keiths at redhat dot com>]

Differences from last version:

1. Removed comments about recording an additional completion from
   add_completion.
2. Removed requirement to xstrdup arg to add_completion; add_completeion
    uses new function completer_strdup.
3. Consolidated (oft-repeated) partial copy code into completer_strdup.
   Removed this code from all completion functions.

---

This patch removes the recently introduced "completion tracker" concept
from the public completion API.  In the previous patch, the completion
tracker was added to the (new) completer_data structure that is now
used by all completion functions.

Since completion_tracker_t is now used entirely inside completer.c, there
is no need to make its type opaque, so the typedef has been removed.

This patch also fixes gdb/17960, which is easily demonstrated:

$ gdb -nx -q gdb
(gdb) b gdb.c:ma<TAB>
./../src/gdb/completer.c:837: internal-error: maybe_add_completion:
Assertion `tracker != NULL' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

This occurs because the code path in this case is:
complete_line
- complete_line_internal
-- location_completer

gdb/ChangeLog

	PR gdb/17960
	* completer.c (struct completer_data) <tracker>: Change type to
	htab_t.
	(enum maybe_add_completion_enum): Make private; moved from
	completer.h.
	[DEFAULT_MAX_COMPLETIONS]: Define.
	(new_completion_tracker): Remove function.
	(new_completer_data): New function.
	(free_completion_tracker): Remove function.
	(free_completer_data): New function.
	(make_cleanup_free_completion_tracker): Remove function.
	(maybe_add_completion): Make static.  Update comments.
	Use completer_data instead of completion_tracker_t.
	(completer_strdup): New function.
	(add_completion): New function.
	(complete_line, gdb_completion_word_break_characters): Use
	completer_data instead of completion_tracker.
	* completer.h (completion_tracker_t, new_completion_tracker)
	(make_cleanup_free_completion_tracker, maybe_add_completion)
	(enum maybe_add_completion): Remove declarations.
	(enum add_completion_status): Define.
	(add_completion): Declare.
	* symtab.c (completion_tracker): Remove variable.
	(completion_list_add_name): Use add_completion instead of
	maybe_add_completion and partial copy.
	(default_make_symbol_completion_list_break_on_1): Do not use
	completion_tracker.

gdb/testsuite/ChangeLog

	PR gdb/17960
	* gdb.base/completion.exp: Add some basic tests for the
	location completer, including a regression test for
	the gdb/17960 assertion failure.
---
 gdb/completer.c                       |  231 +++++++++++++++++++++++----------
 gdb/completer.h                       |   66 +++------
 gdb/symtab.c                          |   62 +--------
 gdb/testsuite/gdb.base/completion.exp |   78 +++++++++++
 4 files changed, 260 insertions(+), 177 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 476f9f2..bf4137e 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -92,10 +92,31 @@ static char *gdb_completer_quote_characters = "'";
 
 struct completer_data
 {
-  /* The completion tracker being used by the completer.  */
-  completion_tracker_t tracker;
+  /* A hashtable used to track completions.  */
+  htab_t tracker;
 };
 
+/* Return values for maybe_add_completion.  */
+
+enum maybe_add_completion_enum
+{
+  /* NAME has been recorded and max_completions has not been reached,
+     or completion tracking is disabled (max_completions < 0).  */
+  MAYBE_ADD_COMPLETION_OK,
+
+  /* NAME has been recorded and max_completions has been reached
+     (thus the caller can stop searching).  */
+  MAYBE_ADD_COMPLETION_OK_MAX_REACHED,
+
+  /* max-completions entries has been reached.
+     Whether NAME is a duplicate or not is not determined.  */
+  MAYBE_ADD_COMPLETION_MAX_REACHED,
+
+  /* NAME has already been recorded.
+     Note that this is never returned if completion tracking is disabled
+     (max_completions < 0).  */
+  MAYBE_ADD_COMPLETION_DUPLICATE
+};
 
 /* Accessor for some completer data that may interest other files.  */
 
@@ -803,48 +824,41 @@ complete_line_internal (struct completer_data *cdata,
 
 /* See completer.h.  */
 
-int max_completions = 200;
+#define DEFAULT_MAX_COMPLETIONS 200
+int max_completions = DEFAULT_MAX_COMPLETIONS;
 
-/* See completer.h.  */
+/* Allocate a new completer data structure.  */
 
-completion_tracker_t
-new_completion_tracker (void)
+static struct completer_data *
+new_completer_data (int size)
 {
-  if (max_completions <= 0)
-    return NULL;
+  struct completer_data *cdata = XCNEW (struct completer_data);
 
-  return htab_create_alloc (max_completions,
-			    htab_hash_string, (htab_eq) streq,
-			    NULL, xcalloc, xfree);
+  cdata->tracker
+    = htab_create_alloc ((size < 0 ? DEFAULT_MAX_COMPLETIONS : size),
+			 htab_hash_string, (htab_eq) streq, NULL,
+			 xcalloc, xfree);
+  return cdata;
 }
 
-/* Cleanup routine to free a completion tracker and reset the pointer
-   to NULL.  */
+/* Free the completion data represented by P.  */
 
 static void
-free_completion_tracker (void *p)
+free_completer_data (void *p)
 {
-  completion_tracker_t *tracker_ptr = p;
+  struct completer_data *cdata = p;
 
-  htab_delete (*tracker_ptr);
-  *tracker_ptr = NULL;
+  htab_delete (cdata->tracker);
+  xfree (cdata);
 }
 
-/* See completer.h.  */
-
-struct cleanup *
-make_cleanup_free_completion_tracker (completion_tracker_t *tracker_ptr)
-{
-  if (*tracker_ptr == NULL)
-    return make_cleanup (null_cleanup, NULL);
-
-  return make_cleanup (free_completion_tracker, tracker_ptr);
-}
-
-/* See completer.h.  */
+/* Add the completion NAME to the list of generated completions if
+   it is not there already.
+   If max_completions is negative, nothing is done, not even watching
+   for duplicates, and MAYBE_ADD_COMPLETION_OK is always returned.  */
 
-enum maybe_add_completion_enum
-maybe_add_completion (completion_tracker_t tracker, char *name)
+static enum maybe_add_completion_enum
+maybe_add_completion (struct completer_data *cdata, char *name)
 {
   void **slot;
 
@@ -853,23 +867,82 @@ maybe_add_completion (completion_tracker_t tracker, char *name)
   if (max_completions == 0)
     return MAYBE_ADD_COMPLETION_MAX_REACHED;
 
-  gdb_assert (tracker != NULL);
-
-  if (htab_elements (tracker) >= max_completions)
+  if (htab_elements (cdata->tracker) >= max_completions)
     return MAYBE_ADD_COMPLETION_MAX_REACHED;
 
-  slot = htab_find_slot (tracker, name, INSERT);
+  slot = htab_find_slot (cdata->tracker, name, INSERT);
 
   if (*slot != HTAB_EMPTY_ENTRY)
     return MAYBE_ADD_COMPLETION_DUPLICATE;
 
   *slot = name;
 
-  return (htab_elements (tracker) < max_completions
+  return (htab_elements (cdata->tracker) < max_completions
 	  ? MAYBE_ADD_COMPLETION_OK
 	  : MAYBE_ADD_COMPLETION_OK_MAX_REACHED);
 }
 
+/* A strdup-like function used by the completer to copy bits of completion
+   strings.  */
+
+static char *
+completer_strdup (const char *match, const char *text, const char *word)
+{
+  char *alloc;
+
+  if (text == NULL || word == NULL || text == word)
+    return xstrdup (match);
+
+  if (word > text)
+    {
+      gdb_assert (strlen (match) > (word - text));
+
+      /* Return some portion of match.  */
+      match += (word - text);
+      alloc = xmalloc (strlen (match) + 1);
+      strcpy (alloc, match);
+    }
+  else
+    {
+      /* Return some of text plus match.  */
+      alloc = xmalloc (strlen (match) + (text - word) + 1);
+      strncpy (alloc, word, text - word);
+      alloc[text - word] = '\0';
+      strcat (alloc, match);
+    }
+
+  return alloc;
+}
+
+/* See completer.h.  */
+
+enum add_completion_status
+add_completion (struct completer_data *cdata, VEC (char_ptr) **result,
+		const char *match, const char *text, const char *word)
+{
+  enum maybe_add_completion_enum add_status;
+  char *alloc = completer_strdup (match, text, word);
+
+  add_status = maybe_add_completion (cdata, alloc);
+  switch (add_status)
+    {
+    case MAYBE_ADD_COMPLETION_OK:
+      VEC_safe_push (char_ptr, *result, alloc);
+      break;
+    case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
+      VEC_safe_push (char_ptr, *result, alloc);
+      return ADD_COMPLETION_MAX_REACHED;
+    case MAYBE_ADD_COMPLETION_MAX_REACHED:
+      xfree (alloc);
+      return ADD_COMPLETION_MAX_REACHED;
+    case MAYBE_ADD_COMPLETION_DUPLICATE:
+      xfree (alloc);
+      break;
+    }
+
+  return ADD_COMPLETION_OK;
+}
+
 void
 throw_max_completions_reached_error (void)
 {
@@ -894,54 +967,63 @@ complete_line (const char *text, const char *line_buffer, int point)
 {
   VEC (char_ptr) *list;
   VEC (char_ptr) *result = NULL;
-  struct cleanup *cleanups;
-  completion_tracker_t tracker;
+  struct cleanup *cdata_cleanup, *list_cleanup;
   char *candidate;
-  int ix, max_reached;
+  int ix;
+  struct completer_data *cdata;
 
   if (max_completions == 0)
     return NULL;
-  list = complete_line_internal (NULL, text, line_buffer, point,
+
+  cdata = new_completer_data (max_completions);
+  cdata_cleanup = make_cleanup (free_completer_data, cdata);
+  list = complete_line_internal (cdata, text, line_buffer, point,
 				 handle_completions);
   if (max_completions < 0)
-    return list;
-
-  tracker = new_completion_tracker ();
-  cleanups = make_cleanup_free_completion_tracker (&tracker);
-  make_cleanup_free_char_ptr_vec (list);
-
-  /* Do a final test for too many completions.  Individual completers may
-     do some of this, but are not required to.  Duplicates are also removed
-     here.  Otherwise the user is left scratching his/her head: readline and
-     complete_command will remove duplicates, and if removal of duplicates
-     there brings the total under max_completions the user may think gdb quit
-     searching too early.  */
-
-  for (ix = 0, max_reached = 0;
-       !max_reached && VEC_iterate (char_ptr, list, ix, candidate);
-       ++ix)
     {
-      enum maybe_add_completion_enum add_status;
+      do_cleanups (cdata_cleanup);
+      return list;
+    }
+
+  list_cleanup = make_cleanup_free_char_ptr_vec (list);
 
-      add_status = maybe_add_completion (tracker, candidate);
+  /* If complete_line_internal returned more completions than were
+     recorded by the completion tracker, then the completer function that
+     was run does not support completion tracking.  In this case,
+     do a final test for too many completions.
 
-      switch (add_status)
+     Duplicates are also removed here.  Otherwise the user is left
+     scratching his/her head: readline and complete_command will remove
+     duplicates, and if removal of duplicates there brings the total under
+     max_completions the user may think gdb quit searching too early.  */
+
+  if (VEC_length (char_ptr, list) > htab_elements (cdata->tracker))
+    {
+      enum add_completion_status max_reached = ADD_COMPLETION_OK;
+
+      /* Clear the tracker so that we can re-use it to count the number
+	 of returned completions.  */
+      htab_empty (cdata->tracker);
+
+      for (ix = 0; (max_reached == ADD_COMPLETION_OK
+		    && VEC_iterate (char_ptr, list, ix, candidate)); ++ix)
 	{
-	  case MAYBE_ADD_COMPLETION_OK:
-	    VEC_safe_push (char_ptr, result, xstrdup (candidate));
-	    break;
-	  case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
-	    VEC_safe_push (char_ptr, result, xstrdup (candidate));
-	    max_reached = 1;
-	    break;
-      	  case MAYBE_ADD_COMPLETION_MAX_REACHED:
-	    gdb_assert_not_reached ("more than max completions reached");
-	  case MAYBE_ADD_COMPLETION_DUPLICATE:
-	    break;
+	  max_reached = add_completion (cdata, &result, candidate, NULL, NULL);
 	}
+
+      /* The return result has been assembled and the original list from
+	 complete_line_internal is no longer needed.  Free it.  */
+      do_cleanups (list_cleanup);
+    }
+  else
+    {
+      /* There is a valid tracker for the completion -- simply return
+	 the completed list.  */
+      discard_cleanups (list_cleanup);
+      result = list;
     }
 
-  do_cleanups (cleanups);
+  do_cleanups (cdata_cleanup);
 
   return result;
 }
@@ -1074,9 +1156,14 @@ char *
 gdb_completion_word_break_characters (void)
 {
   VEC (char_ptr) *list;
+  struct completer_data *cdata;
+  struct cleanup *cleanup;
 
-  list = complete_line_internal (NULL, rl_line_buffer, rl_line_buffer,
+  cdata = new_completer_data (max_completions);
+  cleanup = make_cleanup (free_completer_data, cdata);
+  list = complete_line_internal (cdata, rl_line_buffer, rl_line_buffer,
 				 rl_point, handle_brkchars);
+  do_cleanups (cleanup);
   gdb_assert (list == NULL);
   return rl_completer_word_break_characters;
 }
diff --git a/gdb/completer.h b/gdb/completer.h
index 7139c0a..43d1321 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -132,59 +132,31 @@ extern const char *skip_quoted (const char *);
 
 extern int max_completions;
 
-/* Object to track how many unique completions have been generated.
-   Used to limit the size of generated completion lists.  */
+/* Return values for add_completion.  */
 
-typedef htab_t completion_tracker_t;
-
-/* Create a new completion tracker.
-   The result is a hash table to track added completions, or NULL
-   if max_completions <= 0.  If max_completions < 0, tracking is disabled.
-   If max_completions == 0, the max is indeed zero.  */
-
-extern completion_tracker_t new_completion_tracker (void);
-
-/* Make a cleanup to free a completion tracker, and reset its pointer
-   to NULL.  */
-
-extern struct cleanup *make_cleanup_free_completion_tracker
-		      (completion_tracker_t *tracker_ptr);
-
-/* Return values for maybe_add_completion.  */
-
-enum maybe_add_completion_enum
+enum add_completion_status
 {
-  /* NAME has been recorded and max_completions has not been reached,
-     or completion tracking is disabled (max_completions < 0).  */
-  MAYBE_ADD_COMPLETION_OK,
-
-  /* NAME has been recorded and max_completions has been reached
-     (thus the caller can stop searching).  */
-  MAYBE_ADD_COMPLETION_OK_MAX_REACHED,
-
-  /* max-completions entries has been reached.
-     Whether NAME is a duplicate or not is not determined.  */
-  MAYBE_ADD_COMPLETION_MAX_REACHED,
-
-  /* NAME has already been recorded.
-     Note that this is never returned if completion tracking is disabled
-     (max_completions < 0).  */
-  MAYBE_ADD_COMPLETION_DUPLICATE
+  /* Completion was added -- keep looking for more.  */
+  ADD_COMPLETION_OK,
+
+  /* Maximum number of completions has been reached or the maximum
+     has already been reached by a previous call to add_completion.
+     Callers can make no assumptions about whether the completion was
+     added or not.  */
+  ADD_COMPLETION_MAX_REACHED
 };
 
-/* Add the completion NAME to the list of generated completions if
-   it is not there already.
-   If max_completions is negative, nothing is done, not even watching
-   for duplicates, and MAYBE_ADD_COMPLETION_OK is always returned.
+/* Add the given MATCH to the completer result in CDATA.
+   Returns one of the above enum add_completion_status codes to indicate
+   whether the caller should continue to look for/compute more completions.
 
-   If MAYBE_ADD_COMPLETION_MAX_REACHED is returned, callers are required to
-   record at least one more completion.  The final list will be pruned to
-   max_completions, but recording at least one more than max_completions is
-   the signal to the completion machinery that too many completions were
-   found.  */
+   If TEXT and WORD are non-NULL, then portions of the strings will be
+   saved into the completion list.  See completer_strdup for more.  */
 
-extern enum maybe_add_completion_enum
-  maybe_add_completion (completion_tracker_t tracker, char *name);
+extern enum add_completion_status
+  add_completion (struct completer_data *cdata,
+		  VEC (char_ptr) **result, const char *match,
+		  const char *text, const char *word);
 
 /* Wrapper to throw MAX_COMPLETIONS_REACHED_ERROR.  */ 
 
diff --git a/gdb/symtab.c b/gdb/symtab.c
index f4407a0..34eb6d7 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5046,15 +5046,6 @@ static VEC (char_ptr) *return_val;
   completion_list_add_name						\
   (cdata, MSYMBOL_NATURAL_NAME (symbol), (sym_text), (len), (text), (word))
 
-/* Tracker for how many unique completions have been generated.  Used
-   to terminate completion list generation early if the list has grown
-   to a size so large as to be useless.  This helps avoid GDB seeming
-   to lock up in the event the user requests to complete on something
-   vague that necessitates the time consuming expansion of many symbol
-   tables.  */
-
-static completion_tracker_t completion_tracker;
-
 /*  Test to see if the symbol specified by SYMNAME (which is already
    demangled for C++ symbols) matches SYM_TEXT in the first SYM_TEXT_LEN
    characters.  If so, add it to the current completion list.  */
@@ -5070,50 +5061,11 @@ completion_list_add_name (struct completer_data *cdata,
     return;
 
   /* We have a match for a completion, so add SYMNAME to the current list
-     of matches.  Note that the name is moved to freshly malloc'd space.  */
-
-  {
-    char *newobj;
-    enum maybe_add_completion_enum add_status;
-
-    if (word == sym_text)
-      {
-	newobj = xmalloc (strlen (symname) + 5);
-	strcpy (newobj, symname);
-      }
-    else if (word > sym_text)
-      {
-	/* Return some portion of symname.  */
-	newobj = xmalloc (strlen (symname) + 5);
-	strcpy (newobj, symname + (word - sym_text));
-      }
-    else
-      {
-	/* Return some of SYM_TEXT plus symname.  */
-	newobj = xmalloc (strlen (symname) + (sym_text - word) + 5);
-	strncpy (newobj, word, sym_text - word);
-	newobj[sym_text - word] = '\0';
-	strcat (newobj, symname);
-      }
-
-    add_status = maybe_add_completion (completion_tracker, newobj);
+     of matches.  */
 
-    switch (add_status)
-      {
-      case MAYBE_ADD_COMPLETION_OK:
-	VEC_safe_push (char_ptr, return_val, newobj);
-	break;
-      case MAYBE_ADD_COMPLETION_OK_MAX_REACHED:
-	VEC_safe_push (char_ptr, return_val, newobj);
-	throw_max_completions_reached_error ();
-      case MAYBE_ADD_COMPLETION_MAX_REACHED:
-	xfree (newobj);
-	throw_max_completions_reached_error ();
-      case MAYBE_ADD_COMPLETION_DUPLICATE:
-	xfree (newobj);
-	break;
-      }
-  }
+    if (add_completion (cdata, &return_val, symname, sym_text, word)
+	== ADD_COMPLETION_MAX_REACHED)
+      throw_max_completions_reached_error ();
 }
 
 /* ObjC: In case we are completing on a selector, look as the msymbol
@@ -5358,7 +5310,6 @@ default_make_symbol_completion_list_break_on_1 (struct completer_data *cdata,
   /* Length of sym_text.  */
   int sym_text_len;
   struct add_name_data datum;
-  struct cleanup *cleanups;
 
   /* Now look for the symbol we are supposed to complete on.  */
   {
@@ -5429,9 +5380,6 @@ default_make_symbol_completion_list_break_on_1 (struct completer_data *cdata,
     }
   gdb_assert (sym_text[sym_text_len] == '\0' || sym_text[sym_text_len] == '(');
 
-  completion_tracker = new_completion_tracker ();
-  cleanups = make_cleanup_free_completion_tracker (&completion_tracker);
-
   datum.sym_text = sym_text;
   datum.sym_text_len = sym_text_len;
   datum.text = text;
@@ -5549,8 +5497,6 @@ default_make_symbol_completion_list_break_on_1 (struct completer_data *cdata,
       /* User-defined macros are always visible.  */
       macro_for_each (macro_user_macros, add_macro_name, &datum);
     }
-
-  do_cleanups (cleanups);
 }
 
 VEC (char_ptr) *
diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
index 1eb0fd8..710aac0 100644
--- a/gdb/testsuite/gdb.base/completion.exp
+++ b/gdb/testsuite/gdb.base/completion.exp
@@ -777,6 +777,84 @@ gdb_test_multiple "" "$test" {
 }
 
 #
+# Tests for the location completer
+#
+
+# Turn off pending breakpoint support so that we don't get queried
+# all the time.
+gdb_test_no_output "set breakpoint pending off"
+
+set subsrc [string range $srcfile 0 [expr {[string length $srcfile] - 3}]]
+set test "tab complete break $subsrc"
+send_gdb "break $subsrc\t\t"
+gdb_test_multiple "" $test {
+    -re "break\.c.*break1\.c.*$gdb_prompt " {
+	send_gdb "1\t\n"
+	gdb_test_multiple "" $test {
+	    -re ".*Function \"$srcfile2\" not defined\..*$gdb_prompt " {
+		pass $test
+	    }
+	    -re "$gdb_prompt p$" {
+		fail $test
+	    }
+	}
+    }
+
+    -re "$gdb_prompt p$" {
+	fail $test
+    }
+}
+
+gdb_test "complete break $subsrc" "break\.c.*break1\.c"
+
+# gdb/17960
+set test "tab complete break $srcfile:ma"
+send_gdb "break $srcfile:ma\t"
+gdb_test_multiple "" $test {
+    -re "break $srcfile:main " {
+	send_gdb "\n"
+	gdb_test_multiple "" $test {
+	    -re ".*Breakpoint.*at .*/$srcfile, line .*$gdb_prompt " {
+		pass $test
+		gdb_test_no_output "delete breakpoint \$bpnum" \
+		    "delete breakpoint for $test"
+	    }
+	    -re "$gdb_prompt p$" {
+		fail $test
+	    }
+	}
+    }
+    -re "$gdb_prompt p$" {
+	fail $test
+    }
+}
+
+gdb_test "complete break $srcfile:ma" "break\.c:main"
+
+set test "tab complete break need"
+send_gdb "break need\t"
+gdb_test_multiple "" $test {
+    -re "break need_malloc " {
+	send_gdb "\n"
+	gdb_test_multiple "" $test {
+	    -re ".*Breakpoint.*at .*/$srcfile, line .*$gdb_prompt " {
+		pass $test
+		gdb_test_no_output "delete breakpoint \$bpnum" \
+		    "delete breakpoint for $test"
+	    }
+	    -re "$gdb_prompt p$" {
+		fail $test
+	    }
+	}
+    }
+    -re "$gdb_prompt p$" {
+	fail $test
+    }
+}
+
+gdb_test "complete break need" "need_malloc"
+
+#
 # Completion limiting.
 #