public inbox for gdb@sourceware.org
 help / color / mirror / Atom feed
* MI3 and async notifications
@ 2019-06-10 21:19 Jan Vrany
  2019-06-10 23:23 ` Jonah Graham
                   ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Jan Vrany @ 2019-06-10 21:19 UTC (permalink / raw)
  To: gdb

Hello,

as an user og GDB/MI (and frontent developer), I'd like to 
open a discussion about one aspect of MI that I'd like to change
in MI3 before it is released into the wild. 

Currently, quite a few commands suppress async notifications when
a command is issued through MI. For example, when a breakpoint is 
added using -break-insert then =breakpoint-created is not emitted.

At least in my case, this behavior complicates client design because
now the client has to care for new breakpoints on multiple places:
(i)  listen to breakpoint created events to handle cases where breakpoint
     is added via CLI. 
(ii) do similar processing whenever MI returns ^done for previously 
     issued -break-insert command.
 
This leads to a complex logic, especially in cases where frontend has
some scripting support (so execution of MI commands is not completely
under frontend developer's control). 

Emitting notifications unconditionally would simplify things a lot 
- again at least in my case. 

Therefore I'd like to propose a change for MI3 to always send notifications. 
If such a change would make things complicated for other frontends 
(Eclipse CDT / Emacs come to mind), I propose new

-gdb-set mi-always-notify 1
-gdb-set mi-always-notify 0

setting to control the behavior so frontends may choose. 

I'm happy to submit a patch. Are there any other frontend developers?
If so, would it be OK to always notify in MI3 or do you prefer to have
an option? Any other comments? 

Thanks! Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-10 21:19 MI3 and async notifications Jan Vrany
@ 2019-06-10 23:23 ` Jonah Graham
  2019-06-11  8:50   ` Jan Vrany
  2019-06-15 14:34 ` Tom Tromey
  2019-06-18  3:14 ` Simon Marchi
  2 siblings, 1 reply; 26+ messages in thread
From: Jonah Graham @ 2019-06-10 23:23 UTC (permalink / raw)
  To: Jan Vrany; +Cc: gdb

On Mon, 10 Jun 2019 at 17:19, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:

> Therefore I'd like to propose a change for MI3 to always send
> notifications.
> If such a change would make things complicated for other frontends
> (Eclipse CDT / Emacs come to mind), I propose new
>
> -gdb-set mi-always-notify 1
> -gdb-set mi-always-notify 0
>

Thank you for considering the other front end consumers of MI. I am one of
the current maintainers of CDT so I will share my 2cents.

Eclipse CDT would certainly require such a flag, but only if MI3 was a
replacement for MI2. If CDT can continue to use gdb in mi2 mode (CDT
launches gdb with --interpreter mi2 [1]) then I don't think you need to
carry on the extra logic in MI3. I haven't followed the discussions on MI3
closely.

I assume from the proposal that the -break-insert still gets the done
message with the breakpoint number in it? And does the async message come
back after the the ^done? If it does not come after the done CDT will have
to hold processing the async message until after it finds out if the
=breakpoint-created was for the MI or CLI inserted breakpoint (consider the
race condition that a user / script inserts a breakpoint from the CLI at
the same time as from the MI).

I hope that helps from CDT perspective.

Jonah

[1]
https://github.com/eclipse-cdt/cdt/blob/7741bd98f7b08a281c4b7f60e60c5839f315f760/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBBackend.java#L188

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-10 23:23 ` Jonah Graham
@ 2019-06-11  8:50   ` Jan Vrany
  2019-06-11 13:37     ` Jonah Graham
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Vrany @ 2019-06-11  8:50 UTC (permalink / raw)
  To: gdb; +Cc: Jonah Graham

On Mon, 2019-06-10 at 19:22 -0400, Jonah Graham wrote:
> On Mon, 10 Jun 2019 at 17:19, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:
> 
> > Therefore I'd like to propose a change for MI3 to always send
> > notifications.
> > If such a change would make things complicated for other frontends
> > (Eclipse CDT / Emacs come to mind), I propose new
> > 
> > -gdb-set mi-always-notify 1
> > -gdb-set mi-always-notify 0
> > 
> 
> Thank you for considering the other front end consumers of MI. I am one of
> the current maintainers of CDT so I will share my 2cents.

Thanks!

> 
> Eclipse CDT would certainly require such a flag, but only if MI3 was a
> replacement for MI2. If CDT can continue to use gdb in mi2 mode (CDT
> launches gdb with --interpreter mi2 [1]) then I don't think you need to
> carry on the extra logic in MI3. I haven't followed the discussions on MI3
> closely.

I don't know what are the plans exactly either. My idea was to change it for
MI3 only, so if you do `--interpreter mi2`, nothing will change for you. 

However I guess you may want to use MI3 in future as it will be the default
(I might be wrong but as of current master, MI3 is the default). 
Not breaking CDT is a (very) strong argument, so I think we need the option. It's not
that hard to implement. 

> 
> I assume from the proposal that the -break-insert still gets the done
> message with the breakpoint number in it? And does the async message come
> back after the the ^done? If it does not come after the done CDT will have
> to hold processing the async message until after it finds out if the
> =breakpoint-created was for the MI or CLI inserted breakpoint 

Yes, you will get ^done with all the data as before, only that before ^done
you'll get =breakpoint-created event (with the same data). 

I modified the code to always send notifications in MI3 but not MI2 (patch below),  
here's an example session to demonstrate the difference on -break-insert:

MI2 (current)

./gdb -i=mi2 ./testsuite/outputs/gdb.python/py-msymbol/py-msymbol
=thread-group-added,id="i1"
~"GNU gdb (GDB) 8.3.50.20190611-git\n"
~"Copyright (C) 2019 Free Software Foundation, Inc.\n"
~"License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>\nThis is free software: you are free to change and redistribute it.\nThere is NO WARRANTY, to the extent permitted by
law."
~"\nType \"show copying\" and \"show warranty\" for details.\n"
~"This GDB was configured as \"x86_64-pc-linux-gnu\".\n"
~"Type \"show configuration\" for configuration details.\n"
~"For bug reporting instructions, please see:\n"
~"<http://www.gnu.org/software/gdb/bugs/>.\n"
~"Find the GDB manual and other documentation resources online at:\n    <http://www.gnu.org/software/gdb/documentation/>;."
~"\n\n"
~"For help, type \"help\".\n"
~"Type \"apropos word\" to search for commands related to \"word\"...\n"
~"Reading symbols from ./testsuite/outputs/gdb.python/py-msymbol/py-msymbol...\n"
(gdb) 
-break-insert main
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000001151",func="main",file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.python/py-
symbol.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.python/py-symbol.c",line="56",thread-groups=["i1"],times="0",original-location="main"}
(gdb) 
quit
&"quit\n"

MI3 with modification (proposed)

