[RFA] new set/show multiple-choice-auto-select command (take 3)

[RFA] new set/show multiple-choice-auto-select command (take 3)

[Joel Brobecker]

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

Hello,

This is my third take on introducing this new set/show command.
The change since the last time is that the default had been changed
to "all". This is not what used to be the default behavior in Ada,
but it better matches the direction taken for C/C++, so it's all
consistent across languages (and will make it easier later, when
we manage to merge the ada code with the core code).

So, to summarize, this patch introduces a new set/show command:

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

The idea is that some expressions are ambiguous, and that the debugger
normally has several options: Select all matching symbols, ask the user
to select one or more of them, or error out.  This new option allows
the user to configure how the debugger should behave in that situation:

  - "all" (default): auto-select all matching symbols.
  - "off": Auto-selection is off, so display a menu with all possible
           choices and ask the user to choose one or more of them.
  - "cancel": Abort the command explaining why.

After the new setting was introduced, I modified ada-lang.c and
linespec.c to take it into account. That resulted in some behavior
changes which I think I desirable and consistent with we have done
recently with multiple-location breakpoints. In order to preserve
the current testcases, I simply forced multiple-choice-auto-select
to "off" to reproduce the previous behavior.

I also added some testing of the cases when multiple-choice-auto-select
is set to "cancel" and "all". I still haven't written an Ada testcase
but that's very easy. I'll do that next, together with the documentation
if this patch is approved.

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

        * symtab.c (auto_select_off, auto_select_all, auto_select_cancel):
        New constants.
        (auto_select_modes, auto_select_mode): New static globals.
        (multiple_choice_auto_select_mode): New function.
        (_initialize_symtab): Add new multiple-choice-auto-select command.
        * symtab.h (auto_select_off, auto_select_all, auto_select_cancel)
        (multiple_choice_auto_select_mode): Add declarations.
        * ada-lang.c (user_select_syms): Add handling of new
        multiple-choice-auto-select setting.
        * linespec.c (decode_line_2): Likewise.

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

        * gdb.cp/ovldbreak.cc: Add missing bodies for methods foo::foofunc.
        * gdb.cp/ovldbreak.exp: Set multiple-choice-auto-select to "off"
        as this is the value that this test was assuming when it was
        written, before this setting got introduced.
        Add a couple of tests that verify the behavior when the new setting
        is "cancel" and "all".
        * gdb.cp/method2.exp, gdb.cp/templates.exp: Set
        multiple-choice- auto-select to "off" before we start the testing.

Tested on x86-linux.
OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: mcas.diff --]
[-- Type: text/plain, Size: 6600 bytes --]

Index: symtab.c
===================================================================
--- symtab.c	(revision 141)
+++ symtab.c	(revision 142)
@@ -124,6 +124,30 @@ void _initialize_symtab (void);
 
 /* */
 
+/* Allow the user to configure the debugger behavior with respect
+   to multiple-choice menus when more than one symbol matches during
+   a symbol lookup.  */
+
+const char const auto_select_off[] = "off";
+const char const auto_select_all[] = "all";
+const char const auto_select_cancel[] = "cancel";
+static const char *auto_select_modes[] =
+{
+  auto_select_off,
+  auto_select_all,
+  auto_select_cancel,
+  NULL
+};
+static const char *auto_select_mode = auto_select_all;
+
+/* Read-only accessor to AUTO_SELECT_MODE.  */
+
+const char *
+multiple_choice_auto_select_mode (void)
+{
+  return auto_select_mode;
+}
+
 /* The single non-language-specific builtin type */
 struct type *builtin_type_error;
 
@@ -4417,6 +4441,15 @@ All global and static variable names, or
 All global and static variable names, or those matching REGEXP."));
     }
 
