MI3 and async notifications

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

MI3 and async notifications

Jan Vrany
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

Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Mon, 10 Jun 2019 at 17:19, Jan Vrany <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jan Vrany
On Mon, 2019-06-10 at 19:22 -0400, Jonah Graham wrote:

> On Mon, 10 Jun 2019 at 17:19, Jan Vrany <[hidden email]> 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;

Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Tue, 11 Jun 2019 at 04:50, Jan Vrany <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Tom Tromey-2
In reply to this post by Jan Vrany
>>>>> "Jan" == Jan Vrany <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jan Vrany
On Sat, 2019-06-15 at 08:34 -0600, Tom Tromey wrote:

> > > > > > "Jan" == Jan Vrany <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Mon, 17 Jun 2019 at 06:53, Jan Vrany <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Joel Brobecker
In reply to this post by Jan Vrany
> > 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Mon, 17 Jun 2019 at 08:14, Joel Brobecker <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Joel Brobecker
> 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Mon, 17 Jun 2019 at 08:56, Joel Brobecker <[hidden email]> 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?
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jan Vrany
In reply to this post by Joel Brobecker
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

Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Mon, 17 Jun 2019 at 09:12, Jan Vrany <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

André Pönitz-4
In reply to this post by Jan Vrany
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 <[hidden email]> 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'
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Joel Brobecker
In reply to this post by Jonah Graham
> > 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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jan Vrany
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

Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jonah Graham
On Mon, 17 Jun 2019 at 16:58, Jan Vrany <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Simon Marchi-4
In reply to this post by Jan Vrany
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
Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Jan Vrany
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

Reply | Threaded
Open this post in threaded view
|

Re: MI3 and async notifications

Simon Marchi-4
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
12