jv@sao:~/Projects/gdb/users_jv_patches/gdb$ ./gdb -i=mi3 ./testsuite/outputs/gdb.python/py-msymbol/py-msymbol
=thread-group-added,id="i1"
~"GNU gdb (GDB) 8.3.50.20190611-git\n"
~"Copyright (C) 2019 Free Software Foundation, Inc.\n"
~"License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>\nThis is free software: you are free to change and redistribute it.\nThere is NO WARRANTY, to the extent permitted by
law."
~"\nType \"show copying\" and \"show warranty\" for details.\n"
~"This GDB was configured as \"x86_64-pc-linux-gnu\".\n"
~"Type \"show configuration\" for configuration details.\n"
~"For bug reporting instructions, please see:\n"
~"<http://www.gnu.org/software/gdb/bugs/>.\n"
~"Find the GDB manual and other documentation resources online at:\n    <http://www.gnu.org/software/gdb/documentation/>;."
~"\n\n"
~"For help, type \"help\".\n"
~"Type \"apropos word\" to search for commands related to \"word\"...\n"
~"Reading symbols from ./testsuite/outputs/gdb.python/py-msymbol/py-msymbol...\n"
(gdb) 
-break-insert main
=breakpoint-created,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000001151",func="main",file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.python/py-
symbol.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.python/py-symbol.c",line="56",thread-groups=["i1"],times="0",original-location="main"}
^done,bkpt={number="1",type="breakpoint",disp="keep",enabled="y",addr="0x0000000000001151",func="main",file="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.python/py-
symbol.c",fullname="/home/jv/Projects/gdb/users_jv_patches/gdb/testsuite/gdb.python/py-symbol.c",line="56",thread-groups=["i1"],times="0",original-location="main"}
(gdb) 
quit
&"quit\n"

As you can see, you get the notification *before* ^done response. Does that answer 
your questions?


> (consider the
> race condition that a user / script inserts a breakpoint from the CLI at
> the same time as from the MI).

I see. 

> 
> I hope that helps from CDT perspective.
> 

Sure, thanks! So it looks me a new option is the way to go. 

Jan

--
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 13c310d494..2f2d1f2612 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -1954,7 +1954,8 @@ mi_execute_command (const char *cmd, int from_tty)
 
       gdb::optional<scoped_restore_tmpl<int>> restore_suppress;
 
-      if (command->cmd != NULL && command->cmd->suppress_notification != NULL)
+      if (command->cmd != NULL && command->cmd->suppress_notification != NULL
+          && mi_version(current_uiout) < 3)
        restore_suppress.emplace (command->cmd->suppress_notification, 1);
 
       command->token = token;

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-11  8:50   ` Jan Vrany
@ 2019-06-11 13:37     ` Jonah Graham
  2019-07-05 20:00       ` Pedro Alves
  0 siblings, 1 reply; 26+ messages in thread
From: Jonah Graham @ 2019-06-11 13:37 UTC (permalink / raw)
  To: Jan Vrany; +Cc: gdb

On Tue, 11 Jun 2019 at 04:50, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:

> As you can see, you get the notification *before* ^done response. Does
> that answer
> your questions?
>
>
Yes that does. I think CDT will use the mi-always-notify option.

Jonah

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-10 21:19 MI3 and async notifications Jan Vrany
  2019-06-10 23:23 ` Jonah Graham
@ 2019-06-15 14:34 ` Tom Tromey
  2019-06-17 10:53   ` Jan Vrany
  2019-06-18  3:14 ` Simon Marchi
  2 siblings, 1 reply; 26+ messages in thread
From: Tom Tromey @ 2019-06-15 14:34 UTC (permalink / raw)
  To: Jan Vrany; +Cc: gdb

>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

Jan> as an user og GDB/MI (and frontent developer), I'd like to 
Jan> open a discussion about one aspect of MI that I'd like to change
Jan> in MI3 before it is released into the wild. 
...
Jan> Emitting notifications unconditionally would simplify things a lot 
Jan> - again at least in my case. 

It seems like a good idea to me.  I wonder if it makes sense to go even
further and say there will only be async notifications for things like
this.

Tom

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-15 14:34 ` Tom Tromey
@ 2019-06-17 10:53   ` Jan Vrany
  2019-06-17 12:11     ` Jonah Graham
                       ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Jan Vrany @ 2019-06-17 10:53 UTC (permalink / raw)
  To: Tom Tromey, Jonah Graham; +Cc: gdb

On Sat, 2019-06-15 at 08:34 -0600, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
> 
> Jan> as an user og GDB/MI (and frontent developer), I'd like to 
> Jan> open a discussion about one aspect of MI that I'd like to change
> Jan> in MI3 before it is released into the wild. 
> ...
> Jan> Emitting notifications unconditionally would simplify things a lot 
> Jan> - again at least in my case. 
> 
> It seems like a good idea to me.  I wonder if it makes sense to go even
> further and say there will only be async notifications for things like
> this.

Yes, I thought the same initially. But then what about other existing MI
consumers? 

From what I understood from Jonah's comments earlier, this would break (at least)
CDT. So CDT would either have to stick with MI2 (not great in a long term)
or refactor their code (not sure CDT guys would be happy to do so, especially as 
- I presume - CDT needs to support wide range of GDB versions already in the wild, 
a problem I do not have). 

While I personally agree with you and will be happy to go that far, it'd break
existing consumers - something that should IMO be carefully discussed and planned. 

Adding a new option as I proposed as an alternative will be backward compatible,
indeed at the cost of more convoluted code in GDB itself. 

Is anyone from Emacs community around? Or any other MI consumers? 

Jan

> 
> Tom

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 10:53   ` Jan Vrany
@ 2019-06-17 12:11     ` Jonah Graham
  2019-06-17 12:14     ` Joel Brobecker
  2019-06-17 19:52     ` André Pönitz
  2 siblings, 0 replies; 26+ messages in thread
From: Jonah Graham @ 2019-06-17 12:11 UTC (permalink / raw)
  To: Jan Vrany; +Cc: Tom Tromey, gdb

On Mon, 17 Jun 2019 at 06:53, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:

> On Sat, 2019-06-15 at 08:34 -0600, Tom Tromey wrote:
>
> From what I understood from Jonah's comments earlier, this would break (at
> least)
> CDT. So CDT would either have to stick with MI2 (not great in a long term)
> or refactor their code (not sure CDT guys would be happy to do so,
> especially as
> - I presume - CDT needs to support wide range of GDB versions already in
> the wild,
> a problem I do not have).
>

Yes, you have understood correctly. CDT currently supports GDB 7.5+.

Jonah

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 10:53   ` Jan Vrany
  2019-06-17 12:11     ` Jonah Graham
@ 2019-06-17 12:14     ` Joel Brobecker
  2019-06-17 12:26       ` Jonah Graham
  2019-06-17 19:52     ` André Pönitz
  2 siblings, 1 reply; 26+ messages in thread
From: Joel Brobecker @ 2019-06-17 12:14 UTC (permalink / raw)
  To: Jan Vrany; +Cc: Tom Tromey, Jonah Graham, gdb

> > It seems like a good idea to me.  I wonder if it makes sense to go even
> > further and say there will only be async notifications for things like
> > this.
> 
> Yes, I thought the same initially. But then what about other existing MI
> consumers? 
> 
> >From what I understood from Jonah's comments earlier, this would break (at least)
> CDT. So CDT would either have to stick with MI2 (not great in a long
> term) or refactor their code (not sure CDT guys would be happy to do
> so, especially as - I presume - CDT needs to support wide range of GDB
> versions already in the wild, a problem I do not have). 
> 
> While I personally agree with you and will be happy to go that far,
> it'd break existing consumers - something that should IMO be carefully
> discussed and planned. 
> 
> Adding a new option as I proposed as an alternative will be backward
> compatible, indeed at the cost of more convoluted code in GDB itself. 
> 
> Is anyone from Emacs community around? Or any other MI consumers? 

My 2 cents:

In my opinion, while I think upward compatibility is very important,
it is also important to avoid having too many configurability options.
Otherwise, we end up with a large number of options and the testing
matrix, if we want to verify that they work well together, quickly
explodes.

In this case, because we have MI versions, and because the notification
shouldn't be different from the data of the "^done" message, I think
the incompatibility would be acceptable -- assuming existing parsers
don't come back to say that it actually is a large effort for them
to adapt.

-- 
Joel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 12:14     ` Joel Brobecker
@ 2019-06-17 12:26       ` Jonah Graham
  2019-06-17 12:56         ` Joel Brobecker
  0 siblings, 1 reply; 26+ messages in thread
From: Jonah Graham @ 2019-06-17 12:26 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Jan Vrany, Tom Tromey, gdb

On Mon, 17 Jun 2019 at 08:14, Joel Brobecker <brobecker@adacore.com> wrote:

> > > It seems like a good idea to me.  I wonder if it makes sense to go even
> > > further and say there will only be async notifications for things like
> > > this.
> >
> > Yes, I thought the same initially. But then what about other existing MI
> > consumers?
>


> In my opinion, while I think upward compatibility is very important,
> it is also important to avoid having too many configurability options.
> Otherwise, we end up with a large number of options and the testing
> matrix, if we want to verify that they work well together, quickly
> explodes.
>
> In this case, because we have MI versions, and because the notification
> shouldn't be different from the data of the "^done" message, I think
> the incompatibility would be acceptable -- assuming existing parsers
> don't come back to say that it actually is a large effort for them
> to adapt.
>
>
I do agree, avoid the extra configurability - but I simply don't know how
to work with just async notifications to sync messages. It means that CDT
will have to issues the -break-insert, look for the done message and
"search" between them to find the =breakpoint-created that matched and
separately process any that don't. Please see my earlier message about how
to handle race condition between -break-inserts over MI and breaks inserted
from CLI. This race condition does not happen during normal operation
(where a human is driving everything) but does kick in during many
semi-automated flows. Perhaps this isn't a big problem, but to me it seems
the logic to match up -break-insert to =breakpoint-created in client side
is complex and bug prone.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 12:26       ` Jonah Graham
@ 2019-06-17 12:56         ` Joel Brobecker
  2019-06-17 13:12           ` Jan Vrany
  2019-06-17 13:12           ` Jonah Graham
  0 siblings, 2 replies; 26+ messages in thread
From: Joel Brobecker @ 2019-06-17 12:56 UTC (permalink / raw)
  To: Jonah Graham; +Cc: Jan Vrany, Tom Tromey, gdb

> I do agree, avoid the extra configurability - but I simply don't know how
> to work with just async notifications to sync messages. It means that CDT
> will have to issues the -break-insert, look for the done message and
> "search" between them to find the =breakpoint-created that matched and
> separately process any that don't. Please see my earlier message about how
> to handle race condition between -break-inserts over MI and breaks inserted
> from CLI. This race condition does not happen during normal operation
> (where a human is driving everything) but does kick in during many
> semi-automated flows. Perhaps this isn't a big problem, but to me it seems
> the logic to match up -break-insert to =breakpoint-created in client side
> is complex and bug prone.

The part I don't understand is why it matters to sync the two.

-- 
Joel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 12:56         ` Joel Brobecker
@ 2019-06-17 13:12           ` Jan Vrany
  2019-06-17 13:23             ` Jonah Graham
  2019-06-17 13:12           ` Jonah Graham
  1 sibling, 1 reply; 26+ messages in thread
From: Jan Vrany @ 2019-06-17 13:12 UTC (permalink / raw)
  To: Joel Brobecker, Jonah Graham; +Cc: Tom Tromey, gdb

On Mon, 2019-06-17 at 08:56 -0400, Joel Brobecker wrote:
> > I do agree, avoid the extra configurability - but I simply don't know how
> > to work with just async notifications to sync messages. It means that CDT
> > will have to issues the -break-insert, look for the done message and
> > "search" between them to find the =breakpoint-created that matched and
> > separately process any that don't. Please see my earlier message about how
> > to handle race condition between -break-inserts over MI and breaks inserted
> > from CLI. This race condition does not happen during normal operation
> > (where a human is driving everything) but does kick in during many
> > semi-automated flows. Perhaps this isn't a big problem, but to me it seems
> > the logic to match up -break-insert to =breakpoint-created in client side
> > is complex and bug prone.
> 
> The part I don't understand is why it matters to sync the two.
> 
Jonah, I was about to ask the same. I understand that you need to know
which breakpoint has been inserted by given command, but this
if we respond with something like

1-break-insert main
=breakpoint-created,bkpt={number="1",type=...}
1^done,bkpt-number=1

then you just search for breakpoint with that id, no? Given that MI 
guarantees that =breakpoint-created arrives before ^done reply to command.
Am I missing something? 

Also note that this is not only about breakpoints but also about things like
-gdb-set and few others. 

Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 12:56         ` Joel Brobecker
  2019-06-17 13:12           ` Jan Vrany
@ 2019-06-17 13:12           ` Jonah Graham
  1 sibling, 0 replies; 26+ messages in thread
From: Jonah Graham @ 2019-06-17 13:12 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Jan Vrany, Tom Tromey, gdb

On Mon, 17 Jun 2019 at 08:56, Joel Brobecker <brobecker@adacore.com> wrote:

> The part I don't understand is why it matters to sync the two.
>

For example, to allow program to update the UI element that represents a
breakpoint. In CDT when a breakpoint is inserted in the UI and fails to be
installed or is only pending the UI element is updated to display that
fact. CDT maintains a map of UI elements to GDB instances+breakpoint
numbers. That connection (UI element to breakpoint number) is made as
result of the response to -break-insert.

The UI elements are breakpoints in a table, or in the margin of an editor.

Does that make sense?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 13:12           ` Jan Vrany
@ 2019-06-17 13:23             ` Jonah Graham
  2019-06-17 20:45               ` Joel Brobecker
  0 siblings, 1 reply; 26+ messages in thread
From: Jonah Graham @ 2019-06-17 13:23 UTC (permalink / raw)
  To: Jan Vrany; +Cc: Joel Brobecker, Tom Tromey, gdb

On Mon, 17 Jun 2019 at 09:12, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:

> On Mon, 2019-06-17 at 08:56 -0400, Joel Brobecker wrote:
> > > I do agree, avoid the extra configurability - but I simply don't know
> how
> > > to work with just async notifications to sync messages. It means that
> CDT
> > > will have to issues the -break-insert, look for the done message and
> > > "search" between them to find the =breakpoint-created that matched and
> > > separately process any that don't. Please see my earlier message about
> how
> > > to handle race condition between -break-inserts over MI and breaks
> inserted
> > > from CLI. This race condition does not happen during normal operation
> > > (where a human is driving everything) but does kick in during many
> > > semi-automated flows. Perhaps this isn't a big problem, but to me it
> seems
> > > the logic to match up -break-insert to =breakpoint-created in client
> side
> > > is complex and bug prone.
> >
> > The part I don't understand is why it matters to sync the two.
> >
> Jonah, I was about to ask the same. I understand that you need to know
> which breakpoint has been inserted by given command, but this
> if we respond with something like
>
> 1-break-insert main
> =breakpoint-created,bkpt={number="1",type=...}
> 1^done,bkpt-number=1
>
> then you just search for breakpoint with that id, no? Given that MI
> guarantees that =breakpoint-created arrives before ^done reply to command.
> Am I missing something?
>

No you aren't missing anything. That would be a perfectly acceptable
solution for CDT.

There would still be some other new logic needed for CDT, we would still
have to store all the =breakpoint-created if there is a -break-insert
active and then process all of them when the ^done is received. However
that seems fairly reasonable.



>
> Also note that this is not only about breakpoints but also about things
> like
> -gdb-set and few others.
>

?? This is down to me not following closely enough. If you can point me in
the right direction then I can provide feedback now. If not it will have to
wait until if/when CDT needs to update to MI3 support. However there are no
gdb-sets that are likely to have any such issues as there is little to no
UI elements connected to gdb-sets. CDT does all of its gdb-sets at startup
to set the environment and then does not normally change any of them.

Thanks,
Jonah

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 10:53   ` Jan Vrany
  2019-06-17 12:11     ` Jonah Graham
  2019-06-17 12:14     ` Joel Brobecker
@ 2019-06-17 19:52     ` André Pönitz
  2 siblings, 0 replies; 26+ messages in thread
From: André Pönitz @ 2019-06-17 19:52 UTC (permalink / raw)
  To: Jan Vrany; +Cc: Tom Tromey, Jonah Graham, gdb

