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

Keith Seitz
2015-04-13T19:23:11Z

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

\f
@@ -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.
#