[RFC 0/6] MI notification of command option change

[RFC 0/6] MI notification of command option change

[Yao Qi]

When Eclipse connects to GDB through MI, user is still abel to
type command in console (or telnet session), so the state of GDB
may be changed while Eclipse is not aware of.  GDB is lack of
notifications to MI front end and/or telnet session when the
internal state of GDB is changed (eg. changed by some 'set foo bar'
commands or tracepoint is downloaded).

This patch series is to address the problem that caused by
command 'set foo bar'.  When user types some interesting commands
in console, GDB will send MI notifications about the command
option changes.  The following patch series will address the
problem caused by other aspects in the future.  In this way,
Eclipse is aware of the change of GDB state, and refresh or
update itself accordingly.

Patch 1/6 - 3/6 are about the new observer 'command_option_changed'
and new MI notification '=option-changed' when observer is notified.
Patch 4/6 - 6/6 are about registering three types of command
respectively.  Three commands will send MI notifications if their
option is changed.  There are more than three commands that can
change the state of GDB, but these three are reprsentives of
them.  The rest of commands can be easily changed to sending
MI notifications.

Regression tested on x86_64 native/gdbserver.

[PATCH 2/6] allow to suppress more mi notification

[Yao Qi]

Hi,
'mi_suppress_breakpoint_notifications' is used to suppress mi
breakpoint notifications.  This patch is to change this variable
to an array so that multiple mi notifications can be suppressed.
The following patch will use it to suppress other MI notifications.

gdb:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* mi/mi-interp.c: Remove mi_suppress_breakpoint_notifications.
	Define array mi_suppress_notification.
	(mi_breakpoint_created): Update.
	(mi_breakpoint_deleted): Likewise.
	(mi_breakpoint_modified): Likewise.
	* mi/mi-main.c (mi_cmd_execute): Likewise.
	* mi/mi-main.h (enum MI_SUPRESS_NOTIFICATION): New.
---
 gdb/mi/mi-interp.c |   12 ++++++------
 gdb/mi/mi-main.c   |    6 ++++--
 gdb/mi/mi-main.h   |    4 +++-
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index b487136..885d08b 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -501,10 +501,10 @@ mi_about_to_proceed (void)
   mi_proceeded = 1;
 }
 
-/* When non-zero, no MI notifications will be emitted in
-   response to breakpoint change observers.  */
+/* When the element is non-zero, no MI notifications will be emitted in
+   response to the corresponding observers.  */
 
-int mi_suppress_breakpoint_notifications = 0;
+int mi_suppress_notification[] = { 0 };
 
 /* Emit notification about a created breakpoint.  */
 
@@ -515,7 +515,7 @@ mi_breakpoint_created (struct breakpoint *b)
   struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
   volatile struct gdb_exception e;
 
-  if (mi_suppress_breakpoint_notifications)
+  if (mi_suppress_notification[MI_SUPPRESS_BREAKPOINT])
     return;
 
   if (b->number <= 0)
@@ -546,7 +546,7 @@ mi_breakpoint_deleted (struct breakpoint *b)
 {
   struct mi_interp *mi = top_level_interpreter_data ();
 
-  if (mi_suppress_breakpoint_notifications)
+  if (mi_suppress_notification[MI_SUPPRESS_BREAKPOINT])
     return;
 
   if (b->number <= 0)
@@ -569,7 +569,7 @@ mi_breakpoint_modified (struct breakpoint *b)
   struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
   volatile struct gdb_exception e;
 
-  if (mi_suppress_breakpoint_notifications)
+  if (mi_suppress_notification[MI_SUPPRESS_BREAKPOINT])
     return;
 
   if (b->number <= 0)
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index dfb4892..0ae867d 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2099,8 +2099,10 @@ mi_cmd_execute (struct mi_parse *parse)
 
   if (strncmp (parse->command, "break-", sizeof ("break-") - 1 ) == 0)
     {
-      make_cleanup_restore_integer (&mi_suppress_breakpoint_notifications);
-      mi_suppress_breakpoint_notifications = 1;
+      int *p = &mi_suppress_notification[MI_SUPPRESS_BREAKPOINT];
+
+      make_cleanup_restore_integer (p);
+      *p = 1;
     }
 
   if (parse->cmd->argv_func != NULL)
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index beac2cd..1c7bf00 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -32,7 +32,9 @@ extern char *current_token;
 
 extern int running_result_record_printed;
 extern int mi_proceeded;
-extern int mi_suppress_breakpoint_notifications;
+
+enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT };
+extern int mi_suppress_notification[];
 
 #endif
 
-- 
1.7.7.6

Re: [PATCH 2/6] allow to suppress more mi notification

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-07-24  Yao Qi  <yao@codesourcery.com>
Yao> 	* mi/mi-interp.c: Remove mi_suppress_breakpoint_notifications.
Yao> 	Define array mi_suppress_notification.
Yao> 	(mi_breakpoint_created): Update.
Yao> 	(mi_breakpoint_deleted): Likewise.
Yao> 	(mi_breakpoint_modified): Likewise.
Yao> 	* mi/mi-main.c (mi_cmd_execute): Likewise.
Yao> 	* mi/mi-main.h (enum MI_SUPRESS_NOTIFICATION): New.

Looks pretty good.  One nit...

Yao> +enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT };

It is odd to use all caps for an enum tag name.

Ok with this changed.

Tom

Re: [PATCH 2/6] allow to suppress more mi notification

[Pedro Alves]

On 07/24/2012 05:10 PM, Yao Qi wrote:
> Hi,
> 'mi_suppress_breakpoint_notifications' is used to suppress mi
> breakpoint notifications.  This patch is to change this variable
> to an array so that multiple mi notifications can be suppressed.
> The following patch will use it to suppress other MI notifications.
> 
> gdb:
> 
> 2012-07-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* mi/mi-interp.c: Remove mi_suppress_breakpoint_notifications.
> 	Define array mi_suppress_notification.
> 	(mi_breakpoint_created): Update.
> 	(mi_breakpoint_deleted): Likewise.
> 	(mi_breakpoint_modified): Likewise.
> 	* mi/mi-main.c (mi_cmd_execute): Likewise.
> 	* mi/mi-main.h (enum MI_SUPRESS_NOTIFICATION): New.
> ---
>  gdb/mi/mi-interp.c |   12 ++++++------
>  gdb/mi/mi-main.c   |    6 ++++--
>  gdb/mi/mi-main.h   |    4 +++-
>  3 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index b487136..885d08b 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -501,10 +501,10 @@ mi_about_to_proceed (void)
>    mi_proceeded = 1;
>  }
>  
> -/* When non-zero, no MI notifications will be emitted in
> -   response to breakpoint change observers.  */
> +/* When the element is non-zero, no MI notifications will be emitted in
> +   response to the corresponding observers.  */
>  
> -int mi_suppress_breakpoint_notifications = 0;
> +int mi_suppress_notification[] = { 0 };
>  
>  /* Emit notification about a created breakpoint.  */
>  
> @@ -515,7 +515,7 @@ mi_breakpoint_created (struct breakpoint *b)
>    struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
>    volatile struct gdb_exception e;
>  
> -  if (mi_suppress_breakpoint_notifications)
> +  if (mi_suppress_notification[MI_SUPPRESS_BREAKPOINT])
>      return;
>  
>    if (b->number <= 0)
> @@ -546,7 +546,7 @@ mi_breakpoint_deleted (struct breakpoint *b)
>  {
>    struct mi_interp *mi = top_level_interpreter_data ();
>  
> -  if (mi_suppress_breakpoint_notifications)
> +  if (mi_suppress_notification[MI_SUPPRESS_BREAKPOINT])
>      return;
>  
>    if (b->number <= 0)
> @@ -569,7 +569,7 @@ mi_breakpoint_modified (struct breakpoint *b)
>    struct ui_out *mi_uiout = interp_ui_out (top_level_interpreter ());
>    volatile struct gdb_exception e;
>  
> -  if (mi_suppress_breakpoint_notifications)
> +  if (mi_suppress_notification[MI_SUPPRESS_BREAKPOINT])
>      return;
>  
>    if (b->number <= 0)
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index dfb4892..0ae867d 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -2099,8 +2099,10 @@ mi_cmd_execute (struct mi_parse *parse)
>  
>    if (strncmp (parse->command, "break-", sizeof ("break-") - 1 ) == 0)
>      {
> -      make_cleanup_restore_integer (&mi_suppress_breakpoint_notifications);
> -      mi_suppress_breakpoint_notifications = 1;
> +      int *p = &mi_suppress_notification[MI_SUPPRESS_BREAKPOINT];
> +
> +      make_cleanup_restore_integer (p);
> +      *p = 1;
>      }
>  
>    if (parse->cmd->argv_func != NULL)
> diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
> index beac2cd..1c7bf00 100644
> --- a/gdb/mi/mi-main.h
> +++ b/gdb/mi/mi-main.h
> @@ -32,7 +32,9 @@ extern char *current_token;
>  
>  extern int running_result_record_printed;
>  extern int mi_proceeded;
> -extern int mi_suppress_breakpoint_notifications;
> +
> +enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT };
> +extern int mi_suppress_notification[];
>  