On Mon, Jun 17, 2019 at 11:53:14AM +0100, Jan Vrany wrote:
> On Sat, 2019-06-15 at 08:34 -0600, Tom Tromey wrote:
> > > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:
> > 
> > Jan> as an user og GDB/MI (and frontent developer), I'd like to Jan>
> > open a discussion about one aspect of MI that I'd like to change Jan> in
> > MI3 before it is released into the wild.  ...  Jan> Emitting
> > notifications unconditionally would simplify things a lot Jan> - again
> > at least in my case. 
> > 
> > It seems like a good idea to me.  I wonder if it makes sense to go even
> > further and say there will only be async notifications for things like
> > this.
> 
> Yes, I thought the same initially. But then what about other existing MI
> consumers? 
> 
> >From what I understood from Jonah's comments earlier, this would break
> >(at least)
> CDT. So CDT would either have to stick with MI2 (not great in a long term)
> or refactor their code (not sure CDT guys would be happy to do so,
> especially as - I presume - CDT needs to support wide range of GDB
> versions already in the wild, a problem I do not have). 
> 
> While I personally agree with you and will be happy to go that far, it'd
> break existing consumers - something that should IMO be carefully
> discussed and planned. 
> 
> Adding a new option as I proposed as an alternative will be backward
> compatible, indeed at the cost of more convoluted code in GDB itself. 
> 
> Is anyone from Emacs community around? Or any other MI consumers? 

Some "other (partial) MI consumer" here. And thank you for considering that
possibility.

1. I also need to support a range of GDB versions (current lower limit is
7.4.1, but I guess I could bump that a bit if really needed)

2. Any kind of additional notification that does not change change 
sequencing or contents of messages of older versions sounds ok to me.

3. Output of gdb --version/show version, -list-features etc was in the past
insufficient for me for feature discovery, so I typically use
trial-and-error-and-fallback on some test input, or, in the areas where it
is available, use the Python interface instead of MI. Insofar, I do not need
backward compatibility in the sense that later versions need to continue to
provide a specific feature, it's typically ok if a feature/command is either
there or not. I believe completely removing features/command would be easier
for me to handle than certain cases where there features only slightly
change behaviour or need special setup to establish a "compatibility mode"
or such.


Andre'

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 13:23             ` Jonah Graham
@ 2019-06-17 20:45               ` Joel Brobecker
  2019-06-17 20:58                 ` Jan Vrany
  0 siblings, 1 reply; 26+ messages in thread
From: Joel Brobecker @ 2019-06-17 20:45 UTC (permalink / raw)
  To: Jonah Graham; +Cc: Jan Vrany, Tom Tromey, gdb

> > Jonah, I was about to ask the same. I understand that you need to know
> > which breakpoint has been inserted by given command, but this
> > if we respond with something like
> >
> > 1-break-insert main
> > =breakpoint-created,bkpt={number="1",type=...}
> > 1^done,bkpt-number=1
> >
> > then you just search for breakpoint with that id, no? Given that MI
> > guarantees that =breakpoint-created arrives before ^done reply to command.
> > Am I missing something?
> >
> 
> No you aren't missing anything. That would be a perfectly acceptable
> solution for CDT.
> 
> There would still be some other new logic needed for CDT, we would
> still have to store all the =breakpoint-created if there is a
> -break-insert active and then process all of them when the ^done is
> received. However that seems fairly reasonable.

Do we even need the bkpt-number=1 attribute in the "done" command?
The notification includes that information, so the GUI should have
enough info from there to determine which UI element to update, right?

-- 
Joel

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 20:45               ` Joel Brobecker
@ 2019-06-17 20:58                 ` Jan Vrany
  2019-06-17 21:50                   ` Jonah Graham
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Vrany @ 2019-06-17 20:58 UTC (permalink / raw)
  To: Joel Brobecker, Jonah Graham; +Cc: Tom Tromey, gdb, André Pönitz

On Mon, 2019-06-17 at 16:45 -0400, Joel Brobecker wrote:
> > > Jonah, I was about to ask the same. I understand that you need to know
> > > which breakpoint has been inserted by given command, but this
> > > if we respond with something like
> > > 
> > > 1-break-insert main
> > > =breakpoint-created,bkpt={number="1",type=...}
> > > 1^done,bkpt-number=1
> > > 
> > > then you just search for breakpoint with that id, no? Given that MI
> > > guarantees that =breakpoint-created arrives before ^done reply to command.
> > > Am I missing something?
> > > 
> > 
> > No you aren't missing anything. That would be a perfectly acceptable
> > solution for CDT.
> > 
> > There would still be some other new logic needed for CDT, we would
> > still have to store all the =breakpoint-created if there is a
> > -break-insert active and then process all of them when the ^done is
> > received. However that seems fairly reasonable.
> 
> Do we even need the bkpt-number=1 attribute in the "done" command?
> The notification includes that information, so the GUI should have
> enough info from there to determine which UI element to update, right?
> 

I think so. Imagine you have a UI with breakpoint list and there's 
an "Add Breakpoint" button. When "Add Breakpoint" is pressed, user
fills location, press "OK", new breakpoint is added and 
*UI pre-selects just added breakpoint*. This is IMO sensible UI behavior. 

Now, when you press "OK" to add the breakpoint on a location, behind the scenes
frontend issues -break-insert command and waits for ^done confirmation. 
But as Jonah mentioned, another breakpoint could be inserted meanwhile so
you'd get something like this on MI channel.

10-break-insert main
=breakpoint-created,bkpt={number="1",type=...<some unrelated breakpoint from CLI or script>}
=breakpoint-created,bkpt={number="2",type=...<the breakpoint on main you just requested>}
=breakpoint-created,bkpt={number="3",type=...<some unrelated breakpoint from CLI or script>}
10^done

Now, if ^done response would not contain bkpt-number=2 attribute, how would you know which
is the breakpoint you need to pre-select in the UI?

Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-17 20:58                 ` Jan Vrany
@ 2019-06-17 21:50                   ` Jonah Graham
  0 siblings, 0 replies; 26+ messages in thread
From: Jonah Graham @ 2019-06-17 21:50 UTC (permalink / raw)
  To: Jan Vrany; +Cc: Joel Brobecker, Tom Tromey, gdb, André Pönitz

On Mon, 17 Jun 2019 at 16:58, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:

> On Mon, 2019-06-17 at 16:45 -0400, Joel Brobecker wrote:
> > > > Jonah, I was about to ask the same. I understand that you need to
> know
> > > > which breakpoint has been inserted by given command, but this
> > > > if we respond with something like
> > > >
> > > > 1-break-insert main
> > > > =breakpoint-created,bkpt={number="1",type=...}
> > > > 1^done,bkpt-number=1
> > > >
> > > > then you just search for breakpoint with that id, no? Given that MI
> > > > guarantees that =breakpoint-created arrives before ^done reply to
> command.
> > > > Am I missing something?
> > > >
> > >
> > > No you aren't missing anything. That would be a perfectly acceptable
> > > solution for CDT.
> > >
> > > There would still be some other new logic needed for CDT, we would
> > > still have to store all the =breakpoint-created if there is a
> > > -break-insert active and then process all of them when the ^done is
> > > received. However that seems fairly reasonable.
> >
> > Do we even need the bkpt-number=1 attribute in the "done" command?
> > The notification includes that information, so the GUI should have
> > enough info from there to determine which UI element to update, right?
> >
>
> I think so. Imagine you have a UI with breakpoint list and there's
> an "Add Breakpoint" button. When "Add Breakpoint" is pressed, user
> fills location, press "OK", new breakpoint is added and
> *UI pre-selects just added breakpoint*. This is IMO sensible UI behavior.
>
> Now, when you press "OK" to add the breakpoint on a location, behind the
> scenes
> frontend issues -break-insert command and waits for ^done confirmation.
> But as Jonah mentioned, another breakpoint could be inserted meanwhile so
> you'd get something like this on MI channel.
>
> 10-break-insert main
> =breakpoint-created,bkpt={number="1",type=...<some unrelated breakpoint
> from CLI or script>}
> =breakpoint-created,bkpt={number="2",type=...<the breakpoint on main you
> just requested>}
> =breakpoint-created,bkpt={number="3",type=...<some unrelated breakpoint
> from CLI or script>}
> 10^done
>
> Now, if ^done response would not contain bkpt-number=2 attribute, how
> would you know which
> is the breakpoint you need to pre-select in the UI?
>
>
>
Thanks Jan, that looks like a good example to show the problem.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-10 21:19 MI3 and async notifications Jan Vrany
  2019-06-10 23:23 ` Jonah Graham
  2019-06-15 14:34 ` Tom Tromey
