[RFA] new set/show multiple-choice-auto-select commands

[RFA] new set/show multiple-choice-auto-select commands

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]

Hello,

The user sometimes enters an expression which is ambiguous. For instance,
the name of a function or a variable which has homonyms... In that case,
the debugger is expected to display the list of possibilities and ask
the user to choose which one he meant (with an additional 2 possible
choices: "all" and "cancel").

This has been a very practical solution when in interactive mode
(ie when a user is driving the session), but an annoyance when driving
the debugger with a tool. So we added a new set/show command that
allowed the user to configure the behavior of the debugger so that:
  1. Either the menu is still displayed (default behavior);
  2. The choice "all" is always automatically selected;
  3. The choice "cancel" is always automatically selected.
Some of the AdaCore engineers actually disliked the menu and started
using this new command to always select all options, even in
interactive mode.

The syntax is as follow:

        (gdb) set multiple-choice-auto-select (off|all|cancel)
        (gdb) show multiple-choice-auto-select

As said above, the default is "off", which preserves the current
behavior by default.

The reason why I am proposing this as an RFA is that I'm wondering
if others might be interested in sharing this setting. I noticed
from a comment that Paul wrote that menus are also printed by
linespec:decode_line_2. Otherwise, I'm happy to keep it in ada-lang.c
(less work for me :-).

2008-01-01  Joel Brobecker  <brobecker@adacore.com>

        * ada-lang.c (auto_select_off, auto_select_all, auto_select_cancel)
        (auto_select_choices, auto_select_choice): New static global variables.
        (user_select_syms): Add handling of new multiple-choice-auto-select
        setting.
        (_initialize_ada_language): Add new multiple-choice-auto-select option.

Tested on x86-linux. No regression.

A testcase and documentation will follow as soon as we agree on
the patch itself.

Thoughts?

Thanks,
-- 
Joel

