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 02/18] Remove completion_tracker_t from the public completion API.


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
--- make_file_symbol_completion_list
(...)
---- completion_list_add_name
----- maybe_add_completion
----> gdb_assert (tracker != NULL);

The tracker in maybe_add_completion is a static global in symtab.c called
completion_tracker.  It is initialized by
default_make_symbol_completion_list_break_on_1 (which is a defaulted
language vector method).  In this case, this function is never called
and completion_tracker is left NULL/uninitialized.

gdb/ChangeLog

	PR gdb/17960
	* completer.c (struct completer_data) <tracker>: Change type
	to htab_t.
	(DEFAULT_MAX_COMPLETIONS): Define.
	(max_completions): Initialize global to above value.
	(new_completion_tracker): Delete.
	(new_completer_data): New function.
	(make_cleanup_free_completion_tracker): Delete.
	(free_completer_data): New function.
	(maybe_add_completion): Change argument `tracker' to struct
	completer_data and use the tracker in that structure to
	do completion limiting.
	(complete_line): Remove completion_tracker code.
	Allocate a completer_data and pass that to complete_line_internal.
	Remove now unused cleanup.
	When complete_line_internal returns more completions than are
	in the tracker, reset the tracker and use maybe_add_completion
	to implement completion tracking.
	(gdb_completion_word_break_characters): Use completer_data.
	* completer.h (completion_tracker_t): Remove.
	(new_completion_tracker): Remove.
	(make_cleanup_free_completion_tracker): Remove.
	(maybe_add_completion): Replace `completion_tracker_t tracker'
	with `struct completer_data *cdata'.
	* symtab.c (completion_tracker): Remove global variable.
	(completion_list_add_name): Pass completer_data to
	maybe_add_completion instead of completion_tracker_t.
	(default_make_symbol_completion_list_break_on_1): Remove now
	unused cleanup.
	Do not allocate a completion tracker.  Use the supplied
	completer_data instead.

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                       |  157 ++++++++++++++++++---------------
 gdb/completer.h                       |   20 ----
 gdb/symtab.c                          |   17 ----
 gdb/testsuite/gdb.base/completion.exp |   78 ++++++++++++++++
 4 files changed, 168 insertions(+), 104 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index f2b31e9..6708bb1 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -91,8 +91,8 @@ 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;
 };
 
 
@@ -802,47 +802,38 @@ 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.  */
-
-completion_tracker_t
-new_completion_tracker (void)
-{
-  if (max_completions <= 0)
-    return NULL;
+/* Allocate a new completer data structure.  */
 
-  return htab_create_alloc (max_completions,
-			    htab_hash_string, (htab_eq) streq,
-			    NULL, xcalloc, xfree);
-}
-
-/* Cleanup routine to free a completion tracker and reset the pointer
-   to NULL.  */
-static void
-free_completion_tracker (void *p)
+static struct completer_data *
+new_completer_data (int size)
 {
-  completion_tracker_t *tracker_ptr = p;
+  struct completer_data *cdata = XCNEW (struct completer_data);
 
-  htab_delete (*tracker_ptr);
-  *tracker_ptr = NULL;
+  cdata->tracker
+    = htab_create_alloc ((size < 0 ? DEFAULT_MAX_COMPLETIONS : size),
+			 htab_hash_string, (htab_eq) streq, NULL,
+			 xcalloc, xfree);
+  return cdata;
 }
 
-/* See completer.h.  */
+/* Free the completion data represented by P.  */
 
-struct cleanup *
-make_cleanup_free_completion_tracker (completion_tracker_t *tracker_ptr)
+static void
+free_completer_data (void *p)
 {
-  if (*tracker_ptr == NULL)
-    return make_cleanup (null_cleanup, NULL);
+  struct completer_data *cdata = p;
 
-  return make_cleanup (free_completion_tracker, tracker_ptr);
+  htab_delete (cdata->tracker);
+  xfree (cdata);
 }
 
 /* See completer.h.  */
 
 enum maybe_add_completion_enum
-maybe_add_completion (completion_tracker_t tracker, char *name)
+maybe_add_completion (struct completer_data *cdata, char *name)
 {
   void **slot;
 
@@ -851,19 +842,17 @@ 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);
 }
@@ -892,54 +881,79 @@ 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;
+  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);
+
+  /* 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.
+
+     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.  */
 
-      add_status = maybe_add_completion (tracker, candidate);
+  if (VEC_length (char_ptr, list) > htab_elements (cdata->tracker))
+    {
+      /* Clear the tracker so that we can re-use it to count the number
+	 of returned completions.  */
+      htab_empty (cdata->tracker);
 
-      switch (add_status)
+      for (ix = 0, max_reached = 0;
+	   !max_reached && 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;
+	  enum maybe_add_completion_enum add_status;
+
+	  add_status = maybe_add_completion (cdata, candidate);
+
+	  switch (add_status)
+	    {
+	    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;
+	    }
 	}
+
+      /* 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;
 }
@@ -1032,9 +1046,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 f32c696..003c66c 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -128,24 +128,6 @@ 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.  */
-
-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
@@ -180,7 +162,7 @@ enum maybe_add_completion_enum
    found.  */
 
 extern enum maybe_add_completion_enum
-  maybe_add_completion (completion_tracker_t tracker, char *name);
+  maybe_add_completion (struct completer_data *cdata, char *name);
 
 /* Wrapper to throw MAX_COMPLETIONS_REACHED_ERROR.  */ 
 
diff --git a/gdb/symtab.c b/gdb/symtab.c
index c0562e1..0736582 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5017,15 +5017,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.  */
@@ -5067,7 +5058,7 @@ completion_list_add_name (struct completer_data *cdata,
 	strcat (newobj, symname);
       }
 
-    add_status = maybe_add_completion (completion_tracker, newobj);
+    add_status = maybe_add_completion (cdata, newobj);
 
     switch (add_status)
       {
@@ -5329,7 +5320,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.  */
   {
@@ -5400,9 +5390,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;
@@ -5520,8 +5507,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 f77bfe2..5afd851 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.
 #
 


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