@ 2019-06-18  3:14 ` Simon Marchi
  2019-06-18 20:38   ` Jan Vrany
  2 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2019-06-18  3:14 UTC (permalink / raw)
  To: Jan Vrany, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

On 2019-06-10 5:19 p.m., Jan Vrany wrote:
> Hello,
> 
> as an user og GDB/MI (and frontent developer), I'd like to 
> open a discussion about one aspect of MI that I'd like to change
> in MI3 before it is released into the wild. 

Hi Jan and others,

Thanks, nice to see some action on improving MI.

I have just read the thread, and I have a few questions about where
this is going.

> Currently, quite a few commands suppress async notifications when
> a command is issued through MI. For example, when a breakpoint is 
> added using -break-insert then =breakpoint-created is not emitted.
> 
> At least in my case, this behavior complicates client design because
> now the client has to care for new breakpoints on multiple places:
> (i)  listen to breakpoint created events to handle cases where breakpoint
>      is added via CLI. 
> (ii) do similar processing whenever MI returns ^done for previously 
>      issued -break-insert command.
>  
> This leads to a complex logic, especially in cases where frontend has
> some scripting support (so execution of MI commands is not completely
> under frontend developer's control).
>
> Emitting notifications unconditionally would simplify things a lot 
> - again at least in my case.

The idea of the current behavior is that you shouldn't be asynchronously notified
of the result of your own command.  As was illustrated in this thread, it would be
difficult to determine whether a =breakpoint-created async notification is the result
of the pending -break-insert, or it just happens that the user has created a breakpoint
on the CLI at the same time.  So to remove this ambiguity, we don't send the notification.

I am skeptical about the complex logic you are talking about to handle both
=breakpoint-created notifications and responses to the -break-insert.  Both contain pretty
much the same information.  So rather than adding an option in GDB for emitting async
notifications unconditionally, can't you just process both using the same function?  That
doesn't really complicated, but maybe I am misunderstanding your problem, in which case
please expand.

It would be useful to have a very concrete use case where you could point out "see here,
I am missing some info and a notification would be useful".  It could very well be that
some notifications are just missing.  For example I would argue we are missing notifications
if using two MI clients (through the new-ui command, although I don't remember if that's
"officially" supported).  If one of the MI client inserts a breakpoint with -break-insert,
the other one is not notified.  I would consider that a bug that the second client doesn't
get a =breakpoint-created.

Also, I am a bit worried by a proposal in the thread, which would be to remove information
from the -break-insert ^done response, arguing that the async notification would have already
been emitted.  It is clear and unambiguous how to map a response to a request, but it would not
be obvious to map an async notification to a request.  So it appears to me as a regression in
functionality.

Simon

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-18  3:14 ` Simon Marchi
@ 2019-06-18 20:38   ` Jan Vrany
  2019-06-19 15:29     ` Simon Marchi
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Vrany @ 2019-06-18 20:38 UTC (permalink / raw)
  To: Simon Marchi, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

Hi, 

> I am skeptical about the complex logic you are talking about to handle both
> =breakpoint-created notifications and responses to the -break-insert.  Both contain pretty
> much the same information.  So rather than adding an option in GDB for emitting async
> notifications unconditionally, can't you just process both using the same function?  That
> doesn't really complicated, but maybe I am misunderstanding your problemi, in which case
> please expand.

OK, let me expand (hopefully not too much)

I have a (general purpose) library that provides higher-level interface
to GDB/MI. Strictly speaking, it is not bound to any particular UI frontend. 

This library essentially provides three things: 

(i) (supposedly) easy to use API to send MI commands, like: 

    gdb send: (GDBMI_break_insert arguments: { 'main' }) andWithResultDo:[ :result |
      result isSuccess ifTrue:[
        Stdout print: 'Breakpoint inserted'
      ] ifFalse:[
        Stderr print: 'Oops, something went wrong!'
      ]
    ]
  
    (sorry for bit arcane syntax, hope you can make sense of it)

(ii) provide object model of the debugger and its state (like, inferiors, their threads, 
     frames in thread, variables, registers, breakpoints) like: 
     
     "/ Prints the stack of first thread"
     gdb selectedInferior threads first stack do:[:frame|
       Stdout print: frame printString
     ]

     The idea is that if the inferior is stopped, the model is up-to-date, when it's running,
     then "reasonable up-to-date", no guarantees.

(iii) provide a way to get notified of changes, like:

     gdb when: GDBBreakpointModifiedEvent do: [:event |
       "/ something has changed, redraw the list to display
       "/ up-to-date information 
       breakpointListPane redraw.
     ]

     This is essential to build an UI on top of this library

All the above is API exposed to library user. This design has the advantages
of being flexible - users can issue commands on their own, I do not have to 
implement and maintain wrapping API rich enough to handle all cases - and
and  because is close to GDB/MI, I don't really need to document it in depth, looking to
GDB documentation gives you very good idea what how to use it. Reusing GDB events
to notify clients of my library has the same advantage - no need to implement my 
own event hierarchy and document them. 

But if I don't get events in cases when they're result of an MI command, the only
way I can think of handling it is to intercept command result event and:
 1) examine the result (and sometimes the originating command itself) and do the 
    processing
 2) synthesize an event as if it were send by gdb and push it back so it's delivered
    to users of the library - but in this case I have to make sure it is delivered only
    to "external" observers and not "internal" observers which are responsible of keeping
    the model up-to-date. 

Both steps have to cared for case-by-case (like, -break-insert response carries data
- the breakpoint inserted - while response to -gdb-set is plain ^done so in order to
update model I have to dig into the command itself, retrieve it's parameters and 
reconstruct data from there).

All this is indeed doable and in fact, I do this already for some commands to meet my 
need back then, but then I realized I need more of this and was thinking how to avoid
all that code. A quick experiment shows that always emitting a notification solves
most (all?) issues I experienced, which is why brought it here.

Does it make sense?

> 
> It would be useful to have a very concrete use case where you could point out "see here,
> I am missing some info and a notification would be useful".  It could very well be that
> some notifications are just missing.  

To make me clear, I'm not saying that some information is missing, just that the way
it is delivered seem to be inconvenient given the way I use it. It may well be I use it
the wrong way :-)

> For example I would argue we are missing notifications
> if using two MI clients (through the new-ui command, although I don't remember if that's
> "officially" supported).  If one of the MI client inserts a breakpoint with -break-insert,
> the other one is not notified.  I would consider that a bug that the second client doesn't
> get a =breakpoint-created.

Yeah, having a second MI channel is a different story, I'm not considering this for now. 

> 
> Also, I am a bit worried by a proposal in the thread, which would be to remove information
> from the -break-insert ^done response, arguing that the async notification would have already
> been emitted.  It is clear and unambiguous how to map a response to a request, but it would not
> be obvious to map an async notification to a request.  So it appears to me as a regression in
> functionality.

This is why I said that - for example - for -break-insert we need to respond with - at least - 
^done,bkpt-number=1. For some other. like -gdb-set I don't think we need to. 

Thanks! Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-18 20:38   ` Jan Vrany
@ 2019-06-19 15:29     ` Simon Marchi
  2019-06-19 20:58       ` Jan Vrany
  0 siblings, 1 reply; 26+ messages in thread
From: Simon Marchi @ 2019-06-19 15:29 UTC (permalink / raw)
  To: Jan Vrany, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

On 2019-06-18 4:38 p.m., Jan Vrany wrote:
> Hi, 
> 
>> I am skeptical about the complex logic you are talking about to handle both
>> =breakpoint-created notifications and responses to the -break-insert.  Both contain pretty
>> much the same information.  So rather than adding an option in GDB for emitting async
>> notifications unconditionally, can't you just process both using the same function?  That
>> doesn't really complicated, but maybe I am misunderstanding your problemi, in which case
>> please expand.
> 
> OK, let me expand (hopefully not too much)
> 
> I have a (general purpose) library that provides higher-level interface
> to GDB/MI. Strictly speaking, it is not bound to any particular UI frontend. 
> 
> This library essentially provides three things: 
> 
> (i) (supposedly) easy to use API to send MI commands, like: 
> 
>     gdb send: (GDBMI_break_insert arguments: { 'main' }) andWithResultDo:[ :result |
>       result isSuccess ifTrue:[
>         Stdout print: 'Breakpoint inserted'
>       ] ifFalse:[
>         Stderr print: 'Oops, something went wrong!'
>       ]
>     ]
>   
>     (sorry for bit arcane syntax, hope you can make sense of it)
> 
> (ii) provide object model of the debugger and its state (like, inferiors, their threads, 
>      frames in thread, variables, registers, breakpoints) like: 
>      
>      "/ Prints the stack of first thread"
>      gdb selectedInferior threads first stack do:[:frame|
>        Stdout print: frame printString
>      ]
> 
>      The idea is that if the inferior is stopped, the model is up-to-date, when it's running,
>      then "reasonable up-to-date", no guarantees.
> 
> (iii) provide a way to get notified of changes, like:
> 
>      gdb when: GDBBreakpointModifiedEvent do: [:event |
>        "/ something has changed, redraw the list to display
>        "/ up-to-date information 
>        breakpointListPane redraw.
>      ]
> 
>      This is essential to build an UI on top of this library
> 
> All the above is API exposed to library user. This design has the advantages
> of being flexible - users can issue commands on their own, I do not have to 
> implement and maintain wrapping API rich enough to handle all cases - and
> and  because is close to GDB/MI, I don't really need to document it in depth, looking to
> GDB documentation gives you very good idea what how to use it. Reusing GDB events
> to notify clients of my library has the same advantage - no need to implement my 
> own event hierarchy and document them. 
> 
> But if I don't get events in cases when they're result of an MI command, the only
> way I can think of handling it is to intercept command result event and:
>  1) examine the result (and sometimes the originating command itself) and do the 
>     processing
>  2) synthesize an event as if it were send by gdb and push it back so it's delivered
>     to users of the library - but in this case I have to make sure it is delivered only
>     to "external" observers and not "internal" observers which are responsible of keeping
>     the model up-to-date. 
>
> Both steps have to cared for case-by-case (like, -break-insert response carries data
> - the breakpoint inserted - while response to -gdb-set is plain ^done so in order to
> update model I have to dig into the command itself, retrieve it's parameters and 
> reconstruct data from there).
> 
> All this is indeed doable and in fact, I do this already for some commands to meet my 
> need back then, but then I realized I need more of this and was thinking how to avoid
> all that code. A quick experiment shows that always emitting a notification solves
> most (all?) issues I experienced, which is why brought it here.
> 
> Does it make sense?