[-- Attachment #2: auto-select.diff --]
[-- Type: text/plain, Size: 2125 bytes --]

Index: ada-lang.c
===================================================================
--- ada-lang.c	(revision 32)
+++ ada-lang.c	(revision 33)
@@ -293,6 +293,21 @@ static void ada_forward_operator_length 
 /* Maximum-sized dynamic type.  */
 static unsigned int varsize_limit;
 
+/* Allow the user to configure the debugger behavior with respect
+   to multiple-choice menus.  */
+
+static const char auto_select_off[] = "off";
+static const char auto_select_all[] = "all";
+static const char auto_select_cancel[] = "cancel";
+static const char *auto_select_choices[] =
+{
+  auto_select_off,
+  auto_select_all,
+  auto_select_cancel,
+  NULL
+};
+static const char *auto_select_choice = auto_select_off;
+
 /* FIXME: brobecker/2003-09-17: No longer a const because it is
    returned by a function that does not return a const char *.  */
 static char *ada_completer_word_break_characters =
@@ -3341,6 +3356,15 @@ user_select_syms (struct ada_symbol_info
   if (nsyms <= 1)
     return nsyms;
 
+  if (auto_select_choice == auto_select_cancel
+      || (auto_select_choice == auto_select_all && max_results <= 1))
+    error (_("\
+canceled because the command is ambiguous and the multiple-choice menu\n\
+has been deactivated.  See set/show multiple-choice-auto-select."));
+  
+  if (auto_select_choice == auto_select_all)
+    return nsyms;
+
   printf_unfiltered (_("[0] cancel\n"));
   if (max_results > 1)
     printf_unfiltered (_("[1] all\n"));
@@ -10993,6 +11017,15 @@ _initialize_ada_language (void)
 
   varsize_limit = 65536;
 
+  add_setshow_enum_cmd ("multiple-choice-auto-select", no_class,
+                        auto_select_choices, &auto_select_choice,
+                        _("\
+Set the debugger behavior when part of a command is ambiguous and\n\
+a multiple-choice menu would normally be printed."), _("\
+Show how the debugger handles ambiguities in commands."), _("\
+Valid values are \"off\", \"all\", \"cancel\", and the default is \"off\"."),
+                        NULL, NULL, &setlist, &showlist);
+
   obstack_init (&symbol_list_obstack);
 
   decoded_names_store = htab_create_alloc

Re: [RFA] new set/show multiple-choice-auto-select commands

[Eli Zaretskii]

> Date: Tue, 1 Jan 2008 06:36:21 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> The user sometimes enters an expression which is ambiguous. For instance,
> the name of a function or a variable which has homonyms... In that case,
> the debugger is expected to display the list of possibilities and ask
> the user to choose which one he meant (with an additional 2 possible
> choices: "all" and "cancel").
> 
> This has been a very practical solution when in interactive mode
> (ie when a user is driving the session), but an annoyance when driving
> the debugger with a tool.

Please explain why, I'm not sure I understand.  When driving GDB with
a tool, the tool is responsible for doing TRT, so why is this a
nuisance?  And how does your solution alleviates that nuisance?

> So we added a new set/show command that
> allowed the user to configure the behavior of the debugger so that:
>   1. Either the menu is still displayed (default behavior);
>   2. The choice "all" is always automatically selected;
>   3. The choice "cancel" is always automatically selected.
> Some of the AdaCore engineers actually disliked the menu and started
> using this new command to always select all options, even in
> interactive mode.
> 
> The syntax is as follow:
> 
>         (gdb) set multiple-choice-auto-select (off|all|cancel)
>         (gdb) show multiple-choice-auto-select
> 
> As said above, the default is "off", which preserves the current
> behavior by default.

Can you please give an example of the behavior under each one of these
settings?  I'm afraid I cannot judge the suggestion without
understanding its effect on the users.

> +    error (_("\
> +canceled because the command is ambiguous and the multiple-choice menu\n\
> +has been deactivated.  See set/show multiple-choice-auto-select."));

Shouldn't "canceled" be capitalized?

> +  add_setshow_enum_cmd ("multiple-choice-auto-select", no_class,
> +                        auto_select_choices, &auto_select_choice,
> +                        _("\
> +Set the debugger behavior when part of a command is ambiguous and\n\
> +a multiple-choice menu would normally be printed."), _("\

The first line of the command's doc string should be a complete
sentence, or at least a complete phrase.  That's because some help
commands only display the first line (and stop at the first comma or
period in that line, IIRC).

Thanks.

Re: [RFA] new set/show multiple-choice-auto-select commands

[Joel Brobecker]

> > This has been a very practical solution when in interactive mode
> > (ie when a user is driving the session), but an annoyance when driving
> > the debugger with a tool.
> 
> Please explain why, I'm not sure I understand.  When driving GDB with
> a tool, the tool is responsible for doing TRT, so why is this a
> nuisance?  And how does your solution alleviates that nuisance?

The "tool" in this case was a GDB script, and the customer didn't
want to see the menus in the output. And also, some of the AdaCore
engineers were tired of seeing the menu when they were always selecting
the "all" choice. So it's not a matter of the tool doing TRT.

Note that "nuisance" is stronger than what I meant: It's annoying
*sometimes* to have to deal with the menu. Each individual has its
own preferences, and this option allows you to configure GDB accordingly.

> Can you please give an example of the behavior under each one of these
> settings?  I'm afraid I cannot judge the suggestion without
> understanding its effect on the users.

Sure. When the setting is "off", the behavior is unchanged, and when
the expression has multiple matches, the output will look like this:

    (gdb) b normal_menu
    [0] cancel
    [1] all
    [2] pck.normal_menu at pck.adb:6
    [3] pck.normal_menu at pck.adb:11
    >

At this point, if you select 0 (cancel):

    > 0
    cancelled
    (gdb)

If you select 1 (all):

    > 1
    Breakpoint 1 at 0x8049517: file pck.adb, line 6.
    Breakpoint 2 at 0x804951d: file pck.adb, line 11.
    warning: Multiple breakpoints were set.
    Use the "delete" command to delete unwanted breakpoints.

And if you select either 2 or 3 (one specific location):

    > 2
    Breakpoint 4 at 0x8049517: file pck.adb, line 6.

When the setting is "all", then it's the same as the user always
selecting "1" (all) everytime the menu is displayed, except that
the menu is no longer displayed and all locations are always
selected:

    (gdb) set multiple-choice-auto-select all

    (gdb) b normal_menu
    Breakpoint 5 at 0x8049517: file pck.adb, line 6.
    Breakpoint 6 at 0x804951d: file pck.adb, line 11.
    warning: Multiple breakpoints were set.
    Use the "delete" command to delete unwanted breakpoints.

And, if the setting is "cancel", it's the same as the user always
selecting "0" (cancel) everytime. Similarly, the menu is never
displayed, a message is printed explaining that the expression
is ambiguous and has been canceled.

   (gdb) b normal_menu
   canceled because the command is ambiguous and the multiple-choice menu
   has been deactivated.  See set/show multiple-choice-auto-select.

> > +    error (_("\
> > +canceled because the command is ambiguous and the multiple-choice menu\n\
> > +has been deactivated.  See set/show multiple-choice-auto-select."));
> 
> Shouldn't "canceled" be capitalized?

Yes, you're right.

> The first line of the command's doc string should be a complete
> sentence, or at least a complete phrase.  That's because some help
> commands only display the first line (and stop at the first comma or
> period in that line, IIRC).

We can fix that too.

-- 
Joel

Re: [RFA] new set/show multiple-choice-auto-select commands

[Eli Zaretskii]

> Date: Tue, 1 Jan 2008 20:34:49 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > Can you please give an example of the behavior under each one of these
> > settings?  I'm afraid I cannot judge the suggestion without
> > understanding its effect on the users.
> 
> Sure.

Thanks.  I'm in favor of adding this feature, and look forward to
reviewing the appropriate documentation changes when you post them.

Re: [RFA] new set/show multiple-choice-auto-select commands

[Markus Deuling]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

Joel Brobecker schrieb:

> 
>     (gdb) set multiple-choice-auto-select all
> 
>     (gdb) b normal_menu
>     Breakpoint 5 at 0x8049517: file pck.adb, line 6.
>     Breakpoint 6 at 0x804951d: file pck.adb, line 11.
>     warning: Multiple breakpoints were set.
>     Use the "delete" command to delete unwanted breakpoints.
> 

Hi Joel,

I noticed you're working on a multiple symbol (thats how I call it ;-) ), too. I'm currently also working
on s.th. very similar. In 2007 I added a patch which prefers a symbol definition in a shared lib GDB currently
stands in (if built with -Bsymbolic) instead of a symbol for example found in the main executable.

I attached a patch which introduces a new command "set symbol-user-choice on" which is off per default. If set to on a symbol lookup finding
>1 appropriate symbol (for example break foo, with foo in main and in a shared lib) a user choice (decode_line_2) is invoked.


Here is a sample session using the multiply defined symbol testcase from gdb.base/solib-symbol.exp:

(gdb) br foo
Function "foo" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (foo) pending.
(gdb) r
Starting program: /home/deuling/gdb/dev/build/gdb/testsuite/gdb.base/solib-symbol-main 
in main

Breakpoint 1, foo ()
    at /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-lib.c:22
22        printf ("foo in lib\n");
(gdb) set symbol-user-choice on
(gdb) br foo2
[0] cancel
[1] all
[2] foo2 at /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-main.c:39
[3] foo2 at /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-lib.c:29
> 2 3
Breakpoint 2 at 0x8048523: file /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-main.c, line 39.
Breakpoint 3 at 0xd03444: file /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-lib.c, line 29.
warning: Multiple breakpoints were set.
Use the "delete" command to delete unwanted breakpoints.
(gdb) 

So if >1 symbol is found the user can exactly choose which of these are meant.

Maybe we can merge the patches? I'd like to see the "set symbol-user-choice on|off" command rather in a language-undependent place
like linespec.c as in a language-specific file like ada. What do you think? Do you see the possibility for that?

Btw, I'd really appreciate your feedback on that patch. Would this be ok for mainline?

Regards,
Markus

-- 
  Markus Deuling
  GNU Toolchain for Linux on Cell BE
  deuling@de.ibm.com


[-- Attachment #2: diff-user-choice-symbols --]
[-- Type: text/plain, Size: 4415 bytes --]

diff -urpN src/gdb/linespec.c dev/gdb/linespec.c
--- src/gdb/linespec.c	2008-01-01 23:53:11.000000000 +0100
+++ dev/gdb/linespec.c	2008-01-08 15:33:48.000000000 +0100
@@ -36,6 +36,7 @@
 #include "linespec.h"
 #include "exceptions.h"
 #include "language.h"
+#include "gdbcmd.h"
 
 /* We share this one with symtab.c, but it is not exported widely. */
 
@@ -140,6 +141,16 @@ static struct
 symtabs_and_lines minsym_found (int funfirstline,
 				struct minimal_symbol *msymbol);
 
+static int symbol_user_choice = 0;
+
+static void
+show_symbol_user_choice (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("User choice for multiple symbols is %s.\n"),
+                    value);
+}
+
 /* Helper functions. */
 
 /* Issue a helpful hint on using the command completion feature on
@@ -1725,9 +1736,35 @@ decode_variable (char *copy, int funfirs
   struct symbol *sym;
   /* The symtab that SYM was found in.  */
   struct symtab *sym_symtab;
-
   struct minimal_symbol *msymbol;
 
+  if (symbol_user_choice && !file_symtab)
+    {
+      int nelts;
+      struct symbol_search *symbols, *p;
+      struct cleanup *chain;
+      struct symbol **sym_arr;
+
+      search_symbols (copy, FUNCTIONS_DOMAIN, 0, (char **) NULL, &symbols);
+      chain = make_cleanup_free_search_symbols (symbols);
+
+      nelts = count_symbols (symbols);
+      if (nelts)
+	{
+	  int idx = 0;
+	  sym_arr =  xmalloc ((nelts) * sizeof (struct symbol *));
+	  make_cleanup (xfree, sym_arr);
+
+	  for (p = symbols; p != NULL; p = p->next)
+	    if (p->symbol)
+	      sym_arr[idx++] = p->symbol;
+
+	  return decode_line_2 (sym_arr, idx, funfirstline,
+					 canonical); 
+	}
+    }
+
+
   sym = lookup_symbol (copy,
 		       (file_symtab
 			? BLOCKVECTOR_BLOCK (BLOCKVECTOR (file_symtab),
@@ -1856,3 +1893,18 @@ minsym_found (int funfirstline, struct m
   values.nelts = 1;
   return values;
 }
+
+
+void
+_initialize_linespec (void)
+{
+
+  /* Toggle wheter or not to let the user choose multiple defined symbols.  */
+  add_setshow_boolean_cmd ("symbol-user-choice", class_support,
+			  &symbol_user_choice,
+			  _("Set whether the user is to choose a symbol when multiple symbols are defined."),
+			  _("Show whether the user is to choose a symbol when multiple symbols are defined."),
+			  _("Use \"on\" to let the user choose a symbol when multiple symbols are defined. Use \"off\" to pick the first of those symbols by default."),
+			  NULL, show_symbol_user_choice, &setlist, &showlist);
+
+}
diff -urpN src/gdb/Makefile.in dev/gdb/Makefile.in
--- src/gdb/Makefile.in	2008-01-08 11:22:24.000000000 +0100
+++ dev/gdb/Makefile.in	2008-01-08 15:33:48.000000000 +0100
@@ -2331,7 +2331,7 @@ libunwind-frame.o: libunwind-frame.c $(d
 	$(gdb_string_h) $(libunwind_frame_h) $(complaints_h)
 linespec.o: linespec.c $(defs_h) $(symtab_h) $(frame_h) $(command_h) \
 	$(symfile_h) $(objfiles_h) $(source_h) $(demangle_h) $(value_h) \
-	$(completer_h) $(cp_abi_h) $(parser_defs_h) $(block_h) \
+	$(completer_h) $(cp_abi_h) $(parser_defs_h) $(block_h) $(gdbcmd_h)\
 	$(objc_lang_h) $(linespec_h) $(exceptions_h) $(language_h)
 linux-fork.o: linux-fork.c $(defs_h) $(inferior_h) $(regcache_h) $(gdbcmd_h) \
 	$(infcall_h) $(gdb_assert_h) $(gdb_string_h) $(linux_fork_h) \
diff -urpN src/gdb/symtab.c dev/gdb/symtab.c
--- src/gdb/symtab.c	2008-01-03 22:30:13.000000000 +0100
+++ dev/gdb/symtab.c	2008-01-08 15:33:48.000000000 +0100
@@ -2778,6 +2778,21 @@ file_matches (char *file, char *files[],
   return 0;
 }
 
+
+/* Returns number of elements in search reusult SYMBOLS.  */
+
+int
+count_symbols (struct symbol_search *symbols)
+{
+  struct symbol_search *p;
+  int nr = 0;
+
+  for (p = symbols; p != NULL; p = p->next)
+    nr++;
+
+  return nr;
+}
+
 /* Free any memory associated with a search. */
 void
 free_search_symbols (struct symbol_search *symbols)
diff -urpN src/gdb/symtab.h dev/gdb/symtab.h
--- src/gdb/symtab.h	2008-01-01 23:53:13.000000000 +0100
+++ dev/gdb/symtab.h	2008-01-08 15:33:48.000000000 +0100
@@ -1385,6 +1385,7 @@ struct symbol_search
 
 extern void search_symbols (char *, domain_enum, int, char **,
 			    struct symbol_search **);
+extern int count_symbols (struct symbol_search *);
 extern void free_search_symbols (struct symbol_search *);
 extern struct cleanup *make_cleanup_free_search_symbols (struct symbol_search
 							 *);

Re: [RFA] new set/show multiple-choice-auto-select commands

[Joel Brobecker]

Hi Markus,

> I attached a patch which introduces a new command "set symbol-user-choice 
> on" which is off per default. If set to on a symbol lookup finding
> >1 appropriate symbol (for example break foo, with foo in main and in a 
> >shared lib) a user choice (decode_line_2) is invoked.

The principle is indeed the same. The semantics of your command
are a little unclear to me, as you didn't say what should happen
if symbol-user-choice is off and you have more than one symbol matching.
Do you cancel the lookup, choose all symbols, and pick one at random?

We have one non-technical issue to solve if we are to merge the two
ideas into one setting: You suggest the default to be off, which means
no menu. But in Ada, we want the default to be "on" because we have
always printed these menus in Ada, and removing them now would be
a change of behavior.

> Maybe we can merge the patches? I'd like to see the "set
> symbol-user-choice on|off" command rather in a language-undependent
> place like linespec.c as in a language-specific file like ada. What do
> you think?  Do you see the possibility for that?

Absolutely. The reason why I posted this patch as a RFC is that
I realized that this feature could be used for all languages,
not just Ada. It's still on my plate to adjust the patch to put
this in linespec instead of ada-lang - I will get to it soon.

Would you be ok with using my command, however? I like the name better,
but most of all, it's a little more complete: It's a tri-state setting
as opposed to a binary one. We need to resolve the issue of the default.

> Btw, I'd really appreciate your feedback on that patch. Would this be ok 
> for mainline?

I'll admit that I only quickly scanned the patch, because I actually
prefer the command that I suggested...

-- 
Joel

Re: [RFA] new set/show multiple-choice-auto-select commands

[Markus Deuling]

Hi Joel,

Joel Brobecker schrieb:
> Hi Markus,

> The principle is indeed the same. The semantics of your command
> are a little unclear to me, as you didn't say what should happen
> if symbol-user-choice is off and you have more than one symbol matching.
> Do you cancel the lookup, choose all symbols, and pick one at random?

no, if it is set to off the default behaviour of GDB takes place. GDB executes
the "normal" symbol lookup routines as-is now.

If set to "on" and >= 1 symbol is found, decode_line_2 is called to let the user choose one ore more
symbols.

> 
> We have one non-technical issue to solve if we are to merge the two
> ideas into one setting: You suggest the default to be off, which means
> no menu. But in Ada, we want the default to be "on" because we have
> always printed these menus in Ada, and removing them now would be
> a change of behavior.

Hm, I guess it should be ok to have "on" as default. I could change the patch a bit so the
the user menu only gets invoked if > 1 symbol is found and the normal lookup routines if <= 1 symbol is found.
Then there would only be a choice if there are multiple symbols.

>> Maybe we can merge the patches? I'd like to see the "set
>> symbol-user-choice on|off" command rather in a language-undependent
>> place like linespec.c as in a language-specific file like ada. What do
>> you think?  Do you see the possibility for that?
> 
> Absolutely. The reason why I posted this patch as a RFC is that
> I realized that this feature could be used for all languages,
> not just Ada. It's still on my plate to adjust the patch to put
> this in linespec instead of ada-lang - I will get to it soon.
> 
> Would you be ok with using my command, however? I like the name better,
> but most of all, it's a little more complete: It's a tri-state setting
> as opposed to a binary one. We need to resolve the issue of the default.

Sure,  if you put your command in linespec.c and set default to "on" I'll rework the patch
to make use of the command and invoke the user menu if there are > 1 symbols found.

For me this would be a good way to go. What do you think ?

Regards,
Markus


-- 
  Markus Deuling
  GNU Toolchain for Linux on Cell BE
  deuling@de.ibm.com

Re: [RFA] new set/show multiple-choice-auto-select commands

[Joel Brobecker]

> >The principle is indeed the same. The semantics of your command
> >are a little unclear to me, as you didn't say what should happen
> >if symbol-user-choice is off and you have more than one symbol matching.
> >Do you cancel the lookup, choose all symbols, and pick one at random?
> 
> no, if it is set to off the default behaviour of GDB takes place. GDB
> executes the "normal" symbol lookup routines as-is now.

Actually, I was asking you to confirm what the current "normal" symbol
lookup does. AFAIK, it picks one symbol at random, which is a bug.
So my question still stands :), but the answer is not critical for
my part of the patch - we can talk about that later when you make
your own adjustments inside linespec.

> Hm, I guess it should be ok to have "on" as default. I could change
> the patch a bit so the the user menu only gets invoked if > 1 symbol
> is found and the normal lookup routines if <= 1 symbol is found.
> Then there would only be a choice if there are multiple symbols.

Absolutely. There is no point in printing a menu if there is only
one match :). You will notice that I am thinking purely in terms
of user interface at this point, I'm leaving the implementation
aspect out as an implementation detail.

> Sure,  if you put your command in linespec.c and set default to "on"
> I'll rework the patch to make use of the command and invoke the user
> menu if there are > 1 symbols found.

Deal. A new patch should be sent soon.

-- 
Joel

Re: [RFA] new set/show multiple-choice-auto-select commands

[Markus Deuling]

Joel Brobecker schrieb:
>>> The principle is indeed the same. The semantics of your command
>>> are a little unclear to me, as you didn't say what should happen
>>> if symbol-user-choice is off and you have more than one symbol matching.
>>> Do you cancel the lookup, choose all symbols, and pick one at random?
>> no, if it is set to off the default behaviour of GDB takes place. GDB
>> executes the "normal" symbol lookup routines as-is now.
> 
> Actually, I was asking you to confirm what the current "normal" symbol
> lookup does. AFAIK, it picks one symbol at random, which is a bug.
> So my question still stands :), but the answer is not critical for
> my part of the patch - we can talk about that later when you make
> your own adjustments inside linespec.

As far as I see there are two paths GDB can follow currently:

1. If GDB currently stands in a shared library which was built using 
-Bsymbolic it will pick the first symbol matching from exactly that library.

2. "default"
In all other cases (so without -Bsymbolc in a shared library) GDB's lookup starts in the
main executables binary. If the user for example stand in a shared library and invokes "br foo"
and there is a symbol in that library and in the main executable as well, GDB will pick the 
one in the meain executable. 
If the symbol is not found in the main executable GDB iterates over the shared libraries and picks 
the first symbol found.

> 
> Deal. A new patch should be sent soon.
> 

Great, standing by ... :-)

Regards,
Markus

-- 
  Markus Deuling
  GNU Toolchain for Linux on Cell BE
  deuling@de.ibm.com