[PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Yao Qi]

As design, =breakpoint-modified isn't emitted when breakpoints are modified
by MI commands.  This patch is to add tests for this.

gdb/testsuite:

2014-01-24  Yao Qi  <yao@codesourcery.com>

	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Test
	that no =breakpoint-modified is emitted when breakpoints are
	modified through MI commands.
---
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index cb2f7f6..aa991cf 100644
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -104,6 +104,9 @@ proc test_insert_delete_modify { } {
     mi_gdb_test $test \
 	{.*=breakpoint-modified,bkpt=\{number="2",.*,cond=\"main > 0x0\".*\}.*\n\^done} \
 	$test
+    # Modify condition through MI command shouldn't trigger MI notification.
+    mi_gdb_test "-break-condition 2 main == 0x0" "\\^done" \
+	"-break-condition 2 main == 0x0"
 
     # 3. when modifying enableness
     set test "disable 3"
@@ -114,16 +117,30 @@ proc test_insert_delete_modify { } {
     mi_gdb_test $test \
 	{.*=breakpoint-modified,bkpt=\{number="3",.*,enabled=\"y\".*\}.*\n\^done} \
 	$test
+    # Modify enableness through MI commands shouldn't trigger MI
+    # notification.
+    mi_gdb_test "-break-enable 3" "\\^done" "-break-enable 3"
+    mi_gdb_test "-break-disable 3" "\\^done" "-break-disable 3"
+
     # 4. when modifying ignore count.
     set test "ignore 5 1"
     mi_gdb_test $test \
 	{.*=breakpoint-modified,bkpt=\{number="5",.*,ignore=\"1\".*\}.*\n\^done} \
 	$test
+    # Modify ignore count through MI command shouldn't trigger MI
+    # notification.
+    mi_gdb_test "-break-after 5 1" "\\^done" \
+	"-break-after 5 1"
+
     # 5. when modifying pass count.
     set test "passcount 1 4"
     mi_gdb_test $test \
 	{.*=breakpoint-modified,bkpt=\{number="4",.*pass="1".*\}.*\n\^done} \
 	$test
+    # Modify pass count through MI command shouldn't trigger MI
+    # notification.
+    mi_gdb_test "-break-passcount 4 1" "\\^done" \
+	"-break-passcount 4 1"
 
     # Delete some breakpoints and verify that '=breakpoint-deleted
     # notification is correctly emitted.
-- 
1.7.7.6

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Yao Qi]

On 01/24/2014 03:46 PM, Yao Qi wrote:
> As design, =breakpoint-modified isn't emitted when breakpoints are modified
> by MI commands.  This patch is to add tests for this.
> 
> gdb/testsuite:
> 
> 2014-01-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Test
> 	that no =breakpoint-modified is emitted when breakpoints are
> 	modified through MI commands.

Ping.  https://sourceware.org/ml/gdb-patches/2014-01/msg00915.html

-- 
Yao (齐尧)

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[andre]

On Thu, Feb 06, 2014 at 08:36:00PM +0800, Yao Qi wrote:
> On 01/24/2014 03:46 PM, Yao Qi wrote:
> > As design, =breakpoint-modified isn't emitted when breakpoints are modified
> > by MI commands.  This patch is to add tests for this.
> > 
> > gdb/testsuite:
> > 
> > 2014-01-24  Yao Qi  <yao@codesourcery.com>
> > 
> > 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Test
> > 	that no =breakpoint-modified is emitted when breakpoints are
> > 	modified through MI commands.
> 
> Ping.  https://sourceware.org/ml/gdb-patches/2014-01/msg00915.html

I understand the patch, but I do not understand the motivation behind
the design decision to not send notifications when the change is 
triggered by an MI command.

Let's assume a frontend wants to have a functional gdb "console" embedded,
and the frontend in general runs in MI mode.

A trivial implementation would pass user input unfiltered to gdb. A user
can choose to type 'disable 1', or, with the same effect on gdb internal
state, '-break-disable 1'. In the first case, the frontend currently will
get a notification, in the second case it will not.

To get reliable information about the actual gdb internal state in this
setup there are essentially three choices for a frontend developer:

 - prevent the user from entering MI commands in the console
   (and try to catch all possible workarounds to sneak in MI
   commands nevertheless),

 - pre-parse user input before sending it to gdb, try to recognize
   MI commands that are known to not produce notifications and
   interpret a ^done as "all fine according to whatever you currently
   think 'fine' means",

 - ignore =breakpoint-modified notification in general and try to get
   full information using other means.

None of these options is desirable. None would be needed if gdb in
MI mode would simply announce all (non-internal) breakpoint modifications
with =breakpoint-modified notifications.

I would pretty much prefer notifications on all breakpoint changes in
MI mode, no matter whether they are initiated by an MI or a non-MI command.

Andre'

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Yao Qi]

On 02/07/2014 04:39 AM, andre wrote:
> I understand the patch, but I do not understand the motivation behind
> the design decision to not send notifications when the change is 
> triggered by an MI command.
> 

The rationale is mentioned here
https://sourceware.org/ml/gdb-patches/2012-07/msg00487.html

In short, we want MI frontend is aware of GDB's state changes, such as
breakpoint changes.  If requested changes are from MI, MI frontend
should be aware of them, so no notifications are sent.

> Let's assume a frontend wants to have a functional gdb "console" embedded,
> and the frontend in general runs in MI mode.
> 
> A trivial implementation would pass user input unfiltered to gdb. A user
> can choose to type 'disable 1', or, with the same effect on gdb internal
> state, '-break-disable 1'. In the first case, the frontend currently will
> get a notification, in the second case it will not.
> 
> To get reliable information about the actual gdb internal state in this
> setup there are essentially three choices for a frontend developer:
> 
>  - prevent the user from entering MI commands in the console
>    (and try to catch all possible workarounds to sneak in MI
>    commands nevertheless),
> 
>  - pre-parse user input before sending it to gdb, try to recognize
>    MI commands that are known to not produce notifications and
>    interpret a ^done as "all fine according to whatever you currently
>    think 'fine' means",
> 
>  - ignore =breakpoint-modified notification in general and try to get
>    full information using other means.
> 
> None of these options is desirable. None would be needed if gdb in
> MI mode would simply announce all (non-internal) breakpoint modifications
> with =breakpoint-modified notifications.

#1 is fine to me.  It shouldn't be hard to do that.  On the other hand,
I don't understand users have to input MI commands in console, which
is provided to accept CLI commands.

> 
> I would pretty much prefer notifications on all breakpoint changes in
> MI mode, no matter whether they are initiated by an MI or a non-MI command.

That would be too noisy in some cases.

-- 
Yao (齐尧)

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[andre]

On Fri, Feb 07, 2014 at 05:12:06PM +0800, Yao Qi wrote:
> On 02/07/2014 04:39 AM, andre wrote:
> > I understand the patch, but I do not understand the motivation behind
> > the design decision to not send notifications when the change is 
> > triggered by an MI command.
> > 
> 
> The rationale is mentioned here
> https://sourceware.org/ml/gdb-patches/2012-07/msg00487.html

Sure, but I don't understand it. You argue that it is necessary for a
frontend to keep track of changes triggered by manual user interaction.
That's fine. But it is unclear why this reasoning should only be 
applied to non-MI commands, and not for MI commands that a user enters
manually (and that gdb happily accepts).
 
> In short, we want MI frontend is aware of GDB's state changes, such as
> breakpoint changes.  If requested changes are from MI, MI frontend
> should be aware of them, so no notifications are sent.
> > To get reliable information about the actual gdb internal state in this
> > setup there are essentially three choices for a frontend developer:
> > 
> >  - prevent the user from entering MI commands in the console
> >    (and try to catch all possible workarounds to sneak in MI
> >    commands nevertheless),
> > 
> >  - pre-parse user input before sending it to gdb, try to recognize
> >    MI commands that are known to not produce notifications and
> >    interpret a ^done as "all fine according to whatever you currently
> >    think 'fine' means",
> > 
> >  - ignore =breakpoint-modified notification in general and try to get
> >    full information using other means.
> > 
> > None of these options is desirable. None would be needed if gdb in
> > MI mode would simply announce all (non-internal) breakpoint modifications
> > with =breakpoint-modified notifications.
> 
> #1 is fine to me.

But not for me. I don't want to needlessly restrict what my users are
allowed to type, or not. It might even e.g. be useful to copy/paste/modify 
previously sent MI commands and send them manually via the console. In your 
approach this would not be possible, or at least require the user to
re-write the full command in non-MI syntax. 

> It shouldn't be hard to do that.  On the other hand,
> I don't understand users have to input MI commands in console, which
> is provided to accept CLI commands.

The point is not that they should _have to_ use MI syntax, but that they
_could_ if they wish.

> > I would pretty much prefer notifications on all breakpoint changes in
> > MI mode, no matter whether they are initiated by an MI or a non-MI command.
> 
> That would be too noisy in some cases.

It's easier for a frontend to filter out messages (especially if they
are expected) than to act on messages that are not send. And it's not
that we are talking about millions of notifications here that might
the communication line.`

Andre'

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Joel Brobecker]

> > >  - prevent the user from entering MI commands in the console
> > >    (and try to catch all possible workarounds to sneak in MI
> > >    commands nevertheless),
[...]
> > #1 is fine to me.
> 
> But not for me. I don't want to needlessly restrict what my users are
> allowed to type, or not. It might even e.g. be useful to copy/paste/modify 
> previously sent MI commands and send them manually via the console. In your 
> approach this would not be possible, or at least require the user to
> re-write the full command in non-MI syntax. 

I don't feel strongly about it, but I kind of see Andre's point.

If we can allow a certain type of usage without damaging consequences
for the rest of the operations, why not? Wouldn't it simplify the
notification mechanism too?

Food for thought:

I think it would be interesting to investigate whether FEs would
notice if they started receiving those extra notifications. I hope
the processing would be fast enough that they wouldn't.

One other possible option: Add a new option that would be available
to all commands to disable notifications related to the command being
executed. That way, FE could use it to reduce unnecessary back-chatter.
I don't really like that option, though, as it would require a certain
transition period.

-- 
Joel

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Yao Qi]

On 02/08/2014 11:18 AM, Joel Brobecker wrote:
> If we can allow a certain type of usage without damaging consequences
> for the rest of the operations, why not? Wouldn't it simplify the
> notification mechanism too?
> 

I am not familiar with the internals of FE, such as Eclipse, so hard to
tell change like this can break FE or not, but ...

> Food for thought:
> 
> I think it would be interesting to investigate whether FEs would
> notice if they started receiving those extra notifications. I hope
> the processing would be fast enough that they wouldn't.

... as you said, the investigation to FE should be useful to this
discussion.

> 
> One other possible option: Add a new option that would be available
> to all commands to disable notifications related to the command being
> executed. That way, FE could use it to reduce unnecessary back-chatter.

That is what I am thinking about.

> I don't really like that option, though, as it would require a certain
> transition period.

What do you mean by "transition period"?  We can make use of
"-list-features" to tell FE that FE can disable/enable MI notifications
through
a certain command.

-- 
Yao (齐尧)

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Joel Brobecker]

> > One other possible option: Add a new option that would be available
> > to all commands to disable notifications related to the command being
> > executed. That way, FE could use it to reduce unnecessary back-chatter.
> 
> That is what I am thinking about.
> 
> > I don't really like that option, though, as it would require a certain
> > transition period.
> 
> What do you mean by "transition period"?  We can make use of
> "-list-features" to tell FE that FE can disable/enable MI notifications
> through a certain command.

The issue is people using older versions of an FE with a newer version
of GDB. For those, their FE wouldn't know about the new option and
thus get the notifications that they might not expect.

I don't know if we need to be concerned about this sort of compatibility
or not...

-- 
Joel

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[andre]

On Sat, Feb 08, 2014 at 05:15:33PM +0400, Joel Brobecker wrote:
> > > One other possible option: Add a new option that would be available
> > > to all commands to disable notifications related to the command being
> > > executed. That way, FE could use it to reduce unnecessary back-chatter.
> > 
> > That is what I am thinking about.

I don't think the amout of information discussed here is worth any special
action on neither gdb's nor a frontend's side. Normal operation produce
"chatter" e.g. for library load/unload notifications on a much bigger
scale.

> > > I don't really like that option, though, as it would require a
> > > certain transition period.
> > 
> > What do you mean by "transition period"?  We can make use of
> > "-list-features" to tell FE that FE can disable/enable MI notifications
> > through a certain command.
> 
> The issue is people using older versions of an FE with a newer version of
> GDB. For those, their FE wouldn't know about the new option and thus get
> the notifications that they might not expect.

This happened regularly with other new notifications in the past
so I would expect frontends to be able to handle new notifications
gracefully. In this case "new" is even relative as the notifications
are sent in most circumstances already anyway.
 
> I don't know if we need to be concerned about this sort of compatibility
> or not...

Since it is not a concern when introducing new notifications like
=cmd-param-changed, =memory-changed or even =breakpoint-modified
itself and frontends need to handle the case of "unexpected" 
notifications anyway, it's hard for me to see how sending a specific
notification in all cases instead of in "most" cases can do harm.

Andre'

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Pedro Alves]

On 01/24/2014 07:46 AM, Yao Qi wrote:
> As design, =breakpoint-modified isn't emitted when breakpoints are modified
> by MI commands.  This patch is to add tests for this.
> 
> gdb/testsuite:
> 
> 2014-01-24  Yao Qi  <yao@codesourcery.com>
> 
> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Test
> 	that no =breakpoint-modified is emitted when breakpoints are
> 	modified through MI commands.

OK.

Thanks,
-- 
Pedro Alves

Re: [PATCH] Test no =breakpoint-modified is emitted for modifications from MI commands

[Yao Qi]

On 02/07/2014 10:04 PM, Pedro Alves wrote:
> On 01/24/2014 07:46 AM, Yao Qi wrote:
>> As design, =breakpoint-modified isn't emitted when breakpoints are modified
>> by MI commands.  This patch is to add tests for this.
>>
>> gdb/testsuite:
>>
>> 2014-01-24  Yao Qi  <yao@codesourcery.com>
>>
>> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): Test
>> 	that no =breakpoint-modified is emitted when breakpoints are
>> 	modified through MI commands.
> 
> OK.
> 
> Thanks,
> 

Thanks for review, Pedro.  Patch is pushed.

-- 
Yao (齐尧)