+  add_setshow_enum_cmd ("multiple-choice-auto-select", no_class,
+                        auto_select_modes, &auto_select_mode,
+                        _("\
+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);
+
   /* Initialize the one built-in type that isn't language dependent... */
   builtin_type_error = init_type (TYPE_CODE_ERROR, 0, 0,
 				  "<unknown type>", (struct objfile *) NULL);
Index: symtab.h
===================================================================
--- symtab.h	(revision 141)
+++ symtab.h	(revision 142)
@@ -1005,6 +1005,12 @@ extern int asm_demangle;
 
 /* symtab.c lookup functions */
 
+extern const char const auto_select_off[];
+extern const char const auto_select_all[];
+extern const char const auto_select_cancel[];
+
+const char *multiple_choice_auto_select_mode (void);
+
 /* lookup a symbol table by source file name */
 
 extern struct symtab *lookup_symtab (const char *);
Index: ada-lang.c
===================================================================
--- ada-lang.c	(revision 141)
+++ ada-lang.c	(revision 142)
@@ -3362,12 +3362,24 @@ user_select_syms (struct ada_symbol_info
   int *chosen = (int *) alloca (sizeof (int) * nsyms);
   int n_chosen;
   int first_choice = (max_results == 1) ? 1 : 2;
+  const char *auto_select = multiple_choice_auto_select_mode ();
 
   if (max_results < 1)
     error (_("Request to select 0 symbols!"));
   if (nsyms <= 1)
     return nsyms;
 
+  if (auto_select == auto_select_cancel)
+    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_mode is "all", then return all possible symbols.
+     Only do that if more than one symbol can be selected, of course.
+     Otherwise, display the menu as usual.  */
+  if (auto_select == auto_select_all && max_results > 1)
+    return nsyms;
+
   printf_unfiltered (_("[0] cancel\n"));
   if (max_results > 1)
     printf_unfiltered (_("[1] all\n"));
Index: linespec.c
===================================================================
--- linespec.c	(revision 141)
+++ linespec.c	(revision 142)
@@ -492,7 +492,13 @@ decode_line_2 (struct symbol *sym_arr[],
   char *symname;
   struct cleanup *old_chain;
   char **canonical_arr = (char **) NULL;
+  const char *auto_select = multiple_choice_auto_select_mode ();
 
+  if (auto_select == auto_select_cancel)
+    error (_("\
+canceled because the command is ambiguous and the multiple-choice menu\n\
+has been deactivated.  See set/show multiple-choice-auto-select."));
+  
   values.sals = (struct symtab_and_line *)
     alloca (nelts * sizeof (struct symtab_and_line));
   return_values.sals = (struct symtab_and_line *)
@@ -508,38 +514,52 @@ decode_line_2 (struct symbol *sym_arr[],
     }
 
   i = 0;
-  printf_unfiltered (_("[0] cancel\n[1] all\n"));
   while (i < nelts)
     {
       init_sal (&return_values.sals[i]);	/* Initialize to zeroes.  */
       init_sal (&values.sals[i]);
       if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
-	{
-	  values.sals[i] = find_function_start_sal (sym_arr[i], funfirstline);
-	  if (values.sals[i].symtab)
-	    printf_unfiltered ("[%d] %s at %s:%d\n",
-			       (i + 2),
-			       SYMBOL_PRINT_NAME (sym_arr[i]),
-			       values.sals[i].symtab->filename,
-			       values.sals[i].line);
-	  else
-	    printf_unfiltered (_("[%d] %s at ?FILE:%d [No symtab? Probably broken debug info...]\n"),
-			       (i + 2),
-			       SYMBOL_PRINT_NAME (sym_arr[i]),
-			       values.sals[i].line);
-
-	}
-      else
-	printf_unfiltered (_("?HERE\n"));
+	values.sals[i] = find_function_start_sal (sym_arr[i], funfirstline);
       i++;
     }
 
-  prompt = getenv ("PS2");
-  if (prompt == NULL)
+  /* If auto_select_mode is "all", then do not print the multiple-choice
+     menu and act as if the user had chosen choice "1" (all).  */
+  if (auto_select == auto_select_all)
+    args = "1";
+  else
     {
-      prompt = "> ";
+      i = 0;
+      printf_unfiltered (_("[0] cancel\n[1] all\n"));
+      while (i < nelts)
+        {
+          if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
+            {
+              if (values.sals[i].symtab)
+                printf_unfiltered ("[%d] %s at %s:%d\n",
+                                   (i + 2),
+                                   SYMBOL_PRINT_NAME (sym_arr[i]),
+                                   values.sals[i].symtab->filename,
+                                   values.sals[i].line);
+              else
+                printf_unfiltered (_("[%d] %s at ?FILE:%d [No symtab? Probably broken debug info...]\n"),
+                                   (i + 2),
+                                   SYMBOL_PRINT_NAME (sym_arr[i]),
+                                   values.sals[i].line);
+
+            }
+          else
+            printf_unfiltered (_("?HERE\n"));
+          i++;
+        }
+
+      prompt = getenv ("PS2");
+      if (prompt == NULL)
+        {
+          prompt = "> ";
+        }
+      args = command_line_input (prompt, 0, "overload-choice");
     }
-  args = command_line_input (prompt, 0, "overload-choice");
 
   if (args == 0 || *args == 0)
     error_no_arg (_("one or more choice numbers"));

[-- Attachment #3: mcas-tc.diff --]
[-- Type: text/plain, Size: 3809 bytes --]

Index: gdb.cp/ovldbreak.exp
===================================================================
--- gdb.cp/ovldbreak.exp	(revision 139)
+++ gdb.cp/ovldbreak.exp	(revision 140)
@@ -131,7 +131,9 @@ proc set_bp_overloaded {name expectedmen
 
 set menu_overload1arg "\\\[0\\\] cancel\r\n\\\[1\\\] all\r\n\\\[2\\\] foo::overload1arg\\(double\\) at.*$srcfile:121\r\n\\\[3\\\] foo::overload1arg\\(float\\) at.*$srcfile:120\r\n\\\[4\\\] foo::overload1arg\\(unsigned long\\) at.*$srcfile:119\r\n\\\[5\\\] foo::overload1arg\\(long\\) at.*$srcfile:118\r\n\\\[6\\\] foo::overload1arg\\((unsigned int|unsigned)\\) at.*$srcfile:117\r\n\\\[7\\\] foo::overload1arg\\(int\\) at.*$srcfile:116\r\n\\\[8\\\] foo::overload1arg\\(unsigned short\\) at.*$srcfile:115\r\n\\\[9\\\] foo::overload1arg\\(short\\) at.*$srcfile:114\r\n\\\[10\\\] foo::overload1arg\\(unsigned char\\) at.*$srcfile:113\r\n\\\[11\\\] foo::overload1arg\\(signed char\\) at.*$srcfile:112\r\n\\\[12\\\] foo::overload1arg\\(char\\) at.*$srcfile:111\r\n\\\[13\\\] foo::overload1arg\\((void|)\\) at.*$srcfile:110\r\n> $"
 
-
+# Set multiple-choice-auto-select to "off", to allow us to test the use
+# of the multiple-choice menu when breaking on an overloaded method.
+gdb_test "set multiple-choice-auto-select off" ""
 
 # Set breakpoints on foo::overload1arg, one by one.
 
@@ -350,7 +352,18 @@ continue_to_bp_overloaded 0 16 "unsigned
 continue_to_bp_overloaded 0 15 "float" "arg=100"
 continue_to_bp_overloaded 1 14 "double" "arg=200"
 
-
+# Test breaking on an overloaded function when multiple-choice-auto-select
+# is set to "cancel"
+gdb_test "set multiple-choice-auto-select cancel" ""
+gdb_test "break foo::foofunc" \
+         "canceled.*"
+
+# Test breaking on an overloaded function when multiple-choice-auto-select
+# is set to "all"
+gdb_test "set multiple-choice-auto-select all" ""
+gdb_test "break foo::foofunc" \
+         "Breakpoint \[0-9\]+ at ${hex}: file .*ovldbreak\\.cc, line \[0-9\]+\\.\r\nBreakpoint \[0-9\]+ at ${hex}: file .*ovldbreak\\.cc, line \[0-9\]+\\.\r\nwarning: Multiple breakpoints were set\\.\r\nUse the \"delete\" command to delete unwanted breakpoints\\."
+         #"Breakpoint \[0-9\]+ at ${hex}: file .*ovldbreak\\.cc, line \[0-9\]+\r\nBreakpoint \[0-9\]+ at ${hex}: file .*ovldbreak\\.cc, line \[0-9\]+\r\nwarning: Multiple breakpoints were set\\.Use the \"delete\" command to delete unwanted breakpoints\\."
 
 # That's all, folks.
 
Index: gdb.cp/ovldbreak.cc
===================================================================
--- gdb.cp/ovldbreak.cc	(revision 139)
+++ gdb.cp/ovldbreak.cc	(revision 140)
@@ -174,4 +174,11 @@ int foo::overloadargs (int a1, int a2, i
                           a10 = a11 = 0; return 11;}
 
 
+void foo::foofunc (int a)
+{
+}
+
+void foo::foofunc (int b, signed char *c)
+{
+}
 
Index: gdb.cp/method2.exp
===================================================================
--- gdb.cp/method2.exp	(revision 139)
+++ gdb.cp/method2.exp	(revision 140)
@@ -63,6 +63,10 @@ proc test_break { lang } {
     }
 }
 
+# We want in this test to double-check the contents of the multiple-choice
+# menu, so we have to turn the multiple-choice-auto-select feature off first.
+gdb_test "set multiple-choice-auto-select off" ""
+
 test_break "c"
 test_break "c++"
 
Index: gdb.cp/templates.exp
===================================================================
--- gdb.cp/templates.exp	(revision 139)
+++ gdb.cp/templates.exp	(revision 140)
@@ -210,6 +210,10 @@ proc do_tests {} {
     gdb_reinitialize_dir $srcdir/$subdir
     gdb_load $binfile
 
+    # Change the multiple-choice-auto-select value to "off" in order to
+    # get the multiple-choice menu when breaking on overloaded methods.
+    gdb_test "set multiple-choice-auto-select off" ""
+
     runto_main
 
     test_ptype_of_templates

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Daniel Jacobowitz]

On Sat, Feb 02, 2008 at 08:37:55PM -0800, Joel Brobecker wrote:
> So, to summarize, this patch introduces a new set/show command:
> 
>         (gdb) set multiple-choice-auto-select (off|all|cancel)
>         (gdb) show multiple-choice-auto-select

Should it be "multiple-symbol-auto-select"?  I can easily imagine
there being other kinds of multiple choice questions.

(Shorter version, if you like it better: set multiple-symbols
ask|all|cancel)

I think this is going to need a NEWS entry; I don't remember if you've
already got that covered.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Joel Brobecker]

> (Shorter version, if you like it better: set multiple-symbols
> ask|all|cancel)

You are my hero! I'll rework the patch with your suggested name soon.
I can cover the NEWS entry with the documentation patch so that Eli
has only one patch to review.

Thanks,
-- 
Joel

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Eli Zaretskii]

> Date: Sat, 2 Feb 2008 21:06:19 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> I can cover the NEWS entry with the documentation patch so that Eli
> has only one patch to review.

Thanks.

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Eli Zaretskii]

> Date: Sat, 2 Feb 2008 20:37:55 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> 
> The change since the last time is that the default had been changed
> to "all".

Is this the current behavior, or are we changing the behavior here?

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Joel Brobecker]

> > The change since the last time is that the default had been changed
> > to "all".
> 
> Is this the current behavior, or are we changing the behavior here?

It's kind of both :). If you remember some work has been done recently
to handle breakpoints with multiple locations. For instance, breaking
on a constructor results in at least 2 locations for the same
breakpoint. In that case, the behavior was the equivalent of having
the new setting set to "all", so we're not changing anything here.

But when dealing with homonyms (for instance, breaking on overloaded
methods), the standard behavior was to ask the user to select the ones
he wanted to break on. This patch makes things more consistent by
changing the behavior. Now, the debugger will break on all matches
by default - but the old behavior can be of course restored using
"ask".

Similarly in Ada, we used to ask the user by default. But we decided
to change this because we think that breaking on all matches is what
the user wants most of the time, and also to make it consistent with
the other languages.

-- 
Joel

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Eli Zaretskii]

> Date: Sun, 3 Feb 2008 12:40:00 -0800
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > > The change since the last time is that the default had been changed
> > > to "all".
> > 
> > Is this the current behavior, or are we changing the behavior here?
> 
> It's kind of both :). If you remember some work has been done recently
> to handle breakpoints with multiple locations. For instance, breaking
> on a constructor results in at least 2 locations for the same
> breakpoint. In that case, the behavior was the equivalent of having
> the new setting set to "all", so we're not changing anything here.
> 
> But when dealing with homonyms (for instance, breaking on overloaded
> methods), the standard behavior was to ask the user to select the ones
> he wanted to break on. This patch makes things more consistent by
> changing the behavior.

I'm uneasy about changing the default behavior, but perhaps this is
okay if breaking on overloaded methods was introduced not too long
ago.  Is that the case?

In any case, the NEWS entry should mention this change in the default.

Re: [RFA] new set/show multiple-choice-auto-select command (take 3)

[Joel Brobecker]

> I'm uneasy about changing the default behavior, but perhaps this is
> okay if breaking on overloaded methods was introduced not too long
> ago.  Is that the case?

I don't think so (that it was he case). However, I think the new behavior
is more useful in practice (from our experience in Ada at least), and
the old behavior can be restored if some users are really attached to it.
We actually made that change in behavior in the AdaCore version, and no
complaints so far. Given all that, I think the change is for the better.
The multiple-location breakpoint work already set the debugger in that
direction, and this patch is another logical step towards it.

I will make sure that the NEWS entry makes it clear that there is
a change of behavior.

-- 
Joel