Hi Jan,

Thanks for the detailed explanation.

I am not in your shoes, so I might have a wrong picture of the situation, but this doesn't
sound really complicated.  Isn't "synthesizing" an event from the command result just calling
the same function as you do when getting a =breakpoint-created.  Or am I missing something?

I don't really understand the difference between external and internal observers (that's
probably specific to your design).  Specifically, I don't see what's the difference between
a "real" event that would come from GDB, versus an synthetic event that you would have
injected in your system from the -break-insert response.

Let's say you do get async events for breakpoints you create with -break-insert, then you would
forward these events to all these observers?  So in the case where you generate these events
yourself based on the -break-insert response, why shouldn't you also send them to all observers?
The observers wouldn't know whether it's a "real" event coming from GDB or one you created
yourself, so it shouldn't make any difference to them.

Or (re-reading your message, I realized that this is what you might be trying to explain)
are you saying that your library is very low level, and that users of the library send
"-break-insert" on their own and your library just forwards the MI command to GDB, so your
library doesn't really know (unless it sniffs the user command) that a -break-insert
command has been issued?  If so, that might explain my incomprehension.  All the MI-handling
code I have been exposed to has been a bit more high level, where the user calls some
"breakInsert" function of the library.  The library knows it's sending a -break-insert, so
it can easily handle the response however it wants (including generating a "breakpoint created"
event if it wants to).

>> It would be useful to have a very concrete use case where you could point out "see here,
>> I am missing some info and a notification would be useful".  It could very well be that
>> some notifications are just missing.  
> 
> To make me clear, I'm not saying that some information is missing, just that the way
> it is delivered seem to be inconvenient given the way I use it. It may well be I use it
> the wrong way :-)
We'll see once we understand each other better :).

>> Also, I am a bit worried by a proposal in the thread, which would be to remove information
>> from the -break-insert ^done response, arguing that the async notification would have already
>> been emitted.  It is clear and unambiguous how to map a response to a request, but it would not
>> be obvious to map an async notification to a request.  So it appears to me as a regression in
>> functionality.
> 
> This is why I said that - for example - for -break-insert we need to respond with - at least - 
> ^done,bkpt-number=1. For some other. like -gdb-set I don't think we need to

What I am worried about is that doing a change like this has a pretty big cost for on all frontends
that have this currently implemented, so it needs to have a pretty strong justification.  It's not just
moving the data fields a little bit, or adding something that everybody else can ignore, it would require
a significant flow change.  A frontend that is just interested in setting a breakpoint and getting the
result of that (i.e. the simple case) now needs to do something non-trivial: send the command, listen for
an event, store it somewhere, handle it when receiving the ^done corresponding to the command.  This
brings some concurrency/race condition problems that are just not there in the request-response scheme.

So at least, if we end up choosing to unconditionally emit the =breakpoint-created event, I would prefer
keeping the -break-insert response as-is, for backwards compatibility (for existing frontends) and
simplicity (for basic use cases), even if it means there's some redundancy.