Quite frankly, I don't see how putting these in an array is better than
a separate global for each.  The memory used is the same, and with separate
globals, it's a little easier to debug from a top gdb (just print mi_suppress_<TAB> to
see the list of possibilities, etc.)  Are you planning on doing something over the
whole array, that is abstracted from the semantics of each element of the array?

-- 
Pedro Alves

Re: [PATCH 2/6] allow to suppress more mi notification

[Yao Qi]

On Thursday, July 26, 2012 04:30:18 PM Pedro Alves wrote:
> > -extern int mi_suppress_breakpoint_notifications;
> > +
> > +enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT };
> > +extern int mi_suppress_notification[];
> >
> >  
> 
> Quite frankly, I don't see how putting these in an array is better than
> a separate global for each.  The memory used is the same, and with separate
> globals, it's a little easier to debug from a top gdb (just print
> mi_suppress_<TAB> to see the list of possibilities, etc.)  Are you planning
> on doing something over the whole array, that is abstracted from the
> semantics of each element of the array?

The intention of this change is to avoid introducing more global variables to 
suppress different types of notification.  I plan to add more notifications 
here (for register change, memory change, trace experiment change, etc), and 
we need more suppress flags for them.  Array makes sense here.

AFAICS, I don't do something over the whole array.

-- 
Yao (齐尧)

Re: [PATCH 2/6] allow to suppress more mi notification

[Pedro Alves]

On 07/27/2012 03:57 AM, Yao Qi wrote:
> On Thursday, July 26, 2012 04:30:18 PM Pedro Alves wrote:
>>> -extern int mi_suppress_breakpoint_notifications;
>>> +
>>> +enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT };
>>> +extern int mi_suppress_notification[];
>>>
>>>  
>>
>> Quite frankly, I don't see how putting these in an array is better than
>> a separate global for each.  The memory used is the same, and with separate
>> globals, it's a little easier to debug from a top gdb (just print
>> mi_suppress_<TAB> to see the list of possibilities, etc.)  Are you planning
>> on doing something over the whole array, that is abstracted from the
>> semantics of each element of the array?
> 
> The intention of this change is to avoid introducing more global variables to 
> suppress different types of notification.  

More global variables, or more entries in a global array is really not very much
different..  And you still have to add global macros, one for each entry in
the array, so in the end, you end up with _more_ global symbols total.

> I plan to add more notifications
> here (for register change, memory change, trace experiment change, etc), and 
> we need more suppress flags for them.  Array makes sense here.
> 
> AFAICS, I don't do something over the whole array.

Array is really the worse choice.  Use it only when you have the need to iterate
over the elements.  Otherwise, it's just harder to debug.
"p mi_suppress_notification" will show you an array of integers.  Once you have a
few, that will just be cryptic output, and you'll have to
do "p mi_suppress_notification[...]" to make some sense out of it.

If you want to group the variables together, then the first choice should be
a struct instead.  Then you can still inspect the variables easily from
a top gdb.  E.g.,

struct
{
  int breakpoint : 1;
  int option_changed : 1;
  ...
} mi_suppress_notification;

And then you can have:

(gdb) p mi_suppress_notification
(gdb) p mi_suppress_notification.<TAB>
(gdb) p mi_suppress_notification.breakpoint

etc.

-- 
Pedro Alves

[PATCH 4/6] new add_setshow_enum_cmd_with_notif and scheduler-locking.

[Yao Qi]

Hi,
This patch is to add a new function add_setshow_enum_cmd_with_notif,
a friend function of add_setshow_enum_cmd.  If the command is
registered by this function, a MI notification is sent to MI frontend
when the option of command is changed.  We choose "scheduler-locking"
as one example of "enum" command, and write a test case for it.

gdb:

2012-07-23  Yao Qi  <yao@codesourcery.com>

	* cli/cli-decode.c (add_setshow_enum_cmd_with_notif): New.
	* command.h: Declare.
	* infrun.c (_initialize_infrun): Call
 	add_setshow_enum_cmd_with_notif.

gdb/testsuite:

2012-07-23  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-cmd-opt-changed.exp: New.
---
 gdb/cli/cli-decode.c                        |   28 ++++++++++++
 gdb/command.h                               |   12 +++++
 gdb/infrun.c                                |   11 +++--
 gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp |   61 +++++++++++++++++++++++++++
 4 files changed, 107 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 6739aef..5d314db 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -431,6 +431,34 @@ add_setshow_enum_cmd (char *name,
   c->enums = enumlist;
 }
 
