[RFA] new set/show multiple-choice-auto-select commands (take 2)

[RFA] new set/show multiple-choice-auto-select commands (take 2)

[Joel Brobecker]

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

Hello,

This is a followup on:

    [RFA] new set/show multiple-choice-auto-select commands
    http://www.sourceware.org/ml/gdb-patches/2008-01/msg00007.html

I posted a patch that introduced this new option for Ada, but saying
that there was no reason to keep this option ada-specific.

Users sometimes enter expressions that are ambiguous. For instance,
printing the value of a variable that has homonyms.  In that case,
the debugger doesn't know which of the variables the user meant,
and needs to ask the user by displaying a multiple-choice menu.

This option allows the user to configure the behavior of the debugger
so that, either:

  1. The menu is still displayed as before (default behavior)
     and the debugger waits for the user to select the choice he wants;
  2. The menu is not displayed, and the choice "all" is assumed
     (or an error is triggered if the choice "all" is not available);
  3. The menu is not displayed and the choice "cancel" is always
     assumed (resulting in an error).

The syntax is as follow:

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

Markus will also be using this option for another case where
a multiple-choice menu might be useful.

2008-01-16  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.

Tested on x86-linux, no regression.
Documentation and a testcase will be send when this patch is approved.

OK to commit?

Thanks,
-- 
Joel

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

Index: symtab.c
===================================================================
--- symtab.c	(revision 131)
+++ symtab.c	(revision 132)
@@ -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_off;
+
+/* 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;
 
@@ -4418,6 +4442,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 131)
+++ symtab.h	(revision 132)
@@ -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 131)
+++ ada-lang.c	(revision 132)
@@ -3362,12 +3362,22 @@ 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
+      || (auto_select == 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 == auto_select_all)
+    return nsyms;
+
   printf_unfiltered (_("[0] cancel\n"));
   if (max_results > 1)
     printf_unfiltered (_("[1] all\n"));

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

[Markus Deuling]

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

Joel Brobecker schrieb:
> Hello,
> 
> This is a followup on:
> 
>     [RFA] new set/show multiple-choice-auto-select commands
>     http://www.sourceware.org/ml/gdb-patches/2008-01/msg00007.html
> 
> I posted a patch that introduced this new option for Ada, but saying
> that there was no reason to keep this option ada-specific.
> 
> Users sometimes enter expressions that are ambiguous. For instance,
> printing the value of a variable that has homonyms.  In that case,
> the debugger doesn't know which of the variables the user meant,
> and needs to ask the user by displaying a multiple-choice menu.
> 
> This option allows the user to configure the behavior of the debugger
> so that, either:
> 
>   1. The menu is still displayed as before (default behavior)
>      and the debugger waits for the user to select the choice he wants;
>   2. The menu is not displayed, and the choice "all" is assumed
>      (or an error is triggered if the choice "all" is not available);
>   3. The menu is not displayed and the choice "cancel" is always
>      assumed (resulting in an error).
> 
> The syntax is as follow:
> 
>         (gdb) set multiple-choice-auto-select (off|all|cancel)
>         (gdb) show multiple-choice-auto-select
> 
> Markus will also be using this option for another case where
> a multiple-choice menu might be useful.
> 
> 2008-01-16  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.
> 

Hi Joel,

I integrated my patch to use your command and it works fine ;-)

(gdb) br foo2
Breakpoint 2 at 0xf86444: file /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-lib.c, line 29.
(gdb) set multiple-choice-auto-select all
(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 3 at 0x8048523: file /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-main.c, line 39.
Note: breakpoint 2 also set at pc 0xf86444.
Breakpoint 4 at 0xf86444: 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) 

I only see one issue. The the commands state is either off or all (I now ignore cancel). If set to all, my routines
offer the user choice but doesn't choose all symbols right now !.

Would it be possible to add a "on" state to the command which then would give the user a choice. I would adapt my patch 
then a bit. If set to "all" automagically all symbols found are taken, if set to "on" the user is given a choice. What do you think?

I attached the adapted patch in the current version.

Regards,
Markus

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


[-- Attachment #2: new-diff-user-choice-symbols --]
[-- Type: text/plain, Size: 2287 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-17 07:19:54.000000000 +0100
@@ -1725,8 +1725,36 @@ decode_variable (char *copy, int funfirs
   struct symbol *sym;
   /* The symtab that SYM was found in.  */
   struct symtab *sym_symtab;
-
   struct minimal_symbol *msymbol;