Simon

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-19 15:29     ` Simon Marchi
@ 2019-06-19 20:58       ` Jan Vrany
  2019-06-20 15:31         ` Simon Marchi
  0 siblings, 1 reply; 26+ messages in thread
From: Jan Vrany @ 2019-06-19 20:58 UTC (permalink / raw)
  To: Simon Marchi, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

Hi, 

On Wed, 2019-06-19 at 11:29 -0400, Simon Marchi wrote:
> 
> 
> Hi Jan,
> 
> Thanks for the detailed explanation.
> 
> I am not in your shoes, so I might have a wrong picture of the situation, but this doesn't
> sound really complicated.  Isn't "synthesizing" an event from the command result just calling
> the same function as you do when getting a =breakpoint-created.  Or am I missing something?

In case of -break-insert is not that much of work since command response carries the breakpoint
object and event also arries just the breakpoint objects. 

But consider `set` CLI and `-gdb-set` MI commands: 

(gdb) 
set directories /foo/bar
&"set directories /foo/bar\n"
&"Warning: /foo/bar: No such file or directory.\n"
=cmd-param-changed,param="directories",value="/foo/bar:$cdir:$cwd"
^done
(gdb)

and

-gdb-set directories /baz/qux
&"Warning: /baz/qux: No such file or directory.\n"
^done

In this case. the response carries no data at all so in order to synthesize the event
I need to look at the originating command and parse its arguments to pick the name and value.
And as you can see above, the value parameter to the command is just "/foo/bar" while
the value in the event is "/foo/bar:$cdir:$cwd" - another little thing to care about in this
very case. 

Indeed, not a rocket science, but as you see, it requires to write a little specialized 
piece of code for "every" command (vs. not having to write anything at all if I'd get the event from 
GDB). Maybe the phrase "complex logic" was improper - complex in a way that each case has to be
dealt with individually and if one forgets, the internal model can get out of sync. 

> 
> I don't really understand the difference between external and internal observers (that's
> probably specific to your design).  Specifically, I don't see what's the difference between
> a "real" event that would come from GDB, versus an synthetic event that you would have
> injected in your system from the -break-insert response.
> 
> Let's say you do get async events for breakpoints you create with -break-insert, then you would
> forward these events to all these observers?  So in the case where you generate these events
> yourself based on the -break-insert response, why shouldn't you also send them to all observers?

Yeah, this is specific to my design. It is just that the library itself registers observers
on events in order to update the model of debugger (using the same mechanic, same patterns)
so when it is "synthetic" event, is has to be processed slightly differently. Not sure if it makes
much sense to go to such details. 

> The observers wouldn't know whether it's a "real" event coming from GDB or one you created
> yourself, so it shouldn't make any difference to them.
> 
> Or (re-reading your message, I realized that this is what you might be trying to explain)
> are you saying that your library is very low level, and that users of the library send
> "-break-insert" on their own and your library just forwards the MI command to GDB, so your
> library doesn't really know (unless it sniffs the user command) that a -break-insert
> command has been issued? 

Precisely! The library does not really know and therefore would have to "sniff". 

>  If so, that might explain my incomprehension.  All the MI-handling
> code I have been exposed to has been a bit more high level, where the user calls some
> "breakInsert" function of the library.  The library knows it's sending a -break-insert, so
> it can easily handle the response however it wants (including generating a "breakpoint created"
> event if it wants to).

Yeah, I guess that's more common approach. I decided to make it more lower-level as this gives me
more flexibility. Or so it looked back then :-) 

> > > Also, I am a bit worried by a proposal in the thread, which would be to remove information
> > > from the -break-insert ^done response, arguing that the async notification would have already
> > > been emitted.  It is clear and unambiguous how to map a response to a request, but it would not
> > > be obvious to map an async notification to a request.  So it appears to me as a regression in
> > > functionality.
> > 
> > This is why I said that - for example - for -break-insert we need to respond with - at least - 
> > ^done,bkpt-number=1. For some other. like -gdb-set I don't think we need to
> 
> What I am worried about is that doing a change like this has a pretty big cost for on all frontends
> that have this currently implemented, so it needs to have a pretty strong justification.  It's not just
> moving the data fields a little bit, or adding something that everybody else can ignore, it would require
> a significant flow change.  A frontend that is just interested in setting a breakpoint and getting the
> result of that (i.e. the simple case) now needs to do something non-trivial: send the command, listen for
> an event, store it somewhere, handle it when receiving the ^done corresponding to the command.  This
> brings some concurrency/race condition problems that are just not there in the request-response scheme.

Yes, I completely understand. This and this is a valid concern. I said it before - the last thing I want is to
break other people's stuff "that works for decades" because of my obscure thing. If the decision out of this
discussion is "no", I'll happily deal with it in my library. No problem whatsoever.

> 
> So at least, if we end up choosing to unconditionally emit the =breakpoint-created event, I would prefer
> keeping the -break-insert response as-is, for backwards compatibility (for existing frontends) and
> simplicity (for basic use cases), even if it means there's some redundancy.
> 

Yes, that was my original proposal, keep everything as it is, just to emit events in addition. 
Either unconditionally or only if mi-always-async is set (at the cost of having yet another option
which is also far from ideal). 

Jan

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-19 20:58       ` Jan Vrany
@ 2019-06-20 15:31         ` Simon Marchi
  2019-06-20 20:46           ` Jan Vrany
  2019-07-05 19:35           ` Pedro Alves
  0 siblings, 2 replies; 26+ messages in thread
From: Simon Marchi @ 2019-06-20 15:31 UTC (permalink / raw)
  To: Jan Vrany, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

On 2019-06-19 4:57 p.m., Jan Vrany wrote:
> But consider `set` CLI and `-gdb-set` MI commands: 
> 
> (gdb) 
> set directories /foo/bar
> &"set directories /foo/bar\n"
> &"Warning: /foo/bar: No such file or directory.\n"
> =cmd-param-changed,param="directories",value="/foo/bar:$cdir:$cwd"
> ^done
> (gdb)
> 
> and
> 
> -gdb-set directories /baz/qux
> &"Warning: /baz/qux: No such file or directory.\n"
> ^done
> 
> In this case. the response carries no data at all so in order to synthesize the event
> I need to look at the originating command and parse its arguments to pick the name and value.
> And as you can see above, the value parameter to the command is just "/foo/bar" while
> the value in the event is "/foo/bar:$cdir:$cwd" - another little thing to care about in this
> very case. 
> 
> Indeed, not a rocket science, but as you see, it requires to write a little specialized 
> piece of code for "every" command (vs. not having to write anything at all if I'd get the event from 
> GDB). Maybe the phrase "complex logic" was improper - complex in a way that each case has to be
> dealt with individually and if one forgets, the internal model can get out of sync. 

For the -gdb-set case, this suggests that the ^done response should contain the new value.  The reason
being that, as you have shown, GDB can massage it a little bit.  Since the value can end up different
than what the frontend asked for, the frontend would need to be informed of the new/final value.

>> The observers wouldn't know whether it's a "real" event coming from GDB or one you created
>> yourself, so it shouldn't make any difference to them.
>>
>> Or (re-reading your message, I realized that this is what you might be trying to explain)
>> are you saying that your library is very low level, and that users of the library send
>> "-break-insert" on their own and your library just forwards the MI command to GDB, so your
>> library doesn't really know (unless it sniffs the user command) that a -break-insert
>> command has been issued? 
> 
> Precisely! The library does not really know and therefore would have to "sniff". 
> 
>>  If so, that might explain my incomprehension.  All the MI-handling
>> code I have been exposed to has been a bit more high level, where the user calls some
>> "breakInsert" function of the library.  The library knows it's sending a -break-insert, so
>> it can easily handle the response however it wants (including generating a "breakpoint created"
>> event if it wants to).
> 
> Yeah, I guess that's more common approach. I decided to make it more lower-level as this gives me
> more flexibility. Or so it looked back then :-)

Ok, then I understand how ugly it is to try to track the commands and their responses.  I would tend to
say that it falls in the category "you're not using it the way it was intended to": you basically have
two or more interlocutors talking to GDB on the MI channel, whereas GDB expects to be talking to a single
interlocutor.  And your two or more interlocutors are not aware of each other's actions.

It sounds like each "interlocutor" should have its own MI channel.  Assuming the bug I mentioned earlier
is fixed (about cross-MI channel notifications not being sent), the internals of your library (which
maintains the model) would get notified whenever the user of your library creates a breakpoint with
-break-insert.

> Yes, I completely understand. This and this is a valid concern. I said it before - the last thing I want is to
> break other people's stuff "that works for decades" because of my obscure thing. If the decision out of this
> discussion is "no", I'll happily deal with it in my library. No problem whatsoever.
> 
>>
>> So at least, if we end up choosing to unconditionally emit the =breakpoint-created event, I would prefer
>> keeping the -break-insert response as-is, for backwards compatibility (for existing frontends) and
>> simplicity (for basic use cases), even if it means there's some redundancy.
>>
> 
> Yes, that was my original proposal, keep everything as it is, just to emit events in addition. 
> Either unconditionally or only if mi-always-async is set (at the cost of having yet another option
> which is also far from ideal). 

Ok, I had lost track of who suggested what in this long thread.

I am still unsure about emitting events unconditionally.  For example: some frontends want to
insert "internal" breakpoints, which are breakpoints that are not going to be directly shown to
users.  They send a -break-insert and decide that the breakpoint which results from this won't be
propagated to the UI.  But breakpoint async events are propagated to the UI (presumably because
they originate from the user creating a breakpoint in the CLI).  If GDB now emits an event for
the -break-insert breakpoint, the frontend can't know right away which future =breakpoint-created
event it should ignore.  Again, it would need to buffer all =breakpoint-created events until it
gets the ^done, then let pass through all events except the one that matches the created breakpoint.

Simon

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-20 15:31         ` Simon Marchi
@ 2019-06-20 20:46           ` Jan Vrany
  2019-07-05 19:35           ` Pedro Alves
  1 sibling, 0 replies; 26+ messages in thread