+/* Same as add_setshow_enum_cmd except that observer 'command_option_change'
+   will be notified if command option is changed.  */
+
+void
+add_setshow_enum_cmd_with_notif (char *name,
+				 enum command_class class,
+				 const char *const *enumlist,
+				 const char **var,
+				 const char *set_doc,
+				 const char *show_doc,
+				 const char *help_doc,
+				 cmd_sfunc_ftype *set_func,
+				 show_value_ftype *show_func,
+				 struct cmd_list_element **set_list,
+				 struct cmd_list_element **show_list)
+{
+  struct cmd_list_element *c;
+
+  add_setshow_cmd_full (name, class, var_enum, var,
+			set_doc, show_doc, help_doc,
+			set_func, show_func,
+			set_list, show_list,
+			&c, NULL);
+  c->enums = enumlist;
+
+  c->notify_observer_p = 1;
+}
+
 const char *auto_boolean_enums[] = { "on", "off", "auto", NULL };
 
 /* Add an auto-boolean command named NAME to both the set and show
diff --git a/gdb/command.h b/gdb/command.h
index 88895bb..30c0eb2 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -245,6 +245,18 @@ extern void add_setshow_enum_cmd (char *name,
 				  struct cmd_list_element **set_list,
 				  struct cmd_list_element **show_list);
 
+extern void add_setshow_enum_cmd_with_notif (char *name,
+					     enum command_class class,
+					     const char *const *enumlist,
+					     const char **var,
+					     const char *set_doc,
+					     const char *show_doc,
+					     const char *help_doc,
+					     cmd_sfunc_ftype *set_func,
+					     show_value_ftype *show_func,
+					     struct cmd_list_element **set_list,
+					     struct cmd_list_element **show_list);
+
 extern void add_setshow_auto_boolean_cmd (char *name,
 					  enum command_class class,
 					  enum auto_boolean *var,
diff --git a/gdb/infrun.c b/gdb/infrun.c
index efc4162..1c5778c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7250,8 +7250,8 @@ By default, the debugger will use the same inferior."),
 			show_follow_exec_mode_string,
 			&setlist, &showlist);
 
-  add_setshow_enum_cmd ("scheduler-locking", class_run, 
-			scheduler_enums, &scheduler_mode, _("\
+  add_setshow_enum_cmd_with_notif ("scheduler-locking", class_run, 
+				   scheduler_enums, &scheduler_mode, _("\
 Set mode for locking scheduler during execution."), _("\
 Show mode for locking scheduler during execution."), _("\
 off  == no locking (threads may preempt at any time)\n\
@@ -7259,9 +7259,10 @@ on   == full locking (no thread except the current thread may run)\n\
 step == scheduler locked during every single-step operation.\n\
 	In this mode, no other thread may run during a step command.\n\
 	Other threads may run while stepping over a function call ('next')."), 
-			set_schedlock_func,	/* traps on target vector */
-			show_scheduler_mode,
-			&setlist, &showlist);
+				   /* traps on target vector */
+				   set_schedlock_func,
+				   show_scheduler_mode,
+				   &setlist, &showlist);
 
   add_setshow_boolean_cmd ("schedule-multiple", class_run, &sched_multi, _("\
 Set mode for resuming threads of all processes."), _("\
diff --git a/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp b/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
new file mode 100644
index 0000000..a733017
--- /dev/null
+++ b/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
@@ -0,0 +1,61 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib mi-support.exp
+set MIFLAGS "-i=mi"
+
+set testfile "basics"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/mi-cmd-opt-changed
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+     untested mi-cmd-opt-changed.exp
+     return -1
+}
+
+proc test_command_option_change { } { with_test_prefix "cmd option" {
+    if [mi_gdb_start] {
+	return
+    }
+    mi_run_to_main
+
+    foreach opt { "on" "off" "step" } {
+	mi_gdb_test "set scheduler-locking ${opt}" \
+	    ".*=option-changed,option=\"scheduler-locking\",value=\"${opt}\".*\\^done" \
+	    "\"set scheduler-locking ${opt}\""
+    }
+    foreach opt { "on" "off" "step" } {
+	mi_gdb_test "interpreter-exec console \"set scheduler-locking ${opt}\"" \
+	    ".*=option-changed,option=\"scheduler-locking\",value=\"${opt}\".*\\^done" \
+	    "interpreter-exec \"set scheduler-locking ${opt}\""
+    }
+    # Don't emit MI notification for request from MI.
+    mi_gdb_test "-gdb-set scheduler-locking on" \
+	{\^done} \
+	"\"set scheduler-locking on\" no event (requested by MI)"
+
+    mi_gdb_test "interpreter-exec mi \"-gdb-set scheduler-locking step\"" \
+	"\\&\"interpreter-exec mi .*\"-gdb-set scheduler-locking step.*\"\\\\n\"\r\n\\^done\r\n\\^done" \
+	"\"set scheduler-locking step\" no event (requested by MI interp)"
+    mi_gdb_test "set scheduler-locking step" \
+	"\\&\"set scheduler-locking step\\\\n\"\r\n\\^done" \
+	"\"set scheduler-locking stepr\" no event"
+
+    mi_gdb_exit
+}}
+
+test_command_option_change
+
+return 0
-- 
1.7.7.6

Re: [PATCH 4/6] new add_setshow_enum_cmd_with_notif and scheduler-locking.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-07-23  Yao Qi  <yao@codesourcery.com>
Yao> 	* cli/cli-decode.c (add_setshow_enum_cmd_with_notif): New.
Yao> 	* command.h: Declare.
Yao> 	* infrun.c (_initialize_infrun): Call
Yao>  	add_setshow_enum_cmd_with_notif.

This part seems fine, though of course pending on the earlier patch and
discussion about the overall approach.

I wish there were a nice way to do without yet another
command-registration function.  We already have a dizzying variety.

Yao> +set testfile "basics"
Yao> +set srcfile ${testfile}.c
Yao> +set binfile ${objdir}/${subdir}/mi-cmd-opt-changed

Please use standard_testfile now.

Tom

Re: [PATCH 4/6] new add_setshow_enum_cmd_with_notif and scheduler-locking.

[Pedro Alves]

On 07/24/2012 09:50 PM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
> 
> Yao> 2012-07-23  Yao Qi  <yao@codesourcery.com>
> Yao> 	* cli/cli-decode.c (add_setshow_enum_cmd_with_notif): New.
> Yao> 	* command.h: Declare.
> Yao> 	* infrun.c (_initialize_infrun): Call
> Yao>  	add_setshow_enum_cmd_with_notif.
> 
> This part seems fine, though of course pending on the earlier patch and
> discussion about the overall approach.
> 
> I wish there were a nice way to do without yet another
> command-registration function.  We already have a dizzying variety.

I can think of one way.  Do it like deprecate_cmd (and I think we have
other examples, but I'm not finding them now) -- make add_setshow_enum_cmd
return the command handle, and add a set_cmd_wants_notif or something function
that takes the command handle as argument, and sets the boolean.

IOW, the code that installs the command would do

  c = add_setshow_enum_cmd(...);
  set_cmd_wants_notif (c);

-- 
Pedro Alves

[PATCH 1/6] new observer command_option_changed.

[Yao Qi]

Hi,
This patch is to add an extra field 'notify_observer_p' to identify
the command will notify observer 'command_option_changed', and teach
GDB to check the change of command option, and notify observer.

gdb:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* cli/cli-decode.c (add_cmd): Set field 'notify_observer_p' to 0.
	(add_setshow_auto_boolean_cmd): Move auto_boolean_enums out.
	Remove 'static'.
	* cli/cli-decode.h (struct cmd_list_element) <notify_observer_p>:
	New field.
	Declare 'auto_boolean_enums'.
	* cli/cli-setshow.c (do_setshow_command): Notify observer
	command_option_changed if the option of command is changed.

gdb/doc:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* observer.texi: New observer command_option_changed.
---
 gdb/cli/cli-decode.c  |    4 +-
 gdb/cli/cli-decode.h  |    4 ++
 gdb/cli/cli-setshow.c |  142 ++++++++++++++++++++++++++++++++++++++++---------
 gdb/doc/observer.texi |    5 ++
 4 files changed, 129 insertions(+), 26 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index c337b43..6739aef 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -203,6 +203,7 @@ add_cmd (char *name, enum command_class class, void (*fun) (char *, int),
   c->user_commands = NULL;
   c->cmd_pointer = NULL;
   c->alias_chain = NULL;
+  c->notify_observer_p = 0;
 
   return c;
 }
@@ -430,6 +431,8 @@ add_setshow_enum_cmd (char *name,
   c->enums = enumlist;
 }
 
+const char *auto_boolean_enums[] = { "on", "off", "auto", NULL };
+
 /* Add an auto-boolean command named NAME to both the set and show
    command list lists.  CLASS is as in add_cmd.  VAR is address of the
    variable which will contain the value.  DOC is the documentation
@@ -445,7 +448,6 @@ add_setshow_auto_boolean_cmd (char *name,
 			      struct cmd_list_element **set_list,
 			      struct cmd_list_element **show_list)
 {
-  static const char *auto_boolean_enums[] = { "on", "off", "auto", NULL };
   struct cmd_list_element *c;
 
   add_setshow_cmd_full (name, class, var_auto_boolean, var,
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index b5e0790..73001a2 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -210,6 +210,9 @@ struct cmd_list_element
 
     /* Link pointer for aliases on an alias list.  */
     struct cmd_list_element *alias_chain;
+
+    /* Notify 'command_option_changed' observer if it is true.  */
+    int notify_observer_p;
   };
 
 extern void help_cmd_list (struct cmd_list_element *, enum command_class,
@@ -232,5 +235,6 @@ extern void not_just_help_class_command (char *arg, int from_tty);
 
 extern void print_doc_line (struct ui_file *, char *);
 
+extern const char *auto_boolean_enums[];
 
 #endif /* !defined (CLI_DECODE_H) */
diff --git a/gdb/cli/cli-setshow.c b/gdb/cli/cli-setshow.c
index 7ffb89e..9369b29 100644
--- a/gdb/cli/cli-setshow.c
+++ b/gdb/cli/cli-setshow.c
@@ -28,6 +28,8 @@
 #include "cli/cli-cmds.h"
 #include "cli/cli-setshow.h"
 
+#include "observer.h"
+
 /* Prototypes for local functions.  */
 
 static int parse_binary_operation (char *);
@@ -170,50 +172,122 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 #endif
 	    *q++ = '\0';
 	    new = (char *) xrealloc (new, q - new);
-	    xfree (*(char **) c->var);
-	    *(char **) c->var = new;
+
+	    if (*(char **) c->var == NULL
+		|| strcmp (*(char **) c->var, new) != 0)
+	      {
+		xfree (*(char **) c->var);
+		*(char **) c->var = new;
+
+		if (c->notify_observer_p)
+		  observer_notify_command_option_changed (c->name, new);
+	      }
+	    else
+	      xfree (new);
 	  }
 	  break;
 	case var_string_noescape:
 	  if (arg == NULL)
 	    arg = "";
-	  xfree (*(char **) c->var);
-	  *(char **) c->var = xstrdup (arg);
+
+	  if (*(char **) c->var == NULL || strcmp (*(char **) c->var, arg) != 0)
+	    {
+	      xfree (*(char **) c->var);
+	      *(char **) c->var = xstrdup (arg);
+
+	      if (c->notify_observer_p)
+		observer_notify_command_option_changed (c->name, arg);
+	    }
 	  break;
 	case var_filename:
 	  if (arg == NULL)
 	    error_no_arg (_("filename to set it to."));
 	  /* FALLTHROUGH */
 	case var_optional_filename:
-	  xfree (*(char **) c->var);
+	  {
+	    char *val = NULL;
 
-	  if (arg != NULL)
-	    {
-	      /* Clear trailing whitespace of filename.  */
-	      char *ptr = arg + strlen (arg) - 1;
+	    if (arg != NULL)
+	      {
+		/* Clear trailing whitespace of filename.  */
+		char *ptr = arg + strlen (arg) - 1;
 
-	      while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
-		ptr--;
-	      *(ptr + 1) = '\0';
+		while (ptr >= arg && (*ptr == ' ' || *ptr == '\t'))
+		  ptr--;
+		*(ptr + 1) = '\0';
 
-	      *(char **) c->var = tilde_expand (arg);
-	    }
-	  else
-	    *(char **) c->var = xstrdup ("");
+		val = tilde_expand (arg);
+	      }
+	    else
+	      val = xstrdup ("");
+
+	    if (*(char **) c->var == NULL
+		|| strcmp (*(char **) c->var, val) != 0)
+	      {
+		xfree (*(char **) c->var);
+		*(char **) c->var = val;
+
+		if (c->notify_observer_p)
+		  observer_notify_command_option_changed (c->name, val);
+	      }
+	    else
+	      xfree (val);
+	  }
 	  break;
 	case var_boolean:
-	  *(int *) c->var = parse_binary_operation (arg);
+	  {
+	    int val = parse_binary_operation (arg);
+
+	    if (val != *(int *) c->var)
+	      {
+		*(int *) c->var = val;
+
+		if (c->notify_observer_p)
+		  observer_notify_command_option_changed (c->name,
+							  val ? "on" : "off");
+	      }
+	  }
 	  break;
 	case var_auto_boolean:
-	  *(enum auto_boolean *) c->var = parse_auto_binary_operation (arg);
+	  {
+	    enum auto_boolean val = parse_auto_binary_operation (arg);
+
+	    if (*(enum auto_boolean *) c->var != val)
+	      {
+		*(enum auto_boolean *) c->var = val;
+
+		if (c->notify_observer_p)
+		  {
+		    char *s = (char *) auto_boolean_enums[val];
+
+		    observer_notify_command_option_changed (c->name, s);
+		  }
+	      }
+	  }
 	  break;
 	case var_uinteger:
 	case var_zuinteger:
 	  if (arg == NULL)
 	    error_no_arg (_("integer to set it to."));
-	  *(unsigned int *) c->var = parse_and_eval_long (arg);
-	  if (c->var_type == var_uinteger && *(unsigned int *) c->var == 0)
-	    *(unsigned int *) c->var = UINT_MAX;
+	  {
+	    unsigned int val = parse_and_eval_long (arg);
+
+	    if (c->var_type == var_uinteger && val == 0)
+	      val = UINT_MAX;
+
+	    if (*(unsigned int *) c->var != val)
+	      {
+		*(unsigned int *) c->var = val;
+
+		if (c->notify_observer_p)
+		  {
+		    char s[64];
+
+		    xsnprintf (s, sizeof s, "%d", val);
+		    observer_notify_command_option_changed (c->name, s);
+		  }
+	      }
+	  }
 	  break;
 	case var_integer:
 	case var_zinteger:
@@ -224,11 +298,23 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	      error_no_arg (_("integer to set it to."));
 	    val = parse_and_eval_long (arg);
 	    if (val == 0 && c->var_type == var_integer)
-	      *(int *) c->var = INT_MAX;
+	      val = INT_MAX;
 	    else if (val >= INT_MAX)
 	      error (_("integer %u out of range"), val);
-	    else
-	      *(int *) c->var = val;
+
+
+	    if (*(int *) c->var != val)
+	      {
+		*(int *) c->var = val;
+
+		if (c->notify_observer_p)
+		  {
+		    char s[64];
+
+		    xsnprintf (s, sizeof s, "%d", val);
+		    observer_notify_command_option_changed (c->name, s);
+		  }
+	      }
 	    break;
 	  }
 	case var_enum:
@@ -293,7 +379,13 @@ do_setshow_command (char *arg, int from_tty, struct cmd_list_element *c)
 	    if (nmatches > 1)
 	      error (_("Ambiguous item \"%s\"."), arg);
 
-	    *(const char **) c->var = match;
+	    if (*(const char **) c->var != match)
+	      {
+		*(const char **) c->var = match;
+
+		if (c->notify_observer_p)
+		    observer_notify_command_option_changed (c->name, match);
+	      }
 	  }
 	  break;
 	default:
diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
index 14d4ac3..a45df36 100644
--- a/gdb/doc/observer.texi
+++ b/gdb/doc/observer.texi
@@ -234,6 +234,11 @@ the current top-level prompt.
 Variable gdb_datadir has been set.  The value may not necessarily change.
 @end deftypefun
 
+@deftypefun void command_option_changed (const char *@var{command}, const char *@var{option})
+The option of some @code{set} commands in console are changed.  This method is called after
+a command @code{set command option}.
+@end deftypefun
+
 @deftypefun void test_notification (int @var{somearg})
 This observer is used for internal testing.  Do not use.  
 See testsuite/gdb.gdb/observer.exp.
-- 
1.7.7.6

Re: [PATCH 1/6] new observer command_option_changed.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> This patch is to add an extra field 'notify_observer_p' to identify
Yao> the command will notify observer 'command_option_changed', and teach
Yao> GDB to check the change of command option, and notify observer.

I read through the series to find out why this was needed.
My understanding is that the idea is that options will be segregated
into two kinds: those which report via the observer, and others; and
that the decision is made in the gdb sources.

If that is accurate, I wonder if you could explain the rationale for it.
Another possible design would be to report all option changes; or to let
the MI client choose.

The reason I ask is that making the decision statically seems tricky.
When adding an option, what criteria do we use to decide?  How do we
know what MI users want?  Or what if we want to add a Python hook
here -- maybe we would switch many more options over; but does switching
an option from non-reporting to reporting cause issues for existing MI
clients?  (I assume there is at least a bandwidth concern...)

Yao> +const char *auto_boolean_enums[] = { "on", "off", "auto", NULL };

It seems like now this should be 'const char * const'.
I realize you're just moving it, so normally it would be exempt from
this kind of tweakery, but in this case it is also now exported...

Yao> +		if (c->notify_observer_p)
Yao> +		  {
Yao> +		    char *s = (char *) auto_boolean_enums[val];

I don't think the cast is needed.

Yao> +@deftypefun void command_option_changed (const char *@var{command}, const char *@var{option})
Yao> +The option of some @code{set} commands in console are changed.  This method is called after
Yao> +a command @code{set command option}.
Yao> +@end deftypefun

The lines in the body are all too long.

I think it would be good to describe the meaning of the arguments.


What happens in the case where an option has a validation function that
fails?  IIRC gdb has an internal design flaw here.

Tom

Re: [PATCH 1/6] new observer command_option_changed.

[Yao Qi]

On Tuesday, July 24, 2012 02:38:52 PM Tom Tromey wrote:
> >>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Hi, Tom,
Thanks for the quick review.

> Yao> This patch is to add an extra field 'notify_observer_p' to identify
> Yao> the command will notify observer 'command_option_changed', and teach
> Yao> GDB to check the change of command option, and notify observer.
> 
> I read through the series to find out why this was needed.
> My understanding is that the idea is that options will be segregated
> into two kinds: those which report via the observer, and others; and
> that the decision is made in the gdb sources.
> 
> If that is accurate, I wonder if you could explain the rationale for it.
> Another possible design would be to report all option changes; or to let
> the MI client choose.

The general goal of my work is that "when the internal state of GDB is 
changed, and MI frontend and/or telnet session is not aware of the change, 
notify them to keep their display up to date".  For example, when user types 
command 'set $fp=0x1' in console, GDB should emit a MI notification to 
Eclipse, so that Eclipse can update its "register view" to show the latest 
value of $fp.  When user changes the option "set scheduler-locking step", and 
this change is interesting to Eclipse, GDB should emit a MI notification as 
well.

Not all the changes of GDB state are interesting to MI frontend.  As a subset 
of GDB state changes (other patch series in the future address other GDB state 
changes), not all the command option changes are interesting to MI frontend 
either, so it is not necessary to report all option changes.

> 
> The reason I ask is that making the decision statically seems tricky.
> When adding an option, what criteria do we use to decide?  How do we
> know what MI users want?  Or what if we want to add a Python hook

The criteria is "if Eclipse or other MI frontend needs to know the change of 
this command option, then we should report notification".  IMO, MI user should 
tell us what they want, and we can convert these commands from non-reporting 
to reporting.  Here is a list of commands that Marc Khouzam and I discussed 
offline, and we think they should be switched to reporting.  Note that this is 
not a full list, we may have other commands if needed.

   set circular-trace-buffer
   set disconnected-tracing
   set exec-direction
   set observer
   set may-insert-breakpoints (and the other may-*)
   set record
   set scheduler-locking
   set trace-notes (and other trace-*)
   set auto-solib-add

What do you mean by "add a Python hook"?  Is it something like "using an 
instance of gdb.Command to implement a new GDB CLI command"?  I know few on 
gdb python, but we may add extra parameter NOTIFY in 'Command.__init__', and 
notify command_option_changed observer somewhere.

> here -- maybe we would switch many more options over; but does switching
> an option from non-reporting to reporting cause issues for existing MI
> clients?  (I assume there is at least a bandwidth concern...)

AFAICS, the new MI notification "=option-changed" should not cause issues for 
existing MI clients.  In terms of bandwidth, every "=option-changed" is sent 
once user types command in console, user can't enter many commands at one 
moment (one command per second and ten commands at most, I think).  This 
notification should not occupy much bandwidth.

> What happens in the case where an option has a validation function that
> fails?  IIRC gdb has an internal design flaw here.
> 

If I understand you correctly, "validation function" means cli/cli-
setshow.c:parse_binary_operation and cli/cli-
setshow.c:parse_auto_binary_operation.  When validation fails, an error is 
thrown out, and observer is not called.

-- 
Yao (齐尧)

Re: [PATCH 1/6] new observer command_option_changed.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> The general goal of my work is that "when the internal state of GDB
Yao> is changed, and MI frontend and/or telnet session is not aware of
Yao> the change, notify them to keep their display up to date".

I'm fully on board with this plan.

Yao> What do you mean by "add a Python hook"?

Sorry, I wasn't clear.

Suppose we want to add a new Python event so that Python code can also
detect parameter changes.  My supposition is that in this case we'd want
to not filter, and just watch them all.

Yao> In terms of bandwidth, every "=option-changed" is sent once user
Yao> types command in console, user can't enter many commands at one
Yao> moment (one command per second and ten commands at most, I think).
Yao> This notification should not occupy much bandwidth.

In that case, why not just report them all?
I agree with your thinking here, that the rate is going to be quite
low.  I'm not sure why I didn't realize that yesterday :)

Reporting them all means simpler code on the gdb side and also lets us
avoid the new set/show constructors.  I don't see a downside.

Tom> What happens in the case where an option has a validation function that
Tom> fails?  IIRC gdb has an internal design flaw here.

Yao> If I understand you correctly, "validation function" means cli/cli-
Yao> setshow.c:parse_binary_operation and cli/cli-
Yao> setshow.c:parse_auto_binary_operation.  When validation fails, an error is 
Yao> thrown out, and observer is not called.

Sorry again for the lack of clarity.

I mean the call to c->func at the end of do_setshow_command.

There are various spots in gdb that use a hidden variable to keep the
"real" setting in case setting the value here fails.

For example, try "set input-radix 1".  This is an error, but I think
with your patch the MI client will see a notification saying that it was
changed to "1".

The design flaw here is that "set" functions do their validation after
the setting has been made, not before.

One approach to this would be to fix this design flaw.  That's likely to
be a lot of work...

Another approach might be to move the observer notification later.

Tom

Re: [PATCH 1/6] new observer command_option_changed.

[Pedro Alves]

On 07/25/2012 03:43 PM, Tom Tromey wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
> 
> Yao> The general goal of my work is that "when the internal state of GDB
> Yao> is changed, and MI frontend and/or telnet session is not aware of
> Yao> the change, notify them to keep their display up to date".
> 
> I'm fully on board with this plan.
> 
> Yao> What do you mean by "add a Python hook"?
> 
> Sorry, I wasn't clear.
> 
> Suppose we want to add a new Python event so that Python code can also
> detect parameter changes.  My supposition is that in this case we'd want
> to not filter, and just watch them all.
> 
> Yao> In terms of bandwidth, every "=option-changed" is sent once user
> Yao> types command in console, user can't enter many commands at one
> Yao> moment (one command per second and ten commands at most, I think).
> Yao> This notification should not occupy much bandwidth.
> 
> In that case, why not just report them all?
> I agree with your thinking here, that the rate is going to be quite
> low.  I'm not sure why I didn't realize that yesterday :)

The case where rate would be a bit higher is on canned sequences of
commands, like user defined functions, that could do things like
wrap a command with a set foo X; something; set foo Y;

> Reporting them all means simpler code on the gdb side and also lets us
> avoid the new set/show constructors.  I don't see a downside.

I agree.  Statically deciding which options to report changes to will
not scale.  It's about the same as deferring the reporting all
changes to a gdb of 10 years from now, when enough frontends have
requested we notify more and more options.  :-)  It also essentially
is a one way street - only you report a changes for an option, you'll
never decide to stop reporting it, for fear of that breaking some
frontend.  Best is just to either:

- report all
- report none, but instead report a single "some option changed", and
  let the frontend refresh all its state.
- let the frontend tell gdb which settings it is interested in.

The "all" option seems the simplest.

> Tom> What happens in the case where an option has a validation function that
> Tom> fails?  IIRC gdb has an internal design flaw here.
> 
> Yao> If I understand you correctly, "validation function" means cli/cli-
> Yao> setshow.c:parse_binary_operation and cli/cli-
> Yao> setshow.c:parse_auto_binary_operation.  When validation fails, an error is 
> Yao> thrown out, and observer is not called.
> 
> Sorry again for the lack of clarity.
> 
> I mean the call to c->func at the end of do_setshow_command.
> 
> There are various spots in gdb that use a hidden variable to keep the
> "real" setting in case setting the value here fails.
> 
> For example, try "set input-radix 1".  This is an error, but I think
> with your patch the MI client will see a notification saying that it was
> changed to "1".
> 
> The design flaw here is that "set" functions do their validation after
> the setting has been made, not before.

Yeah, or rather, that there is not "validate" hook, and the "set" hook
is abused for that.

> One approach to this would be to fix this design flaw.  That's likely to
> be a lot of work...

I don't think there any that many cases to fix.

> Another approach might be to move the observer notification later.

-- 
Pedro Alves

Re: [PATCH 1/6] new observer command_option_changed.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> +	      if (c->notify_observer_p)
Yao> +		observer_notify_command_option_changed (c->name, arg);

Last night I realized that this does the wrong thing for multi-word
parameters.  E.g., for "set print elements", the MI client will just see
"elements".

Tom

Re: [PATCH 1/6] new observer command_option_changed.

[Yao Qi]

On Wednesday, July 25, 2012 08:31:55 AM Tom Tromey wrote:
> Yao> +        if (c->notify_observer_p)
> Yao> +          observer_notify_command_option_changed (c->name, arg);
> 
> Last night I realized that this does the wrong thing for multi-word
> parameters.  E.g., for "set print elements", the MI client will just see
> "elements".

Right, we are unable to get the previous command instance "print" in command 
instance  "elements".  So we have to move 
'observer_notify_command_option_changed' out of do_setshow_command to its 
caller, top.c:execute_command, where the full command option and value is 
available.  Then, we can change 'do_setshow_command' to return a boolean value 
indicating option is changed or not, and call 
observer_notify_command_option_changed if do_setshow_command returns true.

It works fine for me now.  I'll post a patch.

-- 
Yao (齐尧)

[PATCH 3/6] attach to command_option-changed observer.

[Yao Qi]

Hi,
This patch is to attach function 'mi_command_option_changed' to
observer 'command_option_changed', so that a MI notification
"=option-changed" is sent to MI frontend.  If the command option
change is requested from MI, the notification is suppressed.

gdb:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* NEWS: Mention new MI notification.
	* mi/mi-interp.c: Declare mi_command_option_changed.
	(mi_interpreter_init): Attach mi_command_option_changed to
	observer command_option_changed.
	(mi_command_option_changed): New.
	* mi/mi-main.c (mi_cmd_execute): Check command 'gdb-set' and set
	mi_suppress_notification.
	* mi/mi-main.h: New enum 'MI_SUPPRESS_COMMAND'.

gdb/doc:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* gdb.texinfo (GDB/MI Async Records): Doc for '=option-changed'.
---
 gdb/NEWS            |    5 +++++
 gdb/doc/gdb.texinfo |    3 +++
 gdb/mi/mi-interp.c  |   21 ++++++++++++++++++++-
 gdb/mi/mi-main.c    |    7 +++++++
 gdb/mi/mi-main.h    |    2 +-
 5 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 3333810..0f73e60 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,11 @@
 
 *** Changes since GDB 7.5
 
+* MI changes
+
+  ** Command option changes are now notified using new async record
+     "=option-changed".
+
 *** Changes in GDB 7.5
 
 * GDB now supports x32 ABI.  Visit <http://sites.google.com/site/x32abi/>
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 68ea817..5ad8c9d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -27638,6 +27638,9 @@ breakpoint commands; @xref{GDB/MI Breakpoint Commands}.
 Note that if a breakpoint is emitted in the result record of a
 command, then it will not also be emitted in an async record.
 
+@item =option-changed,option=@var{command},value=@var{value}
+Reports that an option of the command @var{set command} is changed
+to @var{value}.
 @end table
 
 @node GDB/MI Frame Information
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 885d08b..b45b43e 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -71,6 +71,7 @@ static void mi_about_to_proceed (void);
 static void mi_breakpoint_created (struct breakpoint *b);
 static void mi_breakpoint_deleted (struct breakpoint *b);
 static void mi_breakpoint_modified (struct breakpoint *b);
+static void mi_command_option_changed (const char *option, const char *value);
 
 static int report_initial_inferior (struct inferior *inf, void *closure);
 
@@ -128,6 +129,7 @@ mi_interpreter_init (struct interp *interp, int top_level)
       observer_attach_breakpoint_created (mi_breakpoint_created);
       observer_attach_breakpoint_deleted (mi_breakpoint_deleted);
       observer_attach_breakpoint_modified (mi_breakpoint_modified);
+      observer_attach_command_option_changed (mi_command_option_changed);
 
       /* The initial inferior is created before this function is
 	 called, so we need to report it explicitly.  Use iteration in
@@ -504,7 +506,7 @@ mi_about_to_proceed (void)
 /* When the element is non-zero, no MI notifications will be emitted in
    response to the corresponding observers.  */
 
-int mi_suppress_notification[] = { 0 };
+int mi_suppress_notification[] = { 0, 0 };
 
 /* Emit notification about a created breakpoint.  */
 
@@ -730,6 +732,23 @@ mi_solib_unloaded (struct so_list *solib)
   gdb_flush (mi->event_channel);
 }
 
+static void
+mi_command_option_changed (const char *option, const char *value)
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+
+  if (mi_suppress_notification[MI_SUPPRESS_COMMAND])
+    return;
+
+  target_terminal_ours ();
+
+  fprintf_unfiltered (mi->event_channel,
+		      "option-changed,option=\"%s\",value=\"%s\"",
+		      option, value);
+
+  gdb_flush (mi->event_channel);
+}
+
 static int
 report_initial_inferior (struct inferior *inf, void *closure)
 {
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 0ae867d..82a1890 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2104,6 +2104,13 @@ mi_cmd_execute (struct mi_parse *parse)
       make_cleanup_restore_integer (p);
       *p = 1;
     }
+  else if (strncmp (parse->command, "gdb-set", sizeof ("gdb-set") - 1 ) == 0)
+    {
+      int *p = &mi_suppress_notification[MI_SUPPRESS_COMMAND];
+
+      make_cleanup_restore_integer (p);
+      *p = 1;
+    }
 
   if (parse->cmd->argv_func != NULL)
     {
diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
index 1c7bf00..93c0692 100644
--- a/gdb/mi/mi-main.h
+++ b/gdb/mi/mi-main.h
@@ -33,7 +33,7 @@ extern char *current_token;
 extern int running_result_record_printed;
 extern int mi_proceeded;
 
-enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT };
+enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT, MI_SUPPRESS_COMMAND };
 extern int mi_suppress_notification[];
 
 #endif
-- 
1.7.7.6

Re: [PATCH 3/6] attach to command_option-changed observer.

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Wed, 25 Jul 2012 00:10:49 +0800
> 
> This patch is to attach function 'mi_command_option_changed' to
> observer 'command_option_changed', so that a MI notification
> "=option-changed" is sent to MI frontend.  If the command option
> change is requested from MI, the notification is suppressed.

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,11 @@
>  
>  *** Changes since GDB 7.5
>  
> +* MI changes
> +
> +  ** Command option changes are now notified using new async record
> +     "=option-changed".

OK.

> +@item =option-changed,option=@var{command},value=@var{value}
> +Reports that an option of the command @var{set command} is changed
> +to @var{value}.                       ^^^^^^^^^^^^^^^^^

Please use "@code{set @var{command}}" instead of the underlined part.
Only COMMAND should be in @var, since "set" is a literal string.

Btw, it is better to use "option" instead of "command" in the above,
since that's what "set" does -- it changes values of user options.

The documentation parts are OK with those changes.

Re: [PATCH 3/6] attach to command_option-changed observer.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> 2012-07-24  Yao Qi  <yao@codesourcery.com>
Yao> 	* NEWS: Mention new MI notification.
Yao> 	* mi/mi-interp.c: Declare mi_command_option_changed.
Yao> 	(mi_interpreter_init): Attach mi_command_option_changed to
Yao> 	observer command_option_changed.
Yao> 	(mi_command_option_changed): New.
Yao> 	* mi/mi-main.c (mi_cmd_execute): Check command 'gdb-set' and set
Yao> 	mi_suppress_notification.
Yao> 	* mi/mi-main.h: New enum 'MI_SUPPRESS_COMMAND'.

Yao> +static void
Yao> +mi_command_option_changed (const char *option, const char *value)

Needs an intro comment; something small is fine.

Yao> +  fprintf_unfiltered (mi->event_channel,
Yao> +		      "option-changed,option=\"%s\",value=\"%s\"",
Yao> +		      option, value);

This has a quoting bug.  VALUE can contain characters requiring special
quoting for MI.

I once wrote a patch to change the event channel to use ui-out:

    http://sourceware.org/ml/gdb-patches/2011-01/msg00518.html

That would be one way to deal with the problem, but there are probably
other ways as well.  (I didn't commit that patch because Volodya had a
different approach to emitting the notifications that I was interested
in... but I still think it is a decent idea.)

Yao> +  else if (strncmp (parse->command, "gdb-set", sizeof ("gdb-set") - 1 ) == 0)

There's an extra space before the closing paren.

I don't like this approach much but I understand why you did it.

Yao> +enum MI_SUPRESS_NOTIFICATION { MI_SUPPRESS_BREAKPOINT, MI_SUPPRESS_COMMAND };

Put each constant on its own line.

Tom

Re: [PATCH 3/6] attach to command_option-changed observer.

[Yao Qi]

On Tuesday, July 24, 2012 02:47:04 PM Tom Tromey wrote:
> Yao> +  fprintf_unfiltered (mi->event_channel,
> Yao> +                "option-changed,option=\"%s\",value=\"%s\"",
> Yao> +                option, value);
> 
> This has a quoting bug.  VALUE can contain characters requiring special
> quoting for MI.
> 
> I once wrote a patch to change the event channel to use ui-out:
> 
>     http://sourceware.org/ml/gdb-patches/2011-01/msg00518.html
> 
> That would be one way to deal with the problem, but there are probably
> other ways as well.  (I didn't commit that patch because Volodya had a
> different approach to emitting the notifications that I was interested
> in... but I still think it is a decent idea.)

Hi, Tom,
What is the different approach?  I don't see any discussion in that thread.

What should we do here?  Shall we keep using ui-file (as what I wrote in this 
patch) or migrate to ui-out with your patch applied?

-- 
Yao (齐尧)

Re: [PATCH 3/6] attach to command_option-changed observer.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Tom> That would be one way to deal with the problem, but there are probably
Tom> other ways as well.  (I didn't commit that patch because Volodya had a
Tom> different approach to emitting the notifications that I was interested
Tom> in... but I still think it is a decent idea.)

Yao> What is the different approach?  I don't see any discussion in that
Yao> thread.

That patch was preparation for emitting better breakpoint notifications
-- which Vladimir implemented in a different way.

You can see what he did in mi-interp.c:mi_breakpoint_created.

Yao> What should we do here?  Shall we keep using ui-file (as what I
Yao> wrote in this patch) or migrate to ui-out with your patch applied?

I think you should take whichever approach you think is best.
The most important thing is to not have the quoting bug.

Tom

Re: [PATCH 3/6] attach to command_option-changed observer.

[Pedro Alves]

On 07/24/2012 05:10 PM, Yao Qi wrote:
> +  else if (strncmp (parse->command, "gdb-set", sizeof ("gdb-set") - 1 ) == 0)
> +    {
> +      int *p = &mi_suppress_notification[MI_SUPPRESS_COMMAND];

I honestly have trouble mapping "MI_SUPPRESS_COMMAND" to -gdb-set.

This is about =option-changed, so I think MI_SUPPRESS_OPTION_CHANGED would
be clearer.

-- 
Pedro Alves

[PATCH 6/6] new add_setshow_string_cmd_with_notif and trace-notes.

[Yao Qi]

Hi,
this patch is to add a new function add_setshow_string_cmd_with_notif,
and register command "trace-notes".  A new test case is added for it.

gdb:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* cli/cli-decode.c (add_setshow_string_cmd_with_notif): New.
	* command.h: Declare.
	* tracepoint.c (_initialize_tracepoint): Call
	add_setshow_string_cmd_with_notif.

gdb/testsuite:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-cmd-opt-changed.exp (test_command_option_change): Test for command
	'set trace-notes'.
---
 gdb/cli/cli-decode.c                        |   24 ++++++++++++++++++++++++
 gdb/command.h                               |   10 ++++++++++
 gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp |   11 +++++++++++
 gdb/tracepoint.c                            |    8 ++++----
 4 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index b8dfb6c..cf36abe 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -575,6 +575,30 @@ add_setshow_string_cmd (char *name, enum command_class class,
 			NULL, NULL);
 }
 
+/* Same as add_setshow_string_cmd except that observer 'command_option_change'
+   will be notified if command option is changed.  */
+
+void
+add_setshow_string_cmd_with_notif (char *name, enum command_class class,
+				   char **var,
+				   const char *set_doc, const char *show_doc,
+				   const char *help_doc,
+				   cmd_sfunc_ftype *set_func,
+				   show_value_ftype *show_func,
+				   struct cmd_list_element **set_list,
+				   struct cmd_list_element **show_list)
+{
+  struct cmd_list_element *c;
+
+  add_setshow_cmd_full (name, class, var_string, var,
+			set_doc, show_doc, help_doc,
+			set_func, show_func,
+			set_list, show_list,
+			&c, NULL);
+
+  c->notify_observer_p = 1;
+}
+
 /* Add element named NAME to both the set and show command LISTs (the
    list for set/show or some sublist thereof).  */
 void
diff --git a/gdb/command.h b/gdb/command.h
index c581997..50901af 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -309,6 +309,16 @@ extern void add_setshow_string_cmd (char *name,
 				    show_value_ftype *show_func,
 				    struct cmd_list_element **set_list,
 				    struct cmd_list_element **show_list);
+extern void add_setshow_string_cmd_with_notif (char *name,
+					       enum command_class class,
+					       char **var,
+					       const char *set_doc,
+					       const char *show_doc,
+					       const char *help_doc,
+					       cmd_sfunc_ftype *set_func,
+					       show_value_ftype *show_func,
+					       struct cmd_list_element **set_list,
+					       struct cmd_list_element **show_list);
 
 extern void add_setshow_string_noescape_cmd (char *name,
 					     enum command_class class,
diff --git a/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp b/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
index bedc2d7..5388b72 100644
--- a/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
@@ -70,6 +70,17 @@ proc test_command_option_change { } { with_test_prefix "cmd option" {
 	    "\"set ${command}\" no event"
     }
 
+
+    foreach command { "trace-notes" } {
+	foreach str_opt { "foo" "bar" } {
+	    mi_gdb_test "set ${command} ${str_opt}" \
+		".*=option-changed,option=\"${command}\",value=\"${str_opt}\".*\\^done" \
+		"\"set ${command} ${str_opt}\""
+	}
+	mi_gdb_test "set ${command} bar" \
+	    "\\&\"set ${command} bar\\\\n\"\r\n(\\&\"warning.*)\\^done" \
+	    "\"set ${command} bar\" no event"
+    }
     mi_gdb_exit
 }}
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index d09faf8..4eacefa 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -5363,12 +5363,12 @@ Show the user name to use for current and future trace runs"), NULL,
 			  set_trace_user, NULL,
 			  &setlist, &showlist);
 
-  add_setshow_string_cmd ("trace-notes", class_trace,
-			  &trace_notes, _("\
+  add_setshow_string_cmd_with_notif ("trace-notes", class_trace,
+				     &trace_notes, _("\
 Set notes string to use for current and future trace runs"), _("\
 Show the notes string to use for current and future trace runs"), NULL,
-			  set_trace_notes, NULL,
-			  &setlist, &showlist);
+				     set_trace_notes, NULL,
+				     &setlist, &showlist);
 
   add_setshow_string_cmd ("trace-stop-notes", class_trace,
 			  &trace_stop_notes, _("\
-- 
1.7.7.6

Re: [PATCH 6/6] new add_setshow_string_cmd_with_notif and trace-notes.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> this patch is to add a new function add_setshow_string_cmd_with_notif,
Yao> and register command "trace-notes".  A new test case is added for it.

This is ok (again pending the earlier discussion...)

thanks,
Tom

[PATCH 5/6] new add_setshow_boolean_cmd_with_notify and circular-trace-buffer.

[Yao Qi]

Hi,
Similar to previous patch, this patch is to add a new function
add_setshow_boolean_cmd_with_notif, and register command
"circular-trace-buffer" as an example to boolean command.  A new
test case is added for it.

gdb:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* cli/cli-decode.c (add_setshow_auto_boolean_cmd): Move
	'boolean_enums' out.
	(add_setshow_boolean_cmd_with_notif): New.
	* command.h (extern void add_setshow_boolean_cmd): Declare
	it.
	* tracepoint.c (_initialize_tracepoint): Call
	add_setshow_boolean_cmd_with_notif.

gdb/testsuite:

2012-07-24  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-cmd-opt-changed.exp (test_command_option_change): Test for command
	'circular-trace-buffer'.
---
 gdb/cli/cli-decode.c                        |   26 +++++++++++++++++++++++++-
 gdb/command.h                               |   10 ++++++++++
 gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp |   17 +++++++++++++++++
 gdb/tracepoint.c                            |   12 ++++++------
 4 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index 5d314db..b8dfb6c 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -486,6 +486,7 @@ add_setshow_auto_boolean_cmd (char *name,
   c->enums = auto_boolean_enums;
 }
 
+static const char *boolean_enums[] = { "on", "off", NULL };
 /* Add element named NAME to both the set and show command LISTs (the
    list for set/show or some sublist thereof).  CLASS is as in
    add_cmd.  VAR is address of the variable which will contain the
@@ -499,7 +500,6 @@ add_setshow_boolean_cmd (char *name, enum command_class class, int *var,
 			 struct cmd_list_element **set_list,
 			 struct cmd_list_element **show_list)
 {
-  static const char *boolean_enums[] = { "on", "off", NULL };
   struct cmd_list_element *c;
 
   add_setshow_cmd_full (name, class, var_boolean, var,
@@ -510,6 +510,30 @@ add_setshow_boolean_cmd (char *name, enum command_class class, int *var,
   c->enums = boolean_enums;
 }
 
+/* Same as add_setshow_boolean_cmd except that observer 'command_option_change'
+   will be notified if command option is changed.  */
+
+void
+add_setshow_boolean_cmd_with_notif (char *name, enum command_class class,
+				    int *var,
+				    const char *set_doc, const char *show_doc,
+				    const char *help_doc,
+				    cmd_sfunc_ftype *set_func,
+				    show_value_ftype *show_func,
+				    struct cmd_list_element **set_list,
+				    struct cmd_list_element **show_list)
+{
+  struct cmd_list_element *c;
+
+  add_setshow_cmd_full (name, class, var_boolean, var,
+			set_doc, show_doc, help_doc,
+			set_func, show_func,
+			set_list, show_list,
+			&c, NULL);
+  c->enums = boolean_enums;
+  c->notify_observer_p = 1;
+}
+
 /* Add element named NAME to both the set and show command LISTs (the
    list for set/show or some sublist thereof).  */
 void
diff --git a/gdb/command.h b/gdb/command.h
index 30c0eb2..c581997 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -277,6 +277,16 @@ extern void add_setshow_boolean_cmd (char *name,
 				     show_value_ftype *show_func,
 				     struct cmd_list_element **set_list,
 				     struct cmd_list_element **show_list);
+extern void add_setshow_boolean_cmd_with_notif (char *name,
+						enum command_class class,
+						int *var,
+						const char *set_doc,
+						const char *show_doc,
+						const char *help_doc,
+						cmd_sfunc_ftype *set_func,
+						show_value_ftype *show_func,
+						struct cmd_list_element **set_list,
+						struct cmd_list_element **show_list);
 
 extern void add_setshow_filename_cmd (char *name,
 				      enum command_class class,
diff --git a/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp b/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
index a733017..bedc2d7 100644
--- a/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-cmd-opt-changed.exp
@@ -53,6 +53,23 @@ proc test_command_option_change { } { with_test_prefix "cmd option" {
 	"\\&\"set scheduler-locking step\\\\n\"\r\n\\^done" \
 	"\"set scheduler-locking stepr\" no event"
 
+
+    foreach command { "circular-trace-buffer" } {
+
+	# The default value of each command option may be different, so we first
+	# set it to 'off', and this may or may not trigger MI notification.
+	mi_gdb_test "set ${command} off" ".*\\^done" "\"set ${command}\" warmup"
+
+	foreach boolean_opt { "on" "off" } {
+	    mi_gdb_test "set ${command} ${boolean_opt}" \
+		".*=option-changed,option=\"${command}\",value=\"${boolean_opt}\".*\\^done" \
+		"\"set ${command} ${boolean_opt}\""
+	}
+	mi_gdb_test "set ${command} off" \
+	    "\\&\"set ${command} off\\\\n\"\r\n\\^done" \
+	    "\"set ${command}\" no event"
+    }
+
     mi_gdb_exit
 }}
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 8c8d4a8..d09faf8 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -5344,17 +5344,17 @@ trace data collected in the meantime."),
 			   &setlist,
 			   &showlist);
 
-  add_setshow_boolean_cmd ("circular-trace-buffer", no_class,
-			   &circular_trace_buffer, _("\
+  add_setshow_boolean_cmd_with_notif ("circular-trace-buffer", no_class,
+				      &circular_trace_buffer, _("\
 Set target's use of circular trace buffer."), _("\
 Show target's use of circular trace buffer."), _("\
 Use this to make the trace buffer into a circular buffer,\n\
 which will discard traceframes (oldest first) instead of filling\n\
 up and stopping the trace run."),
-			   set_circular_trace_buffer,
-			   NULL,
-			   &setlist,
-			   &showlist);
+				      set_circular_trace_buffer,
+				      NULL,
+				      &setlist,
+				      &showlist);
 
   add_setshow_string_cmd ("trace-user", class_trace,
 			  &trace_user, _("\
-- 
1.7.7.6

Re: [PATCH 5/6] new add_setshow_boolean_cmd_with_notify and circular-trace-buffer.

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> Similar to previous patch, this patch is to add a new function
Yao> add_setshow_boolean_cmd_with_notif, and register command
Yao> "circular-trace-buffer" as an example to boolean command.  A new
Yao> test case is added for it.

Thanks.

This one is ok.

Tom