+  const char *auto_select = multiple_choice_auto_select_mode ();
+
+
+  if (auto_select == auto_select_all && !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 > 1)
+	{
+	  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
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-17 07:22:07.000000000 +0100
@@ -2778,6 +2802,22 @@ 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-17 07:22:45.000000000 +0100
@@ -1385,6 +1391,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 (take 2)

[Joel Brobecker]

Hi Markus,

> (gdb) br foo2
> Breakpoint 2 at 0xf86444: file 
> /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-lib.c, line 29.
> (gdb) set multiple-choice-auto-select all
> (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 3 at 0x8048523: file 
> /home/deuling/gdb/dev/gdb/testsuite/gdb.base/solib-symbol-main.c, line 39.
> Note: breakpoint 2 also set at pc 0xf86444.
> Breakpoint 4 at 0xf86444: 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) 

It looks to me that you may be confused as to the semantics of
the new setting that I introduced. I want to say purely at the user
level for now, and worry about the implementation later.

Let me clarify the current semantics of multiple-choice-auto-select.
The idea is that, if multiple-choice-auto-select is turned on, then
some "auto-select" will happen. Otherwise, no auto-select, and thus
a menu needs to be displayed in order for the expression to be
disambiguated. Now, in terms of the command itself:

  1. set multiple-choice-auto-select off (default):
     Auto-select is turned off, so the multiple-choice menu is
     displayed, and the user must chose the ones he wants.

  2. set multiple-choice-auto-select all:
     In that case, the auto-select will automatically choose the "all"
     choice. If the "all" option is not available because you are expected
     to choose 1 and only 1 choice, then an error is triggered.

  3. set multiple-choice-auto-select cancel:
     In that case, the auto-select will automatically choose the "cancel"
     option of the multiple-choice menu.

Following this logic:
     
In your example above, after having set "multiple-choice-auto-select"
to all, your "break" command should not result in a menu being
displayed - instead, it should automatically break at both locations.

However, in the first case, when multiple-choice-auto-select was still
off, the menu should have been displayed, so that the user can choose
which one he wants.

Back to your particular case: The semantics that you are looking for
are:

  1. Multiple-choice menu activated: Let the user choose.

  2. Multiple-choice menu deactivated: Choose the symbol in the "current"
     shared library first.

I spent some time familiarizing myself with -Bsymbolic and the way
you introduced handling of these duplicated symbol names. Although
you and Daniel thought that it would be a good idea to treat these
types of symbols differently from other ambiguous expressions,
I don't quite understand why. But regardless of that fact, it seems
that the semantics of what you'd like to do is different from what
I am introducing, and that you should therefore have your own option.

What do you think?

-- 
Joel

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

[Markus Deuling]

Hi Joel,

Joel Brobecker schrieb:
> 
> Let me clarify the current semantics of multiple-choice-auto-select.
> The idea is that, if multiple-choice-auto-select is turned on, then
> some "auto-select" will happen. Otherwise, no auto-select, and thus
> a menu needs to be displayed in order for the expression to be
> disambiguated. Now, in terms of the command itself:
> 
>   1. set multiple-choice-auto-select off (default):
>      Auto-select is turned off, so the multiple-choice menu is
>      displayed, and the user must chose the ones he wants.
> 
>   2. set multiple-choice-auto-select all:
>      In that case, the auto-select will automatically choose the "all"
>      choice. If the "all" option is not available because you are expected
>      to choose 1 and only 1 choice, then an error is triggered.
> 
>   3. set multiple-choice-auto-select cancel:
>      In that case, the auto-select will automatically choose the "cancel"
>      option of the multiple-choice menu.
> 

I fully agree. For my patch it means:

1. set multiple-choice-auto-select off (default):
   If there is 0 || 1 symbol found, GDB follows the "normal" code path. If > 1 symbol is found
   the user choice is invoked

2. set multiple-choice-auto-select all:
   If there is > 1 symbol found all symbols found are automagically taken for e.g. breakpoints.

3. set multiple-choice-auto-select cancel:
   I think I'll ignore this. It makes no sense in that case. 

What do you think about it? Does it make sense?

The problem I saw and still see is the change of the default behaviour of GDB (and with that maybe the introduction
of a lot of FAILs in the teststuite). The user has no possibility to avoid the user choice if there are > 1 symbols.
For my understanding that would for example break MI in some cases, doesn't it? Maybe it has even more side-effects I don't see yet.
That was the reason for me to have that feature switched off by default. 

> I spent some time familiarizing myself with -Bsymbolic and the way
> you introduced handling of these duplicated symbol names. Although
> you and Daniel thought that it would be a good idea to treat these
> types of symbols differently from other ambiguous expressions,
> I don't quite understand why. But regardless of that fact, it seems
> that the semantics of what you'd like to do is different from what
> I am introducing, and that you should therefore have your own option.

I implemented this behavior originally for the Cell BE combined debugger. Cell BE has binaries consisting of
PowerPC- and SPU-code. If there is a symbol foo in both the PowerPC- and the SPU-part then GDB would always
resolve the foo symbol from PowerPC (main executable) file no matter if it stands in a "SPU thread" or not. 
GDB recognizes SPU binaries as shared libraries. 

ELF standard (v1.2) says about DT_SYMBOLIC (inserted by -Bsymbolic):

"This elementÂ’s presence in a shared object library alters the dynamic linkerÂ’s symbol
resolution algorithm for references within the library. Instead of starting a symbol
search with the executable file, the dynamic linker starts from the shared object itself. If
the shared object fails to supply the referenced symbol, the dynamic linker then
searches the executable file and other shared objects as usual."

So there are at least two types of shared libs showing the same lookup behaviour. This was the reason to implement
the combined debugger functionality using a new common framework (solib callbacks) and at the same time also implement
it for ELF libs. 


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 (take 2)

[Joel Brobecker]

> 1. set multiple-choice-auto-select off (default):
>   If there is 0 || 1 symbol found, GDB follows the "normal" code path. If > 
>   1 symbol is found
>   the user choice is invoked
> 
> 2. set multiple-choice-auto-select all:
>   If there is > 1 symbol found all symbols found are automagically taken 
>   for e.g. breakpoints.
> 
> 3. set multiple-choice-auto-select cancel:
>   I think I'll ignore this. It makes no sense in that case. 
> 
> What do you think about it? Does it make sense?

So far, so good. But:

> The problem I saw and still see is the change of the default behaviour
> of GDB (and with that maybe the introduction of a lot of FAILs in the
> teststuite). The user has no possibility to avoid the user choice if
> there are > 1 symbols.

Right - that's the hole with using this setting. Thanks to your
explanations, I think that one way to cover that hole would be to
add an extra option like you suggested, and call it something like:
within-shared-library or somesuch. I think it's more informative
and also fits better in the concept introduced by the new "set/show"
command. In that case, the default can be set "within-shared-library"
and your current behavior would be preserved without change the
default behavior in Ada.

I need to look at how your feature is implemented. I wonder how well
this is supported when the language is Ada...  As a first step, Ada
can treat this option as "off" (display menu and ask user).

-- 
Joel

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

[Daniel Jacobowitz]

On Thu, Jan 17, 2008 at 12:38:51PM +0100, Markus Deuling wrote:
> I implemented this behavior originally for the Cell BE combined
> debugger. Cell BE has binaries consisting of PowerPC- and
> SPU-code. If there is a symbol foo in both the PowerPC- and the
> SPU-part then GDB would always resolve the foo symbol from PowerPC
> (main executable) file no matter if it stands in a "SPU thread" or
> not. GDB recognizes SPU binaries as shared libraries.

Why shouldn't GDB default to setting both breakpoints... like we do
for template functions now?  At least, I thought that was the point of
all Vlad's work on breakpoints in the past year.  If you want
something more specific you can adjust the multiple-location
breakpoint.

I am concerned that we now have too many different places where we
have multiple symbols and breakpoints and we're handling them all
differently.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Joel Brobecker]

> Why shouldn't GDB default to setting both breakpoints... like we do
> for template functions now?

That's the part I didn't understand. But since this was agreed on
earlier, I accepted it.

Note that, in Ada, we have made a different choice for the default
behavior: Instead of breaking everywhere by default, we ask the user
by default. This is something we have been doing at least since I joined
AdaCore 7 years ago, so I'd be reluctant to change the default, as it
might surprise Ada users using the FSF debugger. I am not strongly
opposed to the change, but I know I am going to have a very very hard
time convincing AdaCore of this change of behavior and we will likely
end up with different behaviors between the FSF GDB and AdaCore's.
Not that big a deal, but might be disruptive to Ada users who use both
(I know we have some customers who also use Ada at home and thus use
both).

> I am concerned that we now have too many different places where we
> have multiple symbols and breakpoints and we're handling them all
> differently.

I agree. I know that I submitted a patch that does not improve this
situation (addition of ada_finish_decode_line_1 I think), but I am very
motivated in making it better. This is really one of my major goals
for 2008. I will do that gradually, but in the meantime, I thought
that it would still be helpful addition to the FSF tree, as it provides
a feature that is critical in Ada, IMO.

I hope you don't object to the addition of this new set/show setting
whose purpose, I believe, is really orthogonal to the issue that
Markus is trying to address.

-- 
Joel

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

[Daniel Jacobowitz]

On Thu, Jan 17, 2008 at 06:50:52AM -0800, Joel Brobecker wrote:
> I hope you don't object to the addition of this new set/show setting
> whose purpose, I believe, is really orthogonal to the issue that
> Markus is trying to address.

Well, it institutionalizes the Ada behavior as different from the
direction we've taken for C++.  But it doesn't make things any worse,
so I do not object.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Joel Brobecker]

> > I hope you don't object to the addition of this new set/show setting
> > whose purpose, I believe, is really orthogonal to the issue that
> > Markus is trying to address.
> 
> Well, it institutionalizes the Ada behavior as different from the
> direction we've taken for C++.  But it doesn't make things any worse,
> so I do not object.

After thinking about it some more, I thought that it didn't make sense
for the behavior to be language-dependent. Another argument in favor
of making it consistent is that, long term, I want the code to be
merged, and having consistent behavior will of course help. So we
will have Ada be consistent with C/C++, and use "all" as the default
value.  This is a change of behavior for Ada users, but we all thought
that this is usually what the user wants, so it should be for the better.

This patch is withdrawn again, and a new one will be sent later.

-- 
Joel