From: Jan Vrany @ 2019-06-20 20:46 UTC (permalink / raw)
  To: Simon Marchi, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

On Thu, 2019-06-20 at 11:31 -0400, Simon Marchi wrote:
> 
> Ok, then I understand how ugly it is to try to track the commands and their responses.  I would tend to
> say that it falls in the category "you're not using it the way it was intended to": 

Fair enough :-)

> 
> I am still unsure about emitting events unconditionally.  For example: some frontends want to
> insert "internal" breakpoints, which are breakpoints that are not going to be directly shown to
> users.  They send a -break-insert and decide that the breakpoint which results from this won't be
> propagated to the UI.  But breakpoint async events are propagated to the UI (presumably because
> they originate from the user creating a breakpoint in the CLI).  If GDB now emits an event for
> the -break-insert breakpoint, the frontend can't know right away which future =breakpoint-created
> event it should ignore.  Again, it would need to buffer all =breakpoint-created events until it
> gets the ^done, then let pass through all events except the one that matches the created breakpoint.
> 
Yeah, that would be tricky. 

Okay, it looks I lost this case. That's fine. 

Thanks! 

Jan


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-20 15:31         ` Simon Marchi
  2019-06-20 20:46           ` Jan Vrany
@ 2019-07-05 19:35           ` Pedro Alves
  1 sibling, 0 replies; 26+ messages in thread
From: Pedro Alves @ 2019-07-05 19:35 UTC (permalink / raw)
  To: Simon Marchi, Jan Vrany, gdb
  Cc: Joel Brobecker, Tom Tromey, André Pönitz, Jonah Graham

On 6/20/19 4:31 PM, Simon Marchi wrote:
> 
> I am still unsure about emitting events unconditionally.  For example: some frontends want to
> insert "internal" breakpoints, which are breakpoints that are not going to be directly shown to
> users.  They send a -break-insert and decide that the breakpoint which results from this won't be
> propagated to the UI.  But breakpoint async events are propagated to the UI (presumably because
> they originate from the user creating a breakpoint in the CLI).  If GDB now emits an event for
> the -break-insert breakpoint, the frontend can't know right away which future =breakpoint-created
> event it should ignore.  Again, it would need to buffer all =breakpoint-created events until it
> gets the ^done, then let pass through all events except the one that matches the created breakpoint.

In the case of two MI channels, and assuming the issue with MI events not currently
being broadcast to all channels/UIs is fixed, then such a -break-insert would show up
as event in the other MI channel, and that other channel would have no way to know
that that breakpoint is supposed to be internal.

It would seem like this suggests that for such a use case, the breakpoint itself
should carry some "this is an internal breakpoint" property.  Note you can
create internal breakpoints with Python today, but not with MI.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-06-11 13:37     ` Jonah Graham
@ 2019-07-05 20:00       ` Pedro Alves
  2019-07-05 21:58         ` Jonah Graham
  0 siblings, 1 reply; 26+ messages in thread
From: Pedro Alves @ 2019-07-05 20:00 UTC (permalink / raw)
  To: Jonah Graham, Jan Vrany; +Cc: gdb

On 6/11/19 2:37 PM, Jonah Graham wrote:
> On Tue, 11 Jun 2019 at 04:50, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:
> 
>> As you can see, you get the notification *before* ^done response. Does
>> that answer
>> your questions?
>>
>>
> Yes that does. I think CDT will use the mi-always-notify option.

Funny.  ISTR that the breakpoint notification suppressing was
originally done for CDT and reading/hearing Vladimir argue for
it.  :-D  Though I can't find any arguing about that aspect
around the original patch now:

 https://sourceware.org/ml/gdb-patches/2011-04/msg00471.html

Vladimir said "As other MI notifications", but I checked out
the tree for that commit (8d3788bd590a), and I couldn't find
any other suppression that existed back then.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: MI3 and async notifications
  2019-07-05 20:00       ` Pedro Alves
@ 2019-07-05 21:58         ` Jonah Graham
  0 siblings, 0 replies; 26+ messages in thread
From: Jonah Graham @ 2019-07-05 21:58 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Jan Vrany, gdb

On Fri, 5 Jul 2019 at 16:00, Pedro Alves <palves@redhat.com> wrote:

> On 6/11/19 2:37 PM, Jonah Graham wrote:
> > On Tue, 11 Jun 2019 at 04:50, Jan Vrany <jan.vrany@fit.cvut.cz> wrote:
> >
> >> As you can see, you get the notification *before* ^done response. Does
> >> that answer
> >> your questions?
> >>
> >>
> > Yes that does. I think CDT will use the mi-always-notify option.
>
> Funny.  ISTR that the breakpoint notification suppressing was
> originally done for CDT and reading/hearing Vladimir argue for
> it.  :-D  Though I can't find any arguing about that aspect
> around the original patch now:
>
>  https://sourceware.org/ml/gdb-patches/2011-04/msg00471.html
>
> Vladimir said "As other MI notifications", but I checked out
> the tree for that commit (8d3788bd590a), and I couldn't find
> any other suppression that existed back then.
>

To be clear,  "CDT will use the mi-always-notify" meant that CDT would make
use of the option to turn the always notify *off*. In other words, I wanted
GDB to provide it as an option and not have it always on.

Jonah

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2019-07-05 21:58 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-10 21:19 MI3 and async notifications Jan Vrany
2019-06-10 23:23 ` Jonah Graham
2019-06-11  8:50   ` Jan Vrany
2019-06-11 13:37     ` Jonah Graham
2019-07-05 20:00       ` Pedro Alves
2019-07-05 21:58         ` Jonah Graham
2019-06-15 14:34 ` Tom Tromey
2019-06-17 10:53   ` Jan Vrany
2019-06-17 12:11     ` Jonah Graham
2019-06-17 12:14     ` Joel Brobecker
2019-06-17 12:26       ` Jonah Graham
2019-06-17 12:56         ` Joel Brobecker
2019-06-17 13:12           ` Jan Vrany
2019-06-17 13:23             ` Jonah Graham
2019-06-17 20:45               ` Joel Brobecker
2019-06-17 20:58                 ` Jan Vrany
2019-06-17 21:50                   ` Jonah Graham
2019-06-17 13:12           ` Jonah Graham
2019-06-17 19:52     ` André Pönitz
2019-06-18  3:14 ` Simon Marchi
2019-06-18 20:38   ` Jan Vrany
2019-06-19 15:29     ` Simon Marchi
2019-06-19 20:58       ` Jan Vrany
2019-06-20 15:31         ` Simon Marchi
2019-06-20 20:46           ` Jan Vrany
2019-07-05 19:35           ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).