[3/3,v2] Implement completion limiting
Commit Message
This commit adds a new exception, TOO_MANY_COMPLETIONS_ERROR, to be
thrown whenever the completer has generated too many candidates to
be useful. A new user-settable variable, "max_completions", is added
to control this behaviour. A top-level completion limit is added to
complete_line_internal, as the final check to ensure the user never
sees too many completions. An additional limit is added to
default_make_symbol_completion_list_break_on, to halt time-consuming
symbol table expansions.
gdb/ChangeLog:
PR cli/9007
PR cli/11920
PR cli/15548
* common/common-exceptions.h (enum errors)
<TOO_MANY_COMPLETIONS_ERROR>: New value.
* completer.h (completion_tracker_t): New typedef.
(new_completion_tracker): New declaration.
(make_cleanup_free_completion_tracker): Likewise.
(maybe_limit_completions): Likewise.
* completer.c [TUI]: Include tui/tui.h and tui/tui-io.h.
(max_completions): New static variable.
(new_completion_tracker): New function.
(make_cleanup_free_completion_tracker): Likewise.
(maybe_limit_completions): Likewise.
(complete_line_internal): Do not generate any completions if
max_completions = 0. Limit the number of completions if
max_completions >= 0.
(line_completion_function): Handle TOO_MANY_COMPLETIONS_ERROR.
(_initialize_completer): New declaration and function.
* symtab.c: Include completer.h.
(completion_tracker): New static variable.
(completion_list_add_name): Call maybe_limit_completions.
(default_make_symbol_completion_list_break_on): Maintain
completion_tracker across calls to completion_list_add_name.
* NEWS (New Options): Mention set/show max-completions.
gdb/doc/ChangeLog:
* gdb.texinfo (Command Completion): Document new
"set/show max-completions" option.
gdb/testsuite/ChangeLog:
* gdb.base/completion.exp: Disable completion limiting for
existing tests. Add new tests to check completion limiting.
* gdb.linespec/ls-errs.exp: Disable completion limiting.
---
gdb/ChangeLog | 28 ++++++
gdb/NEWS | 11 ++-
gdb/common/common-exceptions.h | 4 +
gdb/completer.c | 146 ++++++++++++++++++++++++++++++--
gdb/completer.h | 23 +++++
gdb/doc/ChangeLog | 5 +
gdb/doc/gdb.texinfo | 24 +++++
gdb/symtab.c | 22 +++++-
gdb/testsuite/ChangeLog | 6 ++
gdb/testsuite/gdb.base/completion.exp | 33 +++++++
gdb/testsuite/gdb.linespec/ls-errs.exp | 3 +
11 files changed, 297 insertions(+), 8 deletions(-)
Comments
> From: Gary Benson <gbenson@redhat.com>
> Cc: Doug Evans <xdje42@gmail.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 27 Nov 2014 13:16:08 +0000
>
> This commit adds a new exception, TOO_MANY_COMPLETIONS_ERROR, to be
> thrown whenever the completer has generated too many candidates to
> be useful. A new user-settable variable, "max_completions", is added
> to control this behaviour. A top-level completion limit is added to
> complete_line_internal, as the final check to ensure the user never
> sees too many completions. An additional limit is added to
> default_make_symbol_completion_list_break_on, to halt time-consuming
> symbol table expansions.
>
> gdb/ChangeLog:
>
> PR cli/9007
> PR cli/11920
> PR cli/15548
> * common/common-exceptions.h (enum errors)
> <TOO_MANY_COMPLETIONS_ERROR>: New value.
> * completer.h (completion_tracker_t): New typedef.
> (new_completion_tracker): New declaration.
> (make_cleanup_free_completion_tracker): Likewise.
> (maybe_limit_completions): Likewise.
> * completer.c [TUI]: Include tui/tui.h and tui/tui-io.h.
> (max_completions): New static variable.
> (new_completion_tracker): New function.
> (make_cleanup_free_completion_tracker): Likewise.
> (maybe_limit_completions): Likewise.
> (complete_line_internal): Do not generate any completions if
> max_completions = 0. Limit the number of completions if
> max_completions >= 0.
> (line_completion_function): Handle TOO_MANY_COMPLETIONS_ERROR.
> (_initialize_completer): New declaration and function.
> * symtab.c: Include completer.h.
> (completion_tracker): New static variable.
> (completion_list_add_name): Call maybe_limit_completions.
> (default_make_symbol_completion_list_break_on): Maintain
> completion_tracker across calls to completion_list_add_name.
> * NEWS (New Options): Mention set/show max-completions.
>
> gdb/doc/ChangeLog:
>
> * gdb.texinfo (Command Completion): Document new
> "set/show max-completions" option.
OK for the documentation parts.
Thanks.
Gary Benson <gbenson@redhat.com> writes:
> This commit adds a new exception, TOO_MANY_COMPLETIONS_ERROR, to be
> thrown whenever the completer has generated too many candidates to
> be useful. A new user-settable variable, "max_completions", is added
> to control this behaviour. A top-level completion limit is added to
> complete_line_internal, as the final check to ensure the user never
> sees too many completions. An additional limit is added to
> default_make_symbol_completion_list_break_on, to halt time-consuming
> symbol table expansions.
>
> gdb/ChangeLog:
>
> PR cli/9007
> PR cli/11920
> PR cli/15548
> * common/common-exceptions.h (enum errors)
> <TOO_MANY_COMPLETIONS_ERROR>: New value.
> * completer.h (completion_tracker_t): New typedef.
> (new_completion_tracker): New declaration.
> (make_cleanup_free_completion_tracker): Likewise.
> (maybe_limit_completions): Likewise.
> * completer.c [TUI]: Include tui/tui.h and tui/tui-io.h.
> (max_completions): New static variable.
> (new_completion_tracker): New function.
> (make_cleanup_free_completion_tracker): Likewise.
> (maybe_limit_completions): Likewise.
> (complete_line_internal): Do not generate any completions if
> max_completions = 0. Limit the number of completions if
> max_completions >= 0.
> (line_completion_function): Handle TOO_MANY_COMPLETIONS_ERROR.
> (_initialize_completer): New declaration and function.
> * symtab.c: Include completer.h.
> (completion_tracker): New static variable.
> (completion_list_add_name): Call maybe_limit_completions.
> (default_make_symbol_completion_list_break_on): Maintain
> completion_tracker across calls to completion_list_add_name.
> * NEWS (New Options): Mention set/show max-completions.
>
> gdb/doc/ChangeLog:
>
> * gdb.texinfo (Command Completion): Document new
> "set/show max-completions" option.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/completion.exp: Disable completion limiting for
> existing tests. Add new tests to check completion limiting.
> * gdb.linespec/ls-errs.exp: Disable completion limiting.
Hi.
I played with the patch a bit.
I have a few questions on the u/i.
1) IWBN if, when "Too many possibilities" is hit, the user was still
shown the completions thus far. I'd rather not have to abort
the command I'm trying to do, increase max-completions, and
then try again (or anything else to try to find what I'm looking
for in order to complete the command). At least not if I don't have to:
the completions thus far may provide a hint at what I'm looking for.
Plus GDB has already computed them, might as well print them.
Imagine if the total count is MAX+1, the user might find it annoying
to not be shown anything just because the count is one beyond the max.
So instead of "Too many possibilities", how about printing
the completions thus far and then include a message saying
the list is clipped due to max-completions being reached?
[Maybe readline makes this difficult, but I think
it'd be really nice have. Thoughts?]
2) Readline has a limiting facility already: rl_completion_query_items.
But it's only applied after all completions have been computed so
it doesn't help us.
It would be good for the docs to explain the difference.
E.g. with the default of 200 for max-completions, if I do
(top-gdb) b dwarf2<tab><tab>
I get
Display all 159 possibilities? (y or n)
As a user, I'm kinda wondering why I'm being asked this if, for example,
I've explicitly set max-completions to some value.
[I know normally users might not even be aware of max-completions,
but suppose for the sake of discussion that they've set it to some value
larger than rl_completion_query_items.]
Note: rl_completion_query_items can be set in ~/.inputrc with
the completion-query-items parameter.
3) rl_completion_query_items uses a value of zero to mean unlimited,
whereas max_completions uses -1 (or "unlimited").
While it might be nice to provide a way to disable completions
completely (by setting max-completions to zero), I'm trying to decide
whether that benefit is sufficient to justify the inconsistency with
rl_completion_query_items. Thoughts?
4) Is there a use-case that involves wanting both
rl_completion_query_items and max_completions parameters?
Certainly we need to limit the number while we're building them,
but do we need both parameters? [IOW, could we fold them into one?]
I can imagine wanting to set max-completions to some large value
but still be given a prompt if the completion-query-items
threshold is reached, so I think we want both.
I'm just raising the possibility that maybe we don't want both
in case someone wants to comment.
At the least, it'd probably be good to mention how both interact in the docs.
> diff --git a/gdb/completer.c b/gdb/completer.c
> index a0f3fa3..4a2302c 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> [...]
> @@ -894,7 +984,35 @@ line_completion_function (const char *text, int matches,
> VEC_free (char_ptr, list);
> }
> index = 0;
> - list = complete_line (text, line_buffer, point);
> +
> + TRY_CATCH (ex, RETURN_MASK_ALL)
> + list = complete_line (text, line_buffer, point);
> +
> + if (ex.reason < 0)
> + {
> + if (ex.error != TOO_MANY_COMPLETIONS_ERROR)
> + throw_exception (ex);
> +
> + if (rl_completion_type != TAB)
> + {
> +#if defined(TUI)
> + if (tui_active)
> + {
> + tui_puts ("\n");
> + tui_puts (ex.message);
> + tui_puts ("\n");
> + }
> + else
> +#endif
> + {
> + rl_crlf ();
> + fputs (ex.message, rl_outstream);
> + rl_crlf ();
> + }
> +
> + rl_on_new_line ();
> + }
> + }
> }
>
> /* If we found a list of potential completions during initialization
Bubbling up TUI implementation details into GDB core gives me pause.
I'm left wondering if there are more problems, and this is just fixing
one of them. I see that TUI has special code for readline,
(grep for readline in tui-io.c)
so at the least I'm wondering why this is necessary.
And if it is, let's push it down into tui/ as much as possible
(with a comment explaining why the code exists :-)).
Doug Evans wrote:
> Gary Benson <gbenson@redhat.com> writes:
> > This commit adds a new exception, TOO_MANY_COMPLETIONS_ERROR, to
> > be thrown whenever the completer has generated too many candidates
> > to be useful. A new user-settable variable, "max_completions", is
> > added to control this behaviour. A top-level completion limit is
> > added to complete_line_internal, as the final check to ensure the
> > user never sees too many completions. An additional limit is
> > added to default_make_symbol_completion_list_break_on, to halt
> > time-consuming symbol table expansions.
>
> 1) IWBN if, when "Too many possibilities" is hit, the user was still
> shown the completions thus far. I'd rather not have to abort the
> command I'm trying to do, increase max-completions, and then try
> again (or anything else to try to find what I'm looking for in order
> to complete the command). At least not if I don't have to: the
> completions thus far may provide a hint at what I'm looking for.
> Plus GDB has already computed them, might as well print them.
> Imagine if the total count is MAX+1, the user might find it annoying
> to not be shown anything just because the count is one beyond the
> max.
> So instead of "Too many possibilities", how about printing the
> completions thus far and then include a message saying the list is
> clipped due to max-completions being reached? [Maybe readline makes
> this difficult, but I think it'd be really nice have. Thoughts?]
It's a nice idea but I'm not volunteering to implement it :)
I already spent too much time figuring out how to thread things
through readline.
> 2) Readline has a limiting facility already: rl_completion_query_items.
> But it's only applied after all completions have been computed so it
> doesn't help us.
> It would be good for the docs to explain the difference.
> E.g. with the default of 200 for max-completions, if I do
>
> (top-gdb) b dwarf2<tab><tab>
>
> I get
>
> Display all 159 possibilities? (y or n)
>
> As a user, I'm kinda wondering why I'm being asked this if, for
> example, I've explicitly set max-completions to some value.
> [I know normally users might not even be aware of max-completions,
> but suppose for the sake of discussion that they've set it to some
> value larger than rl_completion_query_items.]
> Note: rl_completion_query_items can be set in ~/.inputrc with
> the completion-query-items parameter.
rl_completion_query_items is a different thing.
From http://web.mit.edu/gnu/doc/html/rlman_2.html:
Up to this many items will be displayed in response to a possible-
completions call. After that, we ask the user if she is sure she
wants to see them all. The default value is 100.
Basically it's the threshold readline uses to decide whether to say
"Display all 159 possibilities? (y or n)" or just print all the
candidates without asking.
> 3) rl_completion_query_items uses a value of zero to mean unlimited,
> whereas max_completions uses -1 (or "unlimited"). While it might be
> nice to provide a way to disable completions completely (by setting
> max-completions to zero), I'm trying to decide whether that benefit
> is sufficient to justify the inconsistency with
> rl_completion_query_items. Thoughts?
GDB's "remotetimeout" and "trace-buffer-size" options use -1 to denote
unlimited, as do Scheme things defined with PARAM_ZUINTEGER_UNLIMITED.
I prefer "max-completions" being consistent with other GDB options
over "max-completions" being consistent with readline options.
It's not important to me that users can disable completion, I put the
functionality there because existing GDB code uses -1 so I had to
handle 0 somehow.
> 4) Is there a use-case that involves wanting both
> rl_completion_query_items and max_completions parameters?
> Certainly we need to limit the number while we're building them,
> but do we need both parameters? [IOW, could we fold them into one?]
> I can imagine wanting to set max-completions to some large value
> but still be given a prompt if the completion-query-items
> threshold is reached, so I think we want both.
> I'm just raising the possibility that maybe we don't want both
> in case someone wants to comment.
I think we want both.
> At the least, it'd probably be good to mention how both interact in
> in the docs.
I can do this.
> > diff --git a/gdb/completer.c b/gdb/completer.c
> > index a0f3fa3..4a2302c 100644
> > --- a/gdb/completer.c
> > +++ b/gdb/completer.c
> > [...]
> > @@ -894,7 +984,35 @@ line_completion_function (const char *text, int matche> > [...]
> > + if (rl_completion_type != TAB)
> > + {
> > +#if defined(TUI)
> > + if (tui_active)
> > + {
> > + tui_puts ("\n");
> > + tui_puts (ex.message);
> > + tui_puts ("\n");
> > + }
> > + else
> > +#endif
> > + {
> > + rl_crlf ();
> > + fputs (ex.message, rl_outstream);
> > + rl_crlf ();
> > + }
> > +
> > + rl_on_new_line ();
> > + }
>
> Bubbling up TUI implementation details into GDB core gives me pause.
> I'm left wondering if there are more problems, and this is just
> fixing one of them. I see that TUI has special code for readline,
> (grep for readline in tui-io.c) so at the least I'm wondering why
> this is necessary.
> And if it is, let's push it down into tui/ as much as possible
> (with a comment explaining why the code exists :-)).
I'm no TUI expert, so I can't comment on whether it's necessary or
not. Assuming it is necessary, I don't know how I could remove this
block without vectorizing the CLI/TUI interface... or is this done
already? There are other places where "#ifdef TUI" and variants
are used, at least four:
gdb/cli/cli-cmds.c
gdb/main.c
gdb/printcmd.c
gdb/utils.c
Cheers,
Gary
Gary Benson <gbenson@redhat.com> writes:
> Doug Evans wrote:
>> 1) IWBN if, when "Too many possibilities" is hit, the user was still
>> shown the completions thus far. I'd rather not have to abort the
>> command I'm trying to do, increase max-completions, and then try
>> again (or anything else to try to find what I'm looking for in order
>> to complete the command). At least not if I don't have to: the
>> completions thus far may provide a hint at what I'm looking for.
>> Plus GDB has already computed them, might as well print them.
>> Imagine if the total count is MAX+1, the user might find it annoying
>> to not be shown anything just because the count is one beyond the
>> max.
>> So instead of "Too many possibilities", how about printing the
>> completions thus far and then include a message saying the list is
>> clipped due to max-completions being reached? [Maybe readline makes
>> this difficult, but I think it'd be really nice have. Thoughts?]
>
> It's a nice idea but I'm not volunteering to implement it :)
> I already spent too much time figuring out how to thread things
> through readline.
One thought I had was one could add a final completion entry
that was the message.
Would that work?
>> 2) Readline has a limiting facility already: rl_completion_query_items.
>> But it's only applied after all completions have been computed so it
>> doesn't help us.
>> It would be good for the docs to explain the difference.
>> E.g. with the default of 200 for max-completions, if I do
>>
>> (top-gdb) b dwarf2<tab><tab>
>>
>> I get
>>
>> Display all 159 possibilities? (y or n)
>>
>> As a user, I'm kinda wondering why I'm being asked this if, for
>> example, I've explicitly set max-completions to some value.
>> [I know normally users might not even be aware of max-completions,
>> but suppose for the sake of discussion that they've set it to some
>> value larger than rl_completion_query_items.]
>> Note: rl_completion_query_items can be set in ~/.inputrc with
>> the completion-query-items parameter.
>
> rl_completion_query_items is a different thing.
> [...]
I realize that.
Still, the two interact, and we should think through the u/i.
>> 3) rl_completion_query_items uses a value of zero to mean unlimited,
>> whereas max_completions uses -1 (or "unlimited"). While it might be
>> nice to provide a way to disable completions completely (by setting
>> max-completions to zero), I'm trying to decide whether that benefit
>> is sufficient to justify the inconsistency with
>> rl_completion_query_items. Thoughts?
>
> GDB's "remotetimeout" and "trace-buffer-size" options use -1 to denote
> unlimited, as do Scheme things defined with PARAM_ZUINTEGER_UNLIMITED.
> I prefer "max-completions" being consistent with other GDB options
> over "max-completions" being consistent with readline options.
> It's not important to me that users can disable completion, I put the
> functionality there because existing GDB code uses -1 so I had to
> handle 0 somehow.
OTOH,
[dje@seba gdb]$ grep add_setshow_uinteger *.c | wc
11 50 797
[dje@seba gdb]$ grep add_setshow_zuinteger_unlimited *.c | wc
2 8 163
gdb uses a mix, so consistency with gdb is a bit of a toss up.
From a u/i perspective does a value of zero for max-completions
make sense? Maybe it does, but I dunno, it doesn't feel like it.
>> 4) Is there a use-case that involves wanting both
>> rl_completion_query_items and max_completions parameters?
>> Certainly we need to limit the number while we're building them,
>> but do we need both parameters? [IOW, could we fold them into one?]
>> I can imagine wanting to set max-completions to some large value
>> but still be given a prompt if the completion-query-items
>> threshold is reached, so I think we want both.
>> I'm just raising the possibility that maybe we don't want both
>> in case someone wants to comment.
>
> I think we want both.
"works for me"
>> At the least, it'd probably be good to mention how both interact in
>> in the docs.
>
> I can do this.
Thanks. btw, I noticed this in completer.c:
/* Generate completions all at once. Returns a vector of strings
allocated with xmalloc. Returns NULL if there are no completions
or if max_completions is 0. Throws TOO_MANY_COMPLETIONS_ERROR if
max_completions is greater than zero and the number of completions
is greater than max_completions.
But that's not what the code does AFAICT:
/* Possibly throw TOO_MANY_COMPLETIONS_ERROR. Individual
completers may do this too, to avoid unnecessary work,
but this is the ultimate check that stops users seeing
more completions than they wanted. */
if (max_completions >= 0)
>> > diff --git a/gdb/completer.c b/gdb/completer.c
>> > index a0f3fa3..4a2302c 100644
>> > --- a/gdb/completer.c
>> > +++ b/gdb/completer.c
>> > [...]
>> > @@ -894,7 +984,35 @@ line_completion_function (const char *text, int matche> > [...]
>> > + if (rl_completion_type != TAB)
>> > + {
>> > +#if defined(TUI)
>> > + if (tui_active)
>> > + {
>> > + tui_puts ("\n");
>> > + tui_puts (ex.message);
>> > + tui_puts ("\n");
>> > + }
>> > + else
>> > +#endif
>> > + {
>> > + rl_crlf ();
>> > + fputs (ex.message, rl_outstream);
>> > + rl_crlf ();
>> > + }
>> > +
>> > + rl_on_new_line ();
>> > + }
>>
>> Bubbling up TUI implementation details into GDB core gives me pause.
>> I'm left wondering if there are more problems, and this is just
>> fixing one of them. I see that TUI has special code for readline,
>> (grep for readline in tui-io.c) so at the least I'm wondering why
>> this is necessary.
>> And if it is, let's push it down into tui/ as much as possible
>> (with a comment explaining why the code exists :-)).
>
> I'm no TUI expert, so I can't comment on whether it's necessary or
> not. Assuming it is necessary, I don't know how I could remove this
> block without vectorizing the CLI/TUI interface... or is this done
> already? There are other places where "#ifdef TUI" and variants
> are used, at least four:
>
> gdb/cli/cli-cmds.c
> gdb/main.c
> gdb/printcmd.c
> gdb/utils.c
If we don't know whether it's necessary then why are we adding it?
["it" being the test for tui_active and the following code]
I don't understand. Was this derived from another place in gdb
that needed to do a similar thing? I grepped all uses of tui_active
outside of tui/*.c and didn't see anything.
One hope I had was that this would be enough:
>> > + rl_crlf ();
>> > + fputs (ex.message, rl_outstream);
>> > + rl_crlf ();
and that the efforts tui/*.c goes to to support readline would
make that work regardless of the value of tui_active.
But I confess I haven't tried it.
I wouldn't suggest vectorizing the tui interface.
But I do, at the least, want to understand why this is necessary
("this" being the test for tui_active and the different code
depending on whether it is true or not),
and if it is then I would at a minimum put this code:
>> > +#if defined(TUI)
>> > + if (tui_active)
>> > + {
>> > + tui_puts ("\n");
>> > + tui_puts (ex.message);
>> > + tui_puts ("\n");
>> > + }
>> > + else
>> > +#endif
>> > + {
>> > + rl_crlf ();
>> > + fputs (ex.message, rl_outstream);
>> > + rl_crlf ();
>> > + }
>> > +
>> > + rl_on_new_line ();
into a function and call it from line_completion_function.
completer.c up until now has had zero references to tui_*,
and adding one with no explanation of why makes the code
hard to reason about.
Doug Evans writes:
> Gary Benson <gbenson@redhat.com> writes:
> > Doug Evans wrote:
> >> 1) IWBN if, when "Too many possibilities" is hit, the user was still
> >> shown the completions thus far. I'd rather not have to abort the
> >> command I'm trying to do, increase max-completions, and then try
> >> again (or anything else to try to find what I'm looking for in order
> >> to complete the command). At least not if I don't have to: the
> >> completions thus far may provide a hint at what I'm looking for.
> >> Plus GDB has already computed them, might as well print them.
> >> Imagine if the total count is MAX+1, the user might find it annoying
> >> to not be shown anything just because the count is one beyond the
> >> max.
> >> So instead of "Too many possibilities", how about printing the
> >> completions thus far and then include a message saying the list is
> >> clipped due to max-completions being reached? [Maybe readline makes
> >> this difficult, but I think it'd be really nice have. Thoughts?]
> >
> > It's a nice idea but I'm not volunteering to implement it :)
> > I already spent too much time figuring out how to thread things
> > through readline.
>
> One thought I had was one could add a final completion entry
> that was the message.
> Would that work?
I looked into this a bit.
readline provides a hook to print the completion list:
rl_completion_display_matches_hook
and a routine to display the matches:
rl_display_match_list
The code in readline/complete.c:display_matches is
pretty straightforward (though they've apparently
forgotten to export a way for the hook to set
rl_display_fixed - we'll want to be as equivalent
as possible), so I think(!) this will be rather easy to do.
> One hope I had was that this would be enough:
>
> >> > + rl_crlf ();
> >> > + fputs (ex.message, rl_outstream);
> >> > + rl_crlf ();
>
> and that the efforts tui/*.c goes to to support readline would
> make that work regardless of the value of tui_active.
> But I confess I haven't tried it.
>
> I wouldn't suggest vectorizing the tui interface.
> But I do, at the least, want to understand why this is necessary
> ("this" being the test for tui_active and the different code
> depending on whether it is true or not),
> and if it is then I would at a minimum put this code:
>
> >> > +#if defined(TUI)
> >> > + if (tui_active)
> >> > + {
> >> > + tui_puts ("\n");
> >> > + tui_puts (ex.message);
> >> > + tui_puts ("\n");
> >> > + }
> >> > + else
> >> > +#endif
> >> > + {
> >> > + rl_crlf ();
> >> > + fputs (ex.message, rl_outstream);
> >> > + rl_crlf ();
> >> > + }
> >> > +
> >> > + rl_on_new_line ();
So that leaves this as just the remaining thing to resolve (AFAICT).
I'll look into this more next week.
I'd really like to get this into 7.9.
Doug Evans wrote:
> Doug Evans writes:
> > Gary Benson <gbenson@redhat.com> writes:
> > > Doug Evans wrote:
> > >> 1) IWBN if, when "Too many possibilities" is hit, the user was still
> > >> shown the completions thus far. I'd rather not have to abort the
> > >> command I'm trying to do, increase max-completions, and then try
> > >> again (or anything else to try to find what I'm looking for in order
> > >> to complete the command). At least not if I don't have to: the
> > >> completions thus far may provide a hint at what I'm looking for.
> > >> Plus GDB has already computed them, might as well print them.
> > >> Imagine if the total count is MAX+1, the user might find it annoying
> > >> to not be shown anything just because the count is one beyond the
> > >> max.
> > >> So instead of "Too many possibilities", how about printing the
> > >> completions thus far and then include a message saying the list is
> > >> clipped due to max-completions being reached? [Maybe readline makes
> > >> this difficult, but I think it'd be really nice have. Thoughts?]
> > >
> > > It's a nice idea but I'm not volunteering to implement it :)
> > > I already spent too much time figuring out how to thread things
> > > through readline.
> >
> > One thought I had was one could add a final completion entry
> > that was the message.
> > Would that work?
>
> I looked into this a bit.
> readline provides a hook to print the completion list:
> rl_completion_display_matches_hook
> and a routine to display the matches:
> rl_display_match_list
>
> The code in readline/complete.c:display_matches is
> pretty straightforward (though they've apparently
> forgotten to export a way for the hook to set
> rl_display_fixed - we'll want to be as equivalent
> as possible), so I think(!) this will be rather easy to do.
>
> > One hope I had was that this would be enough:
> >
> > >> > + rl_crlf ();
> > >> > + fputs (ex.message, rl_outstream);
> > >> > + rl_crlf ();
> >
> > and that the efforts tui/*.c goes to to support readline would
> > make that work regardless of the value of tui_active.
> > But I confess I haven't tried it.
> >
> > I wouldn't suggest vectorizing the tui interface.
> > But I do, at the least, want to understand why this is necessary
> > ("this" being the test for tui_active and the different code
> > depending on whether it is true or not),
> > and if it is then I would at a minimum put this code:
> >
> > >> > +#if defined(TUI)
> > >> > + if (tui_active)
> > >> > + {
> > >> > + tui_puts ("\n");
> > >> > + tui_puts (ex.message);
> > >> > + tui_puts ("\n");
> > >> > + }
> > >> > + else
> > >> > +#endif
> > >> > + {
> > >> > + rl_crlf ();
> > >> > + fputs (ex.message, rl_outstream);
> > >> > + rl_crlf ();
> > >> > + }
> > >> > +
> > >> > + rl_on_new_line ();
>
> So that leaves this as just the remaining thing to resolve (AFAICT).
> I'll look into this more next week.
> I'd really like to get this into 7.9.
If you want it in 7.9 then how about I commit it as it is then submit
a followup patch to remove the #ifdef, and you can make your own patch
to add whatever functionality you want. The readline part of this
series took a good week to get right and I can guarantee you this will
drag past 7.9 if I touch it.
Cheers,
Gary
On Wed, Jan 7, 2015 at 12:42 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> Doug Evans writes:
>> > Gary Benson <gbenson@redhat.com> writes:
>> > > Doug Evans wrote:
>> > >> 1) IWBN if, when "Too many possibilities" is hit, the user was still
>> > >> shown the completions thus far. I'd rather not have to abort the
>> > >> command I'm trying to do, increase max-completions, and then try
>> > >> again (or anything else to try to find what I'm looking for in order
>> > >> to complete the command). At least not if I don't have to: the
>> > >> completions thus far may provide a hint at what I'm looking for.
>> > >> Plus GDB has already computed them, might as well print them.
>> > >> Imagine if the total count is MAX+1, the user might find it annoying
>> > >> to not be shown anything just because the count is one beyond the
>> > >> max.
>> > >> So instead of "Too many possibilities", how about printing the
>> > >> completions thus far and then include a message saying the list is
>> > >> clipped due to max-completions being reached? [Maybe readline makes
>> > >> this difficult, but I think it'd be really nice have. Thoughts?]
>> > >
>> > > It's a nice idea but I'm not volunteering to implement it :)
>> > > I already spent too much time figuring out how to thread things
>> > > through readline.
>> >
>> > One thought I had was one could add a final completion entry
>> > that was the message.
>> > Would that work?
>>
>> I looked into this a bit.
>> readline provides a hook to print the completion list:
>> rl_completion_display_matches_hook
>> and a routine to display the matches:
>> rl_display_match_list
>>
>> The code in readline/complete.c:display_matches is
>> pretty straightforward (though they've apparently
>> forgotten to export a way for the hook to set
>> rl_display_fixed - we'll want to be as equivalent
>> as possible), so I think(!) this will be rather easy to do.
>>
>> > One hope I had was that this would be enough:
>> >
>> > >> > + rl_crlf ();
>> > >> > + fputs (ex.message, rl_outstream);
>> > >> > + rl_crlf ();
>> >
>> > and that the efforts tui/*.c goes to to support readline would
>> > make that work regardless of the value of tui_active.
>> > But I confess I haven't tried it.
>> >
>> > I wouldn't suggest vectorizing the tui interface.
>> > But I do, at the least, want to understand why this is necessary
>> > ("this" being the test for tui_active and the different code
>> > depending on whether it is true or not),
>> > and if it is then I would at a minimum put this code:
>> >
>> > >> > +#if defined(TUI)
>> > >> > + if (tui_active)
>> > >> > + {
>> > >> > + tui_puts ("\n");
>> > >> > + tui_puts (ex.message);
>> > >> > + tui_puts ("\n");
>> > >> > + }
>> > >> > + else
>> > >> > +#endif
>> > >> > + {
>> > >> > + rl_crlf ();
>> > >> > + fputs (ex.message, rl_outstream);
>> > >> > + rl_crlf ();
>> > >> > + }
>> > >> > +
>> > >> > + rl_on_new_line ();
>>
>> So that leaves this as just the remaining thing to resolve (AFAICT).
>> I'll look into this more next week.
>> I'd really like to get this into 7.9.
>
> If you want it in 7.9 then how about I commit it as it is then submit
> a followup patch to remove the #ifdef, and you can make your own patch
> to add whatever functionality you want. The readline part of this
> series took a good week to get right and I can guarantee you this will
> drag past 7.9 if I touch it.
No worries. I have a tentative combined patch which I'll submit tomorrow.
@@ -22,11 +22,20 @@
** $_any_caller_is(name [, number_of_frames])
** $_any_caller_matches(regexp [, number_of_frames])
-* New commands
+* New commands (for set/show, see "New options" below)
queue-signal signal-name-or-number
Queue a signal to be delivered to the thread when it is resumed.
+* New options
+
+set max-completions
+show max-completions
+ Set the maximum number of candidates to be considered during
+ completion. The default value is 200. This limit allows GDB
+ to avoid generating large completion lists, the computation of
+ which can cause the debugger to become temporarily unresponsive.
+
* On resume, GDB now always passes the signal the program had stopped
for to the thread the signal was sent to, even if the user changed
threads before resuming. Previously GDB would often (but not
@@ -99,6 +99,10 @@ enum errors {
/* Requested feature, method, mechanism, etc. is not supported. */
NOT_SUPPORTED_ERROR,
+ /* The number of candidates generated during line completion has
+ exceeded the user's specified limit. */
+ TOO_MANY_COMPLETIONS_ERROR,
+
/* Add more errors here. */
NR_ERRORS
};
@@ -39,6 +39,11 @@
#include "completer.h"
+#ifdef TUI
+#include "tui/tui.h"
+#include "tui/tui-io.h"
+#endif
+
/* Prototypes for local functions. */
static
char *line_completion_function (const char *text, int matches,
@@ -778,9 +783,67 @@ complete_line_internal (const char *text,
return list;
}
-/* Generate completions all at once. Returns a vector of strings.
- Each element is allocated with xmalloc. It can also return NULL if
- there are no completions.
+
+/* Maximum number of candidates to consider before the completer
+ bails by throwing TOO_MANY_COMPLETIONS_ERROR. Negative values
+ disable limiting. */
+static int max_completions = 200;
+
+/* See completer.h. */
+
+completion_tracker_t
+new_completion_tracker (void)
+{
+ if (max_completions < 1)
+ return NULL;
+
+ return htab_create_alloc (max_completions,
+ htab_hash_string, (htab_eq) streq,
+ NULL, xcalloc, xfree);
+}
+
+/* See completer.h. */
+
+struct cleanup *
+make_cleanup_free_completion_tracker (completion_tracker_t tracker)
+{
+ if (tracker == NULL)
+ return make_cleanup (null_cleanup, NULL);
+
+ return make_cleanup_htab_delete (tracker);
+}
+
+/* See completer.h. */
+
+void
+maybe_limit_completions (completion_tracker_t tracker, char *name)
+{
+ if (max_completions < 0)
+ return;
+
+ if (tracker != NULL)
+ {
+ void **slot = htab_find_slot (tracker, name, INSERT);
+
+ if (*slot != HTAB_EMPTY_ENTRY)
+ return;
+
+ if (htab_elements (tracker) <= max_completions)
+ {
+ *slot = name;
+ return;
+ }
+ }
+
+ throw_error (TOO_MANY_COMPLETIONS_ERROR,
+ _("Too many possibilities."));
+}
+
+/* Generate completions all at once. Returns a vector of strings
+ allocated with xmalloc. Returns NULL if there are no completions
+ or if max_completions is 0. Throws TOO_MANY_COMPLETIONS_ERROR if
+ max_completions is greater than zero and the number of completions
+ is greater than max_completions.
TEXT is the caller's idea of the "word" we are looking at.
@@ -793,8 +856,33 @@ complete_line_internal (const char *text,
VEC (char_ptr) *
complete_line (const char *text, const char *line_buffer, int point)
{
- return complete_line_internal (text, line_buffer,
- point, handle_completions);
+ VEC (char_ptr) *list = NULL;
+ struct cleanup *old_chain;
+
+ list = complete_line_internal (text, line_buffer, point,
+ handle_completions);
+ old_chain = make_cleanup_free_char_ptr_vec (list);
+
+ /* Possibly throw TOO_MANY_COMPLETIONS_ERROR. Individual
+ completers may do this too, to avoid unnecessary work,
+ but this is the ultimate check that stops users seeing
+ more completions than they wanted. */
+ if (max_completions >= 0)
+ {
+ completion_tracker_t tracker = new_completion_tracker ();
+ struct cleanup *limit_chain =
+ make_cleanup_free_completion_tracker (tracker);
+ char *candidate;
+ int ix;
+
+ for (ix = 0; VEC_iterate (char_ptr, list, ix, candidate); ++ix)
+ maybe_limit_completions (tracker, candidate);
+
+ do_cleanups (limit_chain);
+ }
+
+ discard_cleanups (old_chain);
+ return list;
}
/* Complete on command names. Used by "help". */
@@ -881,6 +969,8 @@ line_completion_function (const char *text, int matches,
if (matches == 0)
{
+ volatile struct gdb_exception ex;
+
/* The caller is beginning to accumulate a new set of
completions, so we need to find all of them now, and cache
them for returning one at a time on future calls. */
@@ -894,7 +984,35 @@ line_completion_function (const char *text, int matches,
VEC_free (char_ptr, list);
}
index = 0;
- list = complete_line (text, line_buffer, point);
+
+ TRY_CATCH (ex, RETURN_MASK_ALL)
+ list = complete_line (text, line_buffer, point);
+
+ if (ex.reason < 0)
+ {
+ if (ex.error != TOO_MANY_COMPLETIONS_ERROR)
+ throw_exception (ex);
+
+ if (rl_completion_type != TAB)
+ {
+#if defined(TUI)
+ if (tui_active)
+ {
+ tui_puts ("\n");
+ tui_puts (ex.message);
+ tui_puts ("\n");
+ }
+ else
+#endif
+ {
+ rl_crlf ();
+ fputs (ex.message, rl_outstream);
+ rl_crlf ();
+ }
+
+ rl_on_new_line ();
+ }
+ }
}
/* If we found a list of potential completions during initialization
@@ -978,3 +1096,19 @@ skip_quoted (const char *str)
{
return skip_quoted_chars (str, NULL, NULL);
}
+
+extern initialize_file_ftype _initialize_completer; /* -Wmissing-prototypes */
+
+void
+_initialize_completer (void)
+{
+ add_setshow_zuinteger_unlimited_cmd ("max-completions", no_class,
+ &max_completions, _("\
+Set maximum number of completion candidates."), _("\
+Show maximum number of completion candidates."), _("\
+Use this to limit the number of candidates considered\n\
+during completion. Specifying \"unlimited\" or -1\n\
+disables limiting. Note that setting either no limit or\n\
+a very large limit can make completion slow."),
+ NULL, NULL, &setlist, &showlist);
+}
@@ -63,4 +63,27 @@ extern const char *skip_quoted_chars (const char *, const char *,
extern const char *skip_quoted (const char *);
+/* 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. */
+
+extern completion_tracker_t new_completion_tracker (void);
+
+/* Make a cleanup to free a completion tracker. */
+
+extern struct cleanup *make_cleanup_free_completion_tracker
+ (completion_tracker_t tracker);
+
+/* Add the completion NAME to the list of generated completions if
+ it is not there already. Throw TOO_MANY_COMPLETIONS_ERROR if
+ max_completions >= 0 and the number of generated completions is
+ greater than max_completions. Do nothing if max_completions is
+ negative. */
+
+extern void maybe_limit_completions (completion_tracker_t tracker,
+ char *name);
+
#endif /* defined (COMPLETER_H) */
@@ -1600,6 +1600,30 @@ means @kbd{@key{META} ?}. You can type this either by holding down a
key designated as the @key{META} shift on your keyboard (if there is
one) while typing @kbd{?}, or as @key{ESC} followed by @kbd{?}.
+If the number of possible completions is large, @value{GDBN} will
+print a message rather than displaying the list:
+
+@smallexample
+(@value{GDBP}) b @key{TAB}@key{TAB}
+Too many possibilities.
+(@value{GDBP}) b
+@end smallexample
+
+@noindent
+This behavior can be controlled with the following commands:
+
+@table @code
+@kindex set max-completions
+@item set max-completions @var{limit}
+@itemx set max-completions unlimited
+Set the maximum number of candidates to be shown during completion.
+The default value is 200. Note that setting either no limit or a
+very large limit can make completion slow.
+@kindex show max-completions
+@item show max-completions
+Show the maximum number of candidates to be shown during completion.
+@end table
+
@cindex quotes in commands
@cindex completion of quoted strings
Sometimes the string you need, while logically a ``word'', may contain
@@ -60,6 +60,7 @@
#include "macroscope.h"
#include "parser-defs.h"
+#include "completer.h"
/* Forward declarations for local functions. */
@@ -4155,6 +4156,15 @@ static VEC (char_ptr) *return_val;
completion_list_add_name \
(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. */
+
+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. */
@@ -4195,6 +4205,12 @@ completion_list_add_name (const char *symname,
}
VEC_safe_push (char_ptr, return_val, new);
+
+ /* Throw TOO_MANY_COMPLETIONS_ERROR if we've generated too many
+ completions. We check this after pushing NEW to RETURN_VAL so
+ it's freed by default_make_symbol_completion_list_break_on's
+ cleanup if the exception is thrown. */
+ maybe_limit_completions (completion_tracker, new);
}
}
@@ -4429,7 +4445,7 @@ default_make_symbol_completion_list_break_on (const char *text,
/* Length of sym_text. */
int sym_text_len;
struct add_name_data datum;
- struct cleanup *back_to;
+ struct cleanup *back_to, *limit_chain;
/* Now look for the symbol we are supposed to complete on. */
{
@@ -4503,6 +4519,9 @@ default_make_symbol_completion_list_break_on (const char *text,
return_val = NULL;
back_to = make_cleanup (do_free_completion_list, &return_val);
+ completion_tracker = new_completion_tracker ();
+ limit_chain = make_cleanup_free_completion_tracker (completion_tracker);
+
datum.sym_text = sym_text;
datum.sym_text_len = sym_text_len;
datum.text = text;
@@ -4615,6 +4634,7 @@ default_make_symbol_completion_list_break_on (const char *text,
macro_for_each (macro_user_macros, add_macro_name, &datum);
}
+ do_cleanups (limit_chain);
discard_cleanups (back_to);
return (return_val);
}
@@ -67,6 +67,7 @@ if ![runto_main] then {
}
set timeout 30
+gdb_test_no_output "set max-completions unlimited"
gdb_test_no_output "complete print values\[0\].x." \
"field completion with invalid field"
@@ -747,4 +748,36 @@ gdb_test_multiple "" "$test" {
}
}
+#
+# Completion limiting.
+#
+
+gdb_test_no_output "set max-completions 5"
+
+set test "completion limiting using tab character"
+send_gdb "p\t"
+gdb_test_multiple "" "$test" {
+ -re "^p\\\x07$" {
+ send_gdb "\t"
+ gdb_test_multiple "" "$test" {
+ -re "Too many possibilities.\r\n\\\x07$gdb_prompt p$" {
+ send_gdb "\n"
+ gdb_test_multiple "" "$test" {
+ -re "$gdb_prompt $" {
+ pass "$test"
+ }
+ }
+ }
+ }
+ }
+}
+
+set test "completion limiting using complete command"
+send_gdb "complete p\n"
+gdb_test_multiple "" "$test" {
+ -re "Too many possibilities.\r\n$gdb_prompt $" {
+ pass "$test"
+ }
+}
+
return 0
@@ -26,6 +26,9 @@ if {[prepare_for_testing $testfile $exefile $srcfile \
# Turn off the pending breakpoint queries.
gdb_test_no_output "set breakpoint pending off"
+# Turn off completion limiting
+gdb_test_no_output "set max-completions unlimited"
+
# We intentionally do not use gdb_breakpoint for these tests.
# Break at 'linespec' and expect the message in ::error_messages indexed by