[PATCH] Add a timeout parameter to gdb_do_one_event

[PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

Since commit b2d8657, having a per-interpreter event/command loop is not
possible anymore.

As Insight uses a GUI that has its own event loop, gdb and GUI event
loops have then to be "merged" (i.e.: work together). But this is
problematic as gdb_do_one_event is not aware of this alternate event
loop and thus may wait forever.

The solution is to implement a wait timeout to gdb_do_one_event. This
cannot be done externally as gdb timers are event sources themselves.

The new parameter defaults to "no timeout": as it is used by Insight
only, there is no need to update calls from the gdb source tree.
---
 gdbsupport/event-loop.cc | 45 +++++++++++++++++++++++++++++++---------
 gdbsupport/event-loop.h  |  2 +-
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 385b45b2de1..14168cae5a6 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -33,6 +33,8 @@
 #include <sys/types.h>
 #include "gdbsupport/gdb_sys_time.h"
 #include "gdbsupport/gdb_select.h"
+#include "gdbsupport/gdb_optional.h"
+#include "gdbsupport/scope-exit.h"
 
 /* See event-loop.h.  */
 
@@ -177,12 +179,17 @@ static int update_wait_timeout (void);
 static int poll_timers (void);
 \f
 /* Process one high level event.  If nothing is ready at this time,
-   wait for something to happen (via gdb_wait_for_event), then process
-   it.  Returns >0 if something was done otherwise returns <0 (this
-   can happen if there are no event sources to wait for).  */
+   wait at most MSTIMEOUT milliseconds for something to happen (via
+   gdb_wait_for_event), then process it.  Returns >0 if something was
+   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
+   A timeout of 0 allows to serve an already pending event, but does not
+   wait if none found.
+   Setting the timeout to a negative value disables it.
+   The timeout is never used by gdb itself, it is however needed to
+   integrate gdb event handling within Insight's GUI event loop. */
 
 int
-gdb_do_one_event (void)
+gdb_do_one_event (int mstimeout)
 {
   static int event_source_head = 0;
   const int number_of_sources = 3;
@@ -229,17 +236,35 @@ gdb_do_one_event (void)
 	return 1;
     }
 
+  if (!mstimeout)
+    return 0;	/* Null timeout: do not wait for an event. */
+
   /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
      we should get out because this means that there are no event
      sources left.  This will make the event loop stop, and the
-     application exit.  */
+     application exit.
+     If a timeout has been given, a new timer is set accordingly
+     to abort event wait.  It is deleted upon gdb_wait_for_event
+     termination and thus should never be triggered.
+     When the timeout is reached, events are not monitored again:
+     they already have been checked in the loop above. */
 
-  if (gdb_wait_for_event (1) < 0)
-    return -1;
+  gdb::optional<int> timer_id;
 
-  /* If gdb_wait_for_event has returned 1, it means that one event has
-     been handled.  We break out of the loop.  */
-  return 1;
+  SCOPE_EXIT 
+    {
+      if (timer_id.has_value ())
+	delete_timer (*timer_id);
+    };
+
+  if (mstimeout > 0)
+    timer_id = create_timer (mstimeout,
+			     [] (gdb_client_data arg)
+			     {
+			       ((gdb::optional<int> *) arg)->reset ();
+			     },
+			     &timer_id);
+  return gdb_wait_for_event (1);
 }
 
 /* See event-loop.h  */
diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
index 9ed592877ab..c82493e9bdf 100644
--- a/gdbsupport/event-loop.h
+++ b/gdbsupport/event-loop.h
@@ -76,7 +76,7 @@ typedef void (timer_handler_func) (gdb_client_data);
 
 /* Exported functions from event-loop.c */
 
-extern int gdb_do_one_event (void);
+extern int gdb_do_one_event (int mstimeout = -1);
 extern void delete_file_handler (int fd);
 
 /* Add a file handler/descriptor to the list of descriptors we are
-- 
2.35.1

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Tom Tromey]

>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:

Patrick> As Insight uses a GUI that has its own event loop, gdb and GUI event
Patrick> loops have then to be "merged" (i.e.: work together). But this is
Patrick> problematic as gdb_do_one_event is not aware of this alternate event
Patrick> loop and thus may wait forever.

Can Insight fds just be added to the GDB event loop?

Patrick> The solution is to implement a wait timeout to gdb_do_one_event. This
Patrick> cannot be done externally as gdb timers are event sources themselves.

I guess what you're doing is Insight is calling gdb_do_one_event
repeatedly?  And you want it to exit with a timeout so you can then run
the Insight event loop?

I don't really know the scenario here  so I am just guessing.

Anyway I don't understand why installing your own timer wouldn't work.
It seems to me that if a timer is handled, gdb_do_one_event will return.

Tom

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

On 4/15/22 18:21, Tom Tromey wrote:
>>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:
> Patrick> As Insight uses a GUI that has its own event loop, gdb and GUI event
> Patrick> loops have then to be "merged" (i.e.: work together). But this is
> Patrick> problematic as gdb_do_one_event is not aware of this alternate event
> Patrick> loop and thus may wait forever.
>
> Can Insight fds just be added to the GDB event loop?
In fact, the alternate event loop is the one of Tcl. Fortunately, Tcl 
features a "notifier" (at the C API level) that allows it to connect 
with an external event handler (in our case, gdb_do_one_event()). 
Insight already uses a notifier to handle Tcl own fds via the GDB event 
loop.
> Patrick> The solution is to implement a wait timeout to gdb_do_one_event. This
> Patrick> cannot be done externally as gdb timers are event sources themselves.
>
> I guess what you're doing is Insight is calling gdb_do_one_event
> repeatedly?  And you want it to exit with a timeout so you can then run
> the Insight event loop?
Not at all ! This is not a "parallel" monitoring, but a "nested" one.
> I don't really know the scenario here  so I am just guessing.
The real thing is: Tcl event dispatcher calls gdb_do_one_event() via the 
notifier. There's no real event loop in Insight itself (this is not 
possible anymore after commit b2d8657). We fully rely on the GDB event 
loop to dispatch events either to GDB or to Tcl The latter can then 
dispatch/schedule/monitor/wait events itself (infinite recursion was 
hard to avoid there !). The timeout is a Tcl requirement.
>
> Anyway I don't understand why installing your own timer wouldn't work.
> It seems to me that if a timer is handled, gdb_do_one_event will return.

The problem is: the timeout can be 0ms. Calls with such a timeout are 
supposed to serve pending events, if some.

If an "own" timer were used, it would be an event source by itself. A 
0ms "own" timer prevents other pending events to be handled in 1 out of 
3 gdb_do_one_event() calls, which is not what we want. The effective 
timeout value (if some) is under control of Tcl.

Note that Tcl has an internal "idle" event type. Callbacks in the idle 
queue are not supposed to be called if another event is pending. An 
"own" timer will make it fail 1/3 times.

The solution implemented in the patch creates a timer only to limit the 
wait time by reusing the limiting code that is already in place in GDB. 
This is done AFTER having monitored pending events. It should normally 
never be seen as an event source (the lambda callback should never be 
called).


As you can see, the real work is to interconnect GDB and Tcl event 
handlers. Except in the notifier that does this job, Insight C code does 
not much use the event handler(s).

I hope these explanations are clear enough: the context is not very easy 
to explain !

Thanks for your interest.

Patrick

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Simon Marchi via Gdb-patches]

On 2022-03-17 09:08, Patrick Monnerat via Gdb-patches wrote:
> Since commit b2d8657, having a per-interpreter event/command loop is not
> possible anymore.
> 
> As Insight uses a GUI that has its own event loop, gdb and GUI event
> loops have then to be "merged" (i.e.: work together). But this is
> problematic as gdb_do_one_event is not aware of this alternate event
> loop and thus may wait forever.
> 
> The solution is to implement a wait timeout to gdb_do_one_event. This
> cannot be done externally as gdb timers are event sources themselves.
> 
> The new parameter defaults to "no timeout": as it is used by Insight
> only, there is no need to update calls from the gdb source tree.

Sorry for the delay.  The patch is fine with me.  Pedro and Tom also
took a look at previous iterations and I didn't see any disagreement
either.  I don't remember, do you have push access?  Otherwise I can
push it for you.

Simon

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

On 7/22/22 15:41, Simon Marchi wrote:
>
> On 2022-03-17 09:08, Patrick Monnerat via Gdb-patches wrote:
>> Since commit b2d8657, having a per-interpreter event/command loop is not
>> possible anymore.
>>
>> As Insight uses a GUI that has its own event loop, gdb and GUI event
>> loops have then to be "merged" (i.e.: work together). But this is
>> problematic as gdb_do_one_event is not aware of this alternate event
>> loop and thus may wait forever.
>>
>> The solution is to implement a wait timeout to gdb_do_one_event. This
>> cannot be done externally as gdb timers are event sources themselves.
>>
>> The new parameter defaults to "no timeout": as it is used by Insight
>> only, there is no need to update calls from the gdb source tree.
Hi Simon,
> Sorry for the delay.
Never mind!
> The patch is fine with me.  Pedro and Tom also
> took a look at previous iterations and I didn't see any disagreement
> either.
Thanks to you and mates for these reviews and advice.
> I don't remember, do you have push access?  Otherwise I can
> push it for you.

No, I haven't. That's why I'm pinging many times!

Thanks in advance for commit.

BTW: would it be possible to get push access ? How ? I still have some 
patches (all related to insight) to submit and want to speed up a bit 
the process (without skipping this list, of course!) and discharge you 
from this task.

Patrick

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Simon Marchi via Gdb-patches]

On 2022-07-22 18:45, Patrick Monnerat wrote:
> 
> On 7/22/22 15:41, Simon Marchi wrote:
>>
>> On 2022-03-17 09:08, Patrick Monnerat via Gdb-patches wrote:
>>> Since commit b2d8657, having a per-interpreter event/command loop is not
>>> possible anymore.
>>>
>>> As Insight uses a GUI that has its own event loop, gdb and GUI event
>>> loops have then to be "merged" (i.e.: work together). But this is
>>> problematic as gdb_do_one_event is not aware of this alternate event
>>> loop and thus may wait forever.
>>>
>>> The solution is to implement a wait timeout to gdb_do_one_event. This
>>> cannot be done externally as gdb timers are event sources themselves.
>>>
>>> The new parameter defaults to "no timeout": as it is used by Insight
>>> only, there is no need to update calls from the gdb source tree.
> Hi Simon,
>> Sorry for the delay.
> Never mind!
>> The patch is fine with me.  Pedro and Tom also
>> took a look at previous iterations and I didn't see any disagreement
>> either.
> Thanks to you and mates for these reviews and advice.
>> I don't remember, do you have push access?  Otherwise I can
>> push it for you.
> 
> No, I haven't. That's why I'm pinging many times!
> 
> Thanks in advance for commit.
> 
> BTW: would it be possible to get push access ? How ? I still have some patches (all related to insight) to submit and want to speed up a bit the process (without skipping this list, of course!) and discharge you from this task.

Totally, fill the form here: https://sourceware.org/cgi-bin/pdw/ps_form.cgi

You can put me as the referrer.  Once you have your push access, you can
push a patch adding yourself to gdb/MAINTAINERS.  See the git history of
that file for examples.  You can then push the patch in this thread.

Simon

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Andrew Burgess via Gdb-patches]

Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:

> Since commit b2d8657, having a per-interpreter event/command loop is not
> possible anymore.
>
> As Insight uses a GUI that has its own event loop, gdb and GUI event
> loops have then to be "merged" (i.e.: work together). But this is
> problematic as gdb_do_one_event is not aware of this alternate event
> loop and thus may wait forever.
>
> The solution is to implement a wait timeout to gdb_do_one_event. This
> cannot be done externally as gdb timers are event sources themselves.
>
> The new parameter defaults to "no timeout": as it is used by Insight
> only, there is no need to update calls from the gdb source tree.
> ---
>  gdbsupport/event-loop.cc | 45 +++++++++++++++++++++++++++++++---------
>  gdbsupport/event-loop.h  |  2 +-
>  2 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
> index 385b45b2de1..14168cae5a6 100644
> --- a/gdbsupport/event-loop.cc
> +++ b/gdbsupport/event-loop.cc
> @@ -33,6 +33,8 @@
>  #include <sys/types.h>
>  #include "gdbsupport/gdb_sys_time.h"
>  #include "gdbsupport/gdb_select.h"
> +#include "gdbsupport/gdb_optional.h"
> +#include "gdbsupport/scope-exit.h"
>  
>  /* See event-loop.h.  */
>  
> @@ -177,12 +179,17 @@ static int update_wait_timeout (void);
>  static int poll_timers (void);
>  \f
>  /* Process one high level event.  If nothing is ready at this time,
> -   wait for something to happen (via gdb_wait_for_event), then process
> -   it.  Returns >0 if something was done otherwise returns <0 (this
> -   can happen if there are no event sources to wait for).  */
> +   wait at most MSTIMEOUT milliseconds for something to happen (via
> +   gdb_wait_for_event), then process it.  Returns >0 if something was
> +   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
> +   A timeout of 0 allows to serve an already pending event, but does not
> +   wait if none found.
> +   Setting the timeout to a negative value disables it.
> +   The timeout is never used by gdb itself, it is however needed to
> +   integrate gdb event handling within Insight's GUI event loop. */
>  
>  int
> -gdb_do_one_event (void)
> +gdb_do_one_event (int mstimeout)
>  {
>    static int event_source_head = 0;
>    const int number_of_sources = 3;
> @@ -229,17 +236,35 @@ gdb_do_one_event (void)
>  	return 1;
>      }
>  
> +  if (!mstimeout)
> +    return 0;	/* Null timeout: do not wait for an event. */
> +

This should be:

  if (mstimeout == 0)

As mstimeout is an int, not a bool.

>    /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
>       we should get out because this means that there are no event
>       sources left.  This will make the event loop stop, and the
> -     application exit.  */
> +     application exit.
> +     If a timeout has been given, a new timer is set accordingly
> +     to abort event wait.  It is deleted upon gdb_wait_for_event
> +     termination and thus should never be triggered.
> +     When the timeout is reached, events are not monitored again:
> +     they already have been checked in the loop above. */
>  
> -  if (gdb_wait_for_event (1) < 0)
> -    return -1;
> +  gdb::optional<int> timer_id;
>  
> -  /* If gdb_wait_for_event has returned 1, it means that one event has
> -     been handled.  We break out of the loop.  */
> -  return 1;
> +  SCOPE_EXIT 
> +    {
> +      if (timer_id.has_value ())
> +	delete_timer (*timer_id);
> +    };
> +
> +  if (mstimeout > 0)
> +    timer_id = create_timer (mstimeout,
> +			     [] (gdb_client_data arg)
> +			     {
> +			       ((gdb::optional<int> *) arg)->reset ();
> +			     },
> +			     &timer_id);
> +  return gdb_wait_for_event (1);

More just a warning really, but this isn't going to work in all cases.
If a target doesn't support async-mode then GDB will block waiting for
events in the check_async_event_handlers call above, and never gets down
this far.

I'm interested in this because I also want to have the event loop run
under a timeout for a patch I'm working on, and everything works fine
except for the case when I run with async support disabled.

I'm currently investigating having non-async targets ask the event loop
for a maximum wait time before they block, then return to the event loop
in order to check timers.

If I can get this working, I'll want to move your create_timer call to
the start of gdb_do_one_event, so that the timer is in place before the
call to check_async_event_handlers - though I'm not quite sure how this
would be expected to interact with the case where 'mstimeout == 0'.

Thanks,
Andrew

>  }
>  
>  /* See event-loop.h  */
> diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
> index 9ed592877ab..c82493e9bdf 100644
> --- a/gdbsupport/event-loop.h
> +++ b/gdbsupport/event-loop.h
> @@ -76,7 +76,7 @@ typedef void (timer_handler_func) (gdb_client_data);
>  
>  /* Exported functions from event-loop.c */
>  
> -extern int gdb_do_one_event (void);
> +extern int gdb_do_one_event (int mstimeout = -1);
>  extern void delete_file_handler (int fd);
>  
>  /* Add a file handler/descriptor to the list of descriptors we are
> -- 
> 2.35.1

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

On 8/18/22 13:16, Andrew Burgess wrote:
> Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> @@ -229,17 +236,35 @@ gdb_do_one_event (void)
>>   	return 1;
>>       }
>>   
>> +  if (!mstimeout)
>> +    return 0;	/* Null timeout: do not wait for an event. */
>> +
> This should be:
>
>    if (mstimeout == 0)
>
> As mstimeout is an int, not a bool.

This patch has been pushed now (bac814a), but I will change that.

> More just a warning really, but this isn't going to work in all cases.
> If a target doesn't support async-mode then GDB will block waiting for
> events in the check_async_event_handlers call above, and never gets down
> this far.

Thanks for your remark.

 From what I can see in check_async_event_handlers, there is no direct 
waiting. The only delay/suspension can only come from an invoked handler 
while serving an event that is already active.

The timeout introduced here targets event waiting only and not things 
that may be consuming time in event handlers themselves.

Even with a timer controlled by the gdb_do_one_event caller, the current 
code can suspend a longer time.

But maybe I missed something?

>
> I'm interested in this because I also want to have the event loop run
> under a timeout for a patch I'm working on, and everything works fine
> except for the case when I run with async support disabled.
> I'm currently investigating having non-async targets ask the event loop
> for a maximum wait time before they block, then return to the event loop
> in order to check timers.
Obviously your needs are not exactly the same as mine: from what I can 
understand you want to "limit" the handler running time.
>
> If I can get this working, I'll want to move your create_timer call to
> the start of gdb_do_one_event, so that the timer is in place before the
> call to check_async_event_handlers - though I'm not quite sure how this
> would be expected to interact with the case where 'mstimeout == 0'.
Please don't! this proposal can be implemented with an "external" (i.e.: 
under caller's control) timer and I rejected this solution because using 
mstimeout=0 would cause legitimate pending event misses, the timer 
becoming an event source by itself! In other words, it should not exist 
when calling poll_timers.


Thanks for your comments,

Patrick

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Tom Tromey]

>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:

Patrick> From what I can see in check_async_event_handlers, there is no direct
Patrick> waiting. The only delay/suspension can only come from an invoked
Patrick> handler while serving an event that is already active.

Yes, but there's a hack in prepare_to_wait that works around targets
that have not implemented async:

  /* If the target can't async, emulate it by marking the infrun event
     handler such that as soon as we get back to the event-loop, we
     immediately end up in fetch_inferior_event again calling
     target_wait.  */
  if (!target_can_async_p ())
    mark_infrun_async_event_handler ();

These targets will block in their wait method.

It would be good to convert all targets to be async, but that's hard,
partly because some targets aren't maintained, and also because for
remote-sim it would require a pretty big rewrite.

Tom

[PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

Since commit b2d8657, having a per-interpreter event/command loop is not
possible anymore.

As Insight uses a GUI that has its own event loop, gdb and GUI event
loops have then to be "merged" (i.e.: work together). But this is
problematic as gdb_do_one_event is not aware of this alternate event
loop and thus may wait forever.

The solution is to implement a wait timeout to gdb_do_one_event. This
cannot be done externally as gdb timers are event sources themselves.

The new parameter defaults to "no timeout": as it is used by Insight
only, there is no need to update calls from the gdb source tree.
---
 gdbsupport/event-loop.cc | 55 +++++++++++++++++++++++++++++++---------
 gdbsupport/event-loop.h  |  2 +-
 2 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 385b45b2de1..1129f4cc84e 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -33,6 +33,7 @@
 #include <sys/types.h>
 #include "gdbsupport/gdb_sys_time.h"
 #include "gdbsupport/gdb_select.h"
+#include "gdbsupport/gdb_optional.h"
 
 /* See event-loop.h.  */
 
@@ -177,16 +178,22 @@ static int update_wait_timeout (void);
 static int poll_timers (void);
 \f
 /* Process one high level event.  If nothing is ready at this time,
-   wait for something to happen (via gdb_wait_for_event), then process
-   it.  Returns >0 if something was done otherwise returns <0 (this
-   can happen if there are no event sources to wait for).  */
+   wait at most MSTIMEOUT milliseconds for something to happen (via
+   gdb_wait_for_event), then process it.  Returns >0 if something was
+   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
+   A timeout of 0 allows to serve an already pending event, but does not
+   wait if none found.
+   Setting the timeout to a negative value disables it.
+   The timeout is never used by gdb itself, it is however needed to
+   integrate gdb event handling within Insight's GUI event loop. */
 
 int
-gdb_do_one_event (void)
+gdb_do_one_event (int mstimeout)
 {
   static int event_source_head = 0;
   const int number_of_sources = 3;
   int current = 0;
+  int res = 0;
 
   /* First let's see if there are any asynchronous signal handlers
      that are ready.  These would be the result of invoking any of the
@@ -198,8 +205,6 @@ gdb_do_one_event (void)
      round-robin fashion.  */
   for (current = 0; current < number_of_sources; current++)
     {
-      int res;
-
       switch (event_source_head)
 	{
 	case 0:
@@ -232,14 +237,40 @@ gdb_do_one_event (void)
   /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
      we should get out because this means that there are no event
      sources left.  This will make the event loop stop, and the
-     application exit.  */
+     application exit.
+     If a timeout has been given, a new timer is set accordingly
+     to abort event wait.  It is deleted upon gdb_wait_for_event
+     termination and thus should never be triggered.
+     When the timeout is reached, events are not monitored again:
+     they already have been checked in the loop above. */
+
+  res = 0;
+  if (mstimeout != 0)
+    {
+      gdb::optional<int> timer_id;
 
-  if (gdb_wait_for_event (1) < 0)
-    return -1;
+      try
+	{
+	  if (mstimeout > 0)
+	    timer_id = create_timer (mstimeout,
+				     [] (gdb_client_data arg)
+				     {
+				       ((gdb::optional<int> *) arg)->reset ();
+				     },
+				     &timer_id);
+	  res = gdb_wait_for_event (1);
+	  if (timer_id.has_value ())
+	    delete_timer (*timer_id);
+	}
+      catch (...)
+	{
+	  if (timer_id.has_value ())
+	    delete_timer (*timer_id);
+	  throw;
+	}
+    }
 
-  /* If gdb_wait_for_event has returned 1, it means that one event has
-     been handled.  We break out of the loop.  */
-  return 1;
+  return res;
 }
 
 /* See event-loop.h  */
diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
index 9ed592877ab..c82493e9bdf 100644
--- a/gdbsupport/event-loop.h
+++ b/gdbsupport/event-loop.h
@@ -76,7 +76,7 @@ typedef void (timer_handler_func) (gdb_client_data);
 
 /* Exported functions from event-loop.c */
 
-extern int gdb_do_one_event (void);
+extern int gdb_do_one_event (int mstimeout = -1);
 extern void delete_file_handler (int fd);
 
 /* Add a file handler/descriptor to the list of descriptors we are
-- 
2.35.1

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Pedro Alves]

On 2022-03-14 14:49, Patrick Monnerat via Gdb-patches wrote:
> +      try
> +	{
> +	  if (mstimeout > 0)
> +	    timer_id = create_timer (mstimeout,
> +				     [] (gdb_client_data arg)
> +				     {
> +				       ((gdb::optional<int> *) arg)->reset ();
> +				     },
> +				     &timer_id);
> +	  res = gdb_wait_for_event (1);
> +	  if (timer_id.has_value ())
> +	    delete_timer (*timer_id);
> +	}
> +      catch (...)
> +	{
> +	  if (timer_id.has_value ())
> +	    delete_timer (*timer_id);
> +	  throw;
> +	}


Instead of duplicated code on normal and exception exits, you can use
a SCOPE_EXIT:

          gdb::optional<int> timer_id;

          SCOPE_EXIT 
            {
              if (timer_id.has_value ())
                delete_timer (*timer_id);
            };

	  if (mstimeout > 0)
	    timer_id = create_timer (mstimeout,
				     [] (gdb_client_data arg)
				     {
				       ((gdb::optional<int> *) arg)->reset ();
				     },
				     &timer_id);
	  return gdb_wait_for_event (1);


I think you don't even need the "res" variable this way.

[PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

Since commit b2d8657, having a per-interpreter event/command loop is not
possible anymore.

As Insight uses a GUI that has its own event loop, gdb and GUI event
loops have then to be "merged" (i.e.: work together). But this is
problematic as gdb_do_one_event is not aware of this alternate event
loop and thus may wait forever.

The solution is to implement a wait timeout to gdb_do_one_event. This
cannot be done externally as gdb timers are event sources themselves.

The new parameter defaults to "no timeout": as it is used by Insight
only, there is no need to update calls from the gdb source tree.
---
 gdbsupport/event-loop.cc | 80 ++++++++++++++++++++++++++++++++++------
 gdbsupport/event-loop.h  |  2 +-
 2 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 98d1ada52cd..7c99d8ccbb6 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -33,6 +33,7 @@
 #include <sys/types.h>
 #include "gdbsupport/gdb_sys_time.h"
 #include "gdbsupport/gdb_select.h"
+#include "gdbsupport/gdb_optional.h"
 
 /* See event-loop.h.  */
 
@@ -176,17 +177,64 @@ static int gdb_wait_for_event (int);
 static int update_wait_timeout (void);
 static int poll_timers (void);
 \f
+/* RAII class for the gdb_do_one_event timeout timer. */
+
+class scoped_event_wait_timeout
+{
+public:
+  scoped_event_wait_timeout ()
+    : m_timer_id ()
+  {}
+
+  ~scoped_event_wait_timeout ()
+  {
+    this->destroy ();
+  }
+
+  void reset ()
+  {
+    m_timer_id.reset ();
+  }
+
+  void destroy ()
+  {
+    if (m_timer_id.has_value ())
+      delete_timer (*m_timer_id);
+    this->reset ();
+  }
+
+  void start_timer (int timeout)
+  {
+    this->destroy ();
+    m_timer_id = create_timer (timeout,
+			       [] (gdb_client_data arg)
+			       {
+				 ((scoped_event_wait_timeout *) arg)->reset ();
+			       },
+			       this);
+  }
+
+private:
+  gdb::optional<int> m_timer_id;
+};
+
 /* Process one high level event.  If nothing is ready at this time,
-   wait for something to happen (via gdb_wait_for_event), then process
-   it.  Returns >0 if something was done otherwise returns <0 (this
-   can happen if there are no event sources to wait for).  */
+   wait at most MSTIMEOUT milliseconds for something to happen (via
+   gdb_wait_for_event), then process it.  Returns >0 if something was
+   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
+   A timeout of 0 allows to serve an already pending event, but does not
+   wait if none found.
+   Setting the timeout to a negative value disables it.
+   The timeout is never used by gdb itself, it is however needed to
+   integrate gdb event handling within Insight's GUI event loop. */
 
 int
-gdb_do_one_event (void)
+gdb_do_one_event (int mstimeout)
 {
   static int event_source_head = 0;
   const int number_of_sources = 3;
   int current = 0;
+  int res = 0;
 
   /* First let's see if there are any asynchronous signal handlers
      that are ready.  These would be the result of invoking any of the
@@ -198,8 +246,6 @@ gdb_do_one_event (void)
      round-robin fashion.  */
   for (current = 0; current < number_of_sources; current++)
     {
-      int res;
-
       switch (event_source_head)
 	{
 	case 0:
@@ -232,14 +278,24 @@ gdb_do_one_event (void)
   /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
      we should get out because this means that there are no event
      sources left.  This will make the event loop stop, and the
-     application exit.  */
+     application exit.
+     If a timeout has been given, a new timer is set accordingly
+     to abort event wait.  It is deleted upon gdb_wait_for_event
+     termination and thus should never be triggered.
+     When the timeout is reached, events are not monitored again:
+     they already have been checked in the loop above. */
+
+  res = 0;
+  if (mstimeout != 0)
+    {
+      scoped_event_wait_timeout timer_id;
 
-  if (gdb_wait_for_event (1) < 0)
-    return -1;
+      if (mstimeout > 0)
+	timer_id.start_timer (mstimeout);
+      res = gdb_wait_for_event (1);
+    }
 
-  /* If gdb_wait_for_event has returned 1, it means that one event has
-     been handled.  We break out of the loop.  */
-  return 1;
+  return res;
 }
 
 /* See event-loop.h  */
diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
index dc4e4d59f03..cf62f654c1c 100644
--- a/gdbsupport/event-loop.h
+++ b/gdbsupport/event-loop.h
@@ -76,7 +76,7 @@ typedef void (timer_handler_func) (gdb_client_data);
 
 /* Exported functions from event-loop.c */
 
-extern int gdb_do_one_event (void);
+extern int gdb_do_one_event (int mstimeout = -1);
 extern void delete_file_handler (int fd);
 
 /* Add a file handler/descriptor to the list of descriptors we are
-- 
2.31.1

[PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

Since commit b2d8657, having a per-interpreter event/command loop is not
possible anymore.

As Insight uses a GUI that has its own event loop, gdb and GUI event
loops have then to be "merged" (i.e.: work together). But this is
problematic as gdb_do_one_event is not aware of this alternate event
loop and thus may wait forever.

The solution is to implement a wait timeout to gdb_do_one_event. This
cannot be done externally as timers are event sources themselves.

The new parameter defaults to "no timeout": as it is used by Insight
only, there is no need to update calls from the gdb source tree.
---
 gdbsupport/event-loop.cc | 45 ++++++++++++++++++++++++++++------------
 gdbsupport/event-loop.h  |  2 +-
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
index 98d1ada52cd..72c64dcdb72 100644
--- a/gdbsupport/event-loop.cc
+++ b/gdbsupport/event-loop.cc
@@ -177,16 +177,21 @@ static int update_wait_timeout (void);
 static int poll_timers (void);
 \f
 /* Process one high level event.  If nothing is ready at this time,
-   wait for something to happen (via gdb_wait_for_event), then process
-   it.  Returns >0 if something was done otherwise returns <0 (this
-   can happen if there are no event sources to wait for).  */
+   wait at most mstimeout milliseconds for something to happen (via
+   gdb_wait_for_event), then process it.  Returns >0 if something was
+   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
+   Setting the timeout to a negative value disables it.
+   The timeout is never used by gdb itself, it is however needed to
+   integrate gdb event handling within some external (i.e.: GUI) event
+   loop. */
 
 int
-gdb_do_one_event (void)
+gdb_do_one_event (int mstimeout)
 {
   static int event_source_head = 0;
   const int number_of_sources = 3;
   int current = 0;
+  int res = 0;
 
   /* First let's see if there are any asynchronous signal handlers
      that are ready.  These would be the result of invoking any of the
@@ -198,8 +203,6 @@ gdb_do_one_event (void)
      round-robin fashion.  */
   for (current = 0; current < number_of_sources; current++)
     {
-      int res;
-
       switch (event_source_head)
 	{
 	case 0:
@@ -232,14 +235,30 @@ gdb_do_one_event (void)
   /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
      we should get out because this means that there are no event
      sources left.  This will make the event loop stop, and the
-     application exit.  */
-
-  if (gdb_wait_for_event (1) < 0)
-    return -1;
+     application exit.
+     If a timeout has been given, a new timer is set accordingly
+     to abort event wait.  It is deleted upon gdb_wait_for_event
+     termination and thus should never be triggered.
+     When the timeout is reached, events are not monitored again:
+     they already have been checked in the loop above. */
+
+  if (mstimeout != 0)
+    {
+      int timerid = 0;
+
+      if (mstimeout > 0)
+	timerid = create_timer (mstimeout,
+				[] (gdb_client_data timeridp)
+				  {
+				    *((int *) timeridp) = 0;
+				  },
+				&timerid);
+      res = gdb_wait_for_event (1);
+      if (timerid)
+	delete_timer (timerid);
+    }
 
-  /* If gdb_wait_for_event has returned 1, it means that one event has
-     been handled.  We break out of the loop.  */
-  return 1;
+  return res;
 }
 
 /* See event-loop.h  */
diff --git a/gdbsupport/event-loop.h b/gdbsupport/event-loop.h
index dc4e4d59f03..cf62f654c1c 100644
--- a/gdbsupport/event-loop.h
+++ b/gdbsupport/event-loop.h
@@ -76,7 +76,7 @@ typedef void (timer_handler_func) (gdb_client_data);
 
 /* Exported functions from event-loop.c */
 
-extern int gdb_do_one_event (void);
+extern int gdb_do_one_event (int mstimeout = -1);
 extern void delete_file_handler (int fd);
 
 /* Add a file handler/descriptor to the list of descriptors we are
-- 
2.31.1

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Simon Marchi via Gdb-patches]

On 2021-08-23 2:23 p.m., Patrick Monnerat via Gdb-patches wrote:
> Since commit b2d8657, having a per-interpreter event/command loop is not
> possible anymore.
> 
> As Insight uses a GUI that has its own event loop, gdb and GUI event
> loops have then to be "merged" (i.e.: work together). But this is
> problematic as gdb_do_one_event is not aware of this alternate event
> loop and thus may wait forever.
> 
> The solution is to implement a wait timeout to gdb_do_one_event. This
> cannot be done externally as timers are event sources themselves.
> 
> The new parameter defaults to "no timeout": as it is used by Insight
> only, there is no need to update calls from the gdb source tree.

So, Insight's main loop looks like:

  while True:
    call gdb_do_one_event with a timeout
    call gui_do_one_event with a timeout

?

> ---
>  gdbsupport/event-loop.cc | 45 ++++++++++++++++++++++++++++------------
>  gdbsupport/event-loop.h  |  2 +-
>  2 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
> index 98d1ada52cd..72c64dcdb72 100644
> --- a/gdbsupport/event-loop.cc
> +++ b/gdbsupport/event-loop.cc
> @@ -177,16 +177,21 @@ static int update_wait_timeout (void);
>  static int poll_timers (void);
>  \f
>  /* Process one high level event.  If nothing is ready at this time,
> -   wait for something to happen (via gdb_wait_for_event), then process
> -   it.  Returns >0 if something was done otherwise returns <0 (this
> -   can happen if there are no event sources to wait for).  */
> +   wait at most mstimeout milliseconds for something to happen (via

mstimeout -> MSTIMEOUT

> +   gdb_wait_for_event), then process it.  Returns >0 if something was
> +   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
> +   Setting the timeout to a negative value disables it.

Does setting the timeout to 0 mean return immediately if nothing is
available?

> +   The timeout is never used by gdb itself, it is however needed to
> +   integrate gdb event handling within some external (i.e.: GUI) event
> +   loop. */

Err, you can probably say "Insight" here.  It's not like we want to
support programs doing this in general.  We make an exception for
Insight because of historical reasons, I suppose.

>  int
> -gdb_do_one_event (void)
> +gdb_do_one_event (int mstimeout)
>  {
>    static int event_source_head = 0;
>    const int number_of_sources = 3;
>    int current = 0;
> +  int res = 0;
>  
>    /* First let's see if there are any asynchronous signal handlers
>       that are ready.  These would be the result of invoking any of the
> @@ -198,8 +203,6 @@ gdb_do_one_event (void)
>       round-robin fashion.  */
>    for (current = 0; current < number_of_sources; current++)
>      {
> -      int res;
> -
>        switch (event_source_head)
>  	{
>  	case 0:
> @@ -232,14 +235,30 @@ gdb_do_one_event (void)
>    /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
>       we should get out because this means that there are no event
>       sources left.  This will make the event loop stop, and the
> -     application exit.  */
> -
> -  if (gdb_wait_for_event (1) < 0)
> -    return -1;
> +     application exit.
> +     If a timeout has been given, a new timer is set accordingly
> +     to abort event wait.  It is deleted upon gdb_wait_for_event
> +     termination and thus should never be triggered.
> +     When the timeout is reached, events are not monitored again:
> +     they already have been checked in the loop above. */
> +
> +  if (mstimeout != 0)
> +    {
> +      int timerid = 0;
> +
> +      if (mstimeout > 0)
> +	timerid = create_timer (mstimeout,
> +				[] (gdb_client_data timeridp)
> +				  {
> +				    *((int *) timeridp) = 0;
> +				  },
> +				&timerid);
> +      res = gdb_wait_for_event (1);
> +      if (timerid)
> +	delete_timer (timerid);

Probably not a practical concern, but by creating timers over and over,
for a reaaaaaaally long GDB session, the timer id will eventually wrap
and create_timer might return 0.  I don't know what will happen then.

Simon

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

On 8/26/21 5:24 AM, Simon Marchi wrote:
> On 2021-08-23 2:23 p.m., Patrick Monnerat via Gdb-patches wrote:
>> Since commit b2d8657, having a per-interpreter event/command loop is not
>> possible anymore.
>>
>> As Insight uses a GUI that has its own event loop, gdb and GUI event
>> loops have then to be "merged" (i.e.: work together). But this is
>> problematic as gdb_do_one_event is not aware of this alternate event
>> loop and thus may wait forever.
>>
>> The solution is to implement a wait timeout to gdb_do_one_event. This
>> cannot be done externally as timers are event sources themselves.
>>
>> The new parameter defaults to "no timeout": as it is used by Insight
>> only, there is no need to update calls from the gdb source tree.
> So, Insight's main loop looks like:
>
>    while True:
>      call gdb_do_one_event with a timeout
>      call gui_do_one_event with a timeout
>
> ?

Not exactly, although this is the first idea that emerges. But this 
approach is not reactive enough and consumes CPU uselessly.

The real implementation makes the GUI event loop call gdb_do_one_event 
and recursively. The actual event waiting is performed by 
gdb_do_one_event, but the GUI may define a timeout for it. The hard task 
here is to avoid infinite recursion.

As Insight GUI is Tcl/Tk, a Tcl C API feature called a notifier 
(https://www.tcl.tk/man/tcl8.4/TclLib/Notifier.html) allowed me to 
design such a strategy. See 
https://sourceware.org/git/?p=insight.git;a=blob;f=gdbtk/generic/gdbtk.c 
line 247 and under to satisfy your curiosity! But it is a quite large 
part of the interface between the 2 subsystems that was not needed 
before gdb commit b2d8657. Note that an additional patch (unsubmitted 
yet) is needed to map Tcl file events into gdb file handlers.

The alternate solution would have been to run the GUI in a separate 
thread, but that's even a bigger work!

>
>> ---
>>   gdbsupport/event-loop.cc | 45 ++++++++++++++++++++++++++++------------
>>   gdbsupport/event-loop.h  |  2 +-
>>   2 files changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
>> index 98d1ada52cd..72c64dcdb72 100644
>> --- a/gdbsupport/event-loop.cc
>> +++ b/gdbsupport/event-loop.cc
>> @@ -177,16 +177,21 @@ static int update_wait_timeout (void);
>>   static int poll_timers (void);
>>   \f
>>   /* Process one high level event.  If nothing is ready at this time,
>> -   wait for something to happen (via gdb_wait_for_event), then process
>> -   it.  Returns >0 if something was done otherwise returns <0 (this
>> -   can happen if there are no event sources to wait for).  */
>> +   wait at most mstimeout milliseconds for something to happen (via
> mstimeout -> MSTIMEOUT
Even if not a constant but the name of a parameter?
>
>> +   gdb_wait_for_event), then process it.  Returns >0 if something was
>> +   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
>> +   Setting the timeout to a negative value disables it.
> Does setting the timeout to 0 mean return immediately if nothing is
> available?
Yes,
>> +   The timeout is never used by gdb itself, it is however needed to
>> +   integrate gdb event handling within some external (i.e.: GUI) event
>> +   loop. */
> Err, you can probably say "Insight" here.  It's not like we want to
> support programs doing this in general.  We make an exception for
> Insight because of historical reasons, I suppose.
OK.
>
>>   int
>> -gdb_do_one_event (void)
>> +gdb_do_one_event (int mstimeout)
>>   {
>>     static int event_source_head = 0;
>>     const int number_of_sources = 3;
>>     int current = 0;
>> +  int res = 0;
>>   
>>     /* First let's see if there are any asynchronous signal handlers
>>        that are ready.  These would be the result of invoking any of the
>> @@ -198,8 +203,6 @@ gdb_do_one_event (void)
>>        round-robin fashion.  */
>>     for (current = 0; current < number_of_sources; current++)
>>       {
>> -      int res;
>> -
>>         switch (event_source_head)
>>   	{
>>   	case 0:
>> @@ -232,14 +235,30 @@ gdb_do_one_event (void)
>>     /* Block waiting for a new event.  If gdb_wait_for_event returns -1,
>>        we should get out because this means that there are no event
>>        sources left.  This will make the event loop stop, and the
>> -     application exit.  */
>> -
>> -  if (gdb_wait_for_event (1) < 0)
>> -    return -1;
>> +     application exit.
>> +     If a timeout has been given, a new timer is set accordingly
>> +     to abort event wait.  It is deleted upon gdb_wait_for_event
>> +     termination and thus should never be triggered.
>> +     When the timeout is reached, events are not monitored again:
>> +     they already have been checked in the loop above. */
>> +
>> +  if (mstimeout != 0)
>> +    {
>> +      int timerid = 0;
>> +
>> +      if (mstimeout > 0)
>> +	timerid = create_timer (mstimeout,
>> +				[] (gdb_client_data timeridp)
>> +				  {
>> +				    *((int *) timeridp) = 0;
>> +				  },
>> +				&timerid);
>> +      res = gdb_wait_for_event (1);
>> +      if (timerid)
>> +	delete_timer (timerid);
> Probably not a practical concern, but by creating timers over and over,
> for a reaaaaaaally long GDB session, the timer id will eventually wrap
> and create_timer might return 0.  I don't know what will happen then.

Yes, you're right. I'll change it. Maybe an RAII class here too?

Merci,

Patrick

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Simon Marchi via Gdb-patches]

On 2021-08-26 7:36 a.m., Patrick Monnerat wrote:
> 
> On 8/26/21 5:24 AM, Simon Marchi wrote:
>> On 2021-08-23 2:23 p.m., Patrick Monnerat via Gdb-patches wrote:
>>> Since commit b2d8657, having a per-interpreter event/command loop is not
>>> possible anymore.
>>>
>>> As Insight uses a GUI that has its own event loop, gdb and GUI event
>>> loops have then to be "merged" (i.e.: work together). But this is
>>> problematic as gdb_do_one_event is not aware of this alternate event
>>> loop and thus may wait forever.
>>>
>>> The solution is to implement a wait timeout to gdb_do_one_event. This
>>> cannot be done externally as timers are event sources themselves.
>>>
>>> The new parameter defaults to "no timeout": as it is used by Insight
>>> only, there is no need to update calls from the gdb source tree.
>> So, Insight's main loop looks like:
>>
>>    while True:
>>      call gdb_do_one_event with a timeout
>>      call gui_do_one_event with a timeout
>>
>> ?
> 
> Not exactly, although this is the first idea that emerges. But this approach is not reactive enough and consumes CPU uselessly.

Indeed, happy to know it's not that.

> The real implementation makes the GUI event loop call gdb_do_one_event and recursively. The actual event waiting is performed by gdb_do_one_event, but the GUI may define a timeout for it. The hard task here is to avoid infinite recursion.
> 
> As Insight GUI is Tcl/Tk, a Tcl C API feature called a notifier (https://www.tcl.tk/man/tcl8.4/TclLib/Notifier.html) allowed me to design such a strategy. See https://sourceware.org/git/?p=insight.git;a=blob;f=gdbtk/generic/gdbtk.c line 247 and under to satisfy your curiosity! But it is a quite large part of the interface between the 2 subsystems that was not needed before gdb commit b2d8657. Note that an additional patch (unsubmitted yet) is needed to map Tcl file events into gdb file handlers.

Ok, so you use GDB's event loop, registering some additional events in
it.  For example, the user clicking a button will generate an event,
wake up gdb_wait_for_event and call your handler to go do the GUI work.
Is that it?

And the timeout is needed to call into the GUI subsystem to do some
periodic work?

>>> ---
>>>   gdbsupport/event-loop.cc | 45 ++++++++++++++++++++++++++++------------
>>>   gdbsupport/event-loop.h  |  2 +-
>>>   2 files changed, 33 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
>>> index 98d1ada52cd..72c64dcdb72 100644
>>> --- a/gdbsupport/event-loop.cc
>>> +++ b/gdbsupport/event-loop.cc
>>> @@ -177,16 +177,21 @@ static int update_wait_timeout (void);
>>>   static int poll_timers (void);
>>>   \f
>>>   /* Process one high level event.  If nothing is ready at this time,
>>> -   wait for something to happen (via gdb_wait_for_event), then process
>>> -   it.  Returns >0 if something was done otherwise returns <0 (this
>>> -   can happen if there are no event sources to wait for).  */
>>> +   wait at most mstimeout milliseconds for something to happen (via
>> mstimeout -> MSTIMEOUT
> Even if not a constant but the name of a parameter?

Yes, if you look around, this is how we refer to parameter names in
comment.  Like it or hate it, that's how it is.

>>> +   gdb_wait_for_event), then process it.  Returns >0 if something was
>>> +   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
>>> +   Setting the timeout to a negative value disables it.
>> Does setting the timeout to 0 mean return immediately if nothing is
>> available?
> Yes,

Ok, can you mention it in the comment?  I don't think it is mentioned.

>> Probably not a practical concern, but by creating timers over and over,
>> for a reaaaaaaally long GDB session, the timer id will eventually wrap
>> and create_timer might return 0.  I don't know what will happen then.
> 
> Yes, you're right. I'll change it. Maybe an RAII class here too?

Not sure, event with an RAII class you need a way to flag whether a
timer was created or not.  Maybe optional?

  gdb::optional<int> timer_id;

  if (mstimeout > 0)
    timer_id = create_timer (...);

  res = gdb_wait_for_event (1);

  if (timer_id.has_value ())
    delete_timer (*timer_id);

Simon

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

On 8/26/21 3:47 PM, Simon Marchi wrote:
>
> On 2021-08-26 7:36 a.m., Patrick Monnerat wrote:
>> On 8/26/21 5:24 AM, Simon Marchi wrote:
>>> On 2021-08-23 2:23 p.m., Patrick Monnerat via Gdb-patches wrote:
>>>> Since commit b2d8657, having a per-interpreter event/command loop is not
>>>> possible anymore.
>>>>
>>>> As Insight uses a GUI that has its own event loop, gdb and GUI event
>>>> loops have then to be "merged" (i.e.: work together). But this is
>>>> problematic as gdb_do_one_event is not aware of this alternate event
>>>> loop and thus may wait forever.
>>>>
>>>> The solution is to implement a wait timeout to gdb_do_one_event. This
>>>> cannot be done externally as timers are event sources themselves.
>>>>
>>>> The new parameter defaults to "no timeout": as it is used by Insight
>>>> only, there is no need to update calls from the gdb source tree.
>>> So, Insight's main loop looks like:
>>>
>>>     while True:
>>>       call gdb_do_one_event with a timeout
>>>       call gui_do_one_event with a timeout
>>>
>>> ?
>> Not exactly, although this is the first idea that emerges. But this approach is not reactive enough and consumes CPU uselessly.
> Indeed, happy to know it's not that.
>
>> The real implementation makes the GUI event loop call gdb_do_one_event and recursively. The actual event waiting is performed by gdb_do_one_event, but the GUI may define a timeout for it. The hard task here is to avoid infinite recursion.
>>
>> As Insight GUI is Tcl/Tk, a Tcl C API feature called a notifier (https://www.tcl.tk/man/tcl8.4/TclLib/Notifier.html) allowed me to design such a strategy. See https://sourceware.org/git/?p=insight.git;a=blob;f=gdbtk/generic/gdbtk.c line 247 and under to satisfy your curiosity! But it is a quite large part of the interface between the 2 subsystems that was not needed before gdb commit b2d8657. Note that an additional patch (unsubmitted yet) is needed to map Tcl file events into gdb file handlers.
> Ok, so you use GDB's event loop, registering some additional events in
> it.  For example, the user clicking a button will generate an event,
> wake up gdb_wait_for_event and call your handler to go do the GUI work.
> Is that it?

To be short, yes. But a mouse click is a bad example as it is not 
handled exactly the same way in Unixes and W$.

Tcl "*events" are also timed calls (with their own queue) and "idle" 
(something that should be executed whenever no event is pending). The 
Tcl notifier is informed by the interpreter of the max time to wait, 
taking into account these 2 event sources.

I tried to implement this timeout with a gdb timer created before 
calling gdb_wait_for_event, but this failed as, if 0, it may be 
triggered before any other pending event because of the round-robin 
strategy of gdb_do_one_event initial loop.

>
> And the timeout is needed to call into the GUI subsystem to do some
> periodic work?
That's it. And as noted above, the actual timeout value is determined by 
Tcl.
>
>>>> ---
>>>>    gdbsupport/event-loop.cc | 45 ++++++++++++++++++++++++++++------------
>>>>    gdbsupport/event-loop.h  |  2 +-
>>>>    2 files changed, 33 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/gdbsupport/event-loop.cc b/gdbsupport/event-loop.cc
>>>> index 98d1ada52cd..72c64dcdb72 100644
>>>> --- a/gdbsupport/event-loop.cc
>>>> +++ b/gdbsupport/event-loop.cc
>>>> @@ -177,16 +177,21 @@ static int update_wait_timeout (void);
>>>>    static int poll_timers (void);
>>>>    \f
>>>>    /* Process one high level event.  If nothing is ready at this time,
>>>> -   wait for something to happen (via gdb_wait_for_event), then process
>>>> -   it.  Returns >0 if something was done otherwise returns <0 (this
>>>> -   can happen if there are no event sources to wait for).  */
>>>> +   wait at most mstimeout milliseconds for something to happen (via
>>> mstimeout -> MSTIMEOUT
>> Even if not a constant but the name of a parameter?
> Yes, if you look around, this is how we refer to parameter names in
> comment.  Like it or hate it, that's how it is.
No problem: I just wanted to be sure it was not a misunderstanding.
>
>>>> +   gdb_wait_for_event), then process it.  Returns >0 if something was
>>>> +   done, <0 if there are no event sources to wait for, =0 if timeout occurred.
>>>> +   Setting the timeout to a negative value disables it.
>>> Does setting the timeout to 0 mean return immediately if nothing is
>>> available?
>> Yes,
> Ok, can you mention it in the comment?  I don't think it is mentioned.
Will do!
>
>>> Probably not a practical concern, but by creating timers over and over,
>>> for a reaaaaaaally long GDB session, the timer id will eventually wrap
>>> and create_timer might return 0.  I don't know what will happen then.
>> Yes, you're right. I'll change it. Maybe an RAII class here too?
> Not sure, event with an RAII class you need a way to flag whether a
> timer was created or not.  Maybe optional?
>
>    gdb::optional<int> timer_id;
>
>    if (mstimeout > 0)
>      timer_id = create_timer (...);
>
>    res = gdb_wait_for_event (1);
>
>    if (timer_id.has_value ())
>      delete_timer (*timer_id);
>
Thanks for the hint. I think both together would be optimal.

Patrick

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Tom Tromey]

>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:

Patrick> The real implementation makes the GUI event loop call gdb_do_one_event
Patrick> and recursively. The actual event waiting is performed by 
Patrick> gdb_do_one_event, but the GUI may define a timeout for it. The hard
Patrick> task here is to avoid infinite recursion.

I wonder sometimes how we'll handle integrating event loops when we want
to let users integrate Python await with GDB APIs.

Patrick> The alternate solution would have been to run the GUI in a separate
Patrick> thread, but that's even a bigger work!

This is what my Python GUI does, but indeed it's complicated.  It needed
a hack (to block some signals in the GUI thread), and it has to send
Python code back and forth between the GUI thread and the main thread,
because GDB isn't thread-safe.

Tom

Re: [PATCH] Add a timeout parameter to gdb_do_one_event

[Patrick Monnerat via Gdb-patches]

On 8/27/21 8:08 PM, Tom Tromey wrote:
>>>>>> "Patrick" == Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org> writes:
> Patrick> The real implementation makes the GUI event loop call gdb_do_one_event
> Patrick> and recursively. The actual event waiting is performed by
> Patrick> gdb_do_one_event, but the GUI may define a timeout for it. The hard
> Patrick> task here is to avoid infinite recursion.
>
> I wonder sometimes how we'll handle integrating event loops when we want
> to let users integrate Python await with GDB APIs.

I don't know much about it. Would you monitor gdb events from Python or 
an asyncio event loop from gdb?

In case you haven't seen it yet, I found that: 
https://docs.python.org/3/c-api/typeobj.html#async-object-structures.

>
> Patrick> The alternate solution would have been to run the GUI in a separate
> Patrick> thread, but that's even a bigger work!
>
> This is what my Python GUI does, but indeed it's complicated.  It needed
> a hack (to block some signals in the GUI thread), and it has to send
> Python code back and forth between the GUI thread and the main thread,
> because GDB isn't thread-safe.

Thanks for the comment Tom: it confirms what I "sniffed" !

I have to admit that, without the Tcl notifier feature, threads would 
have been the only solution.

It was (almost) easy to use gdb event system from Tcl because the latter 
provides support for it. The opposite is not true: gdb events cannot be 
delegated to some external event monitoring handler. If Insight is not 
the only piece of software having to support alternate concurrent event 
loops, shouldn't we dream of such a gdb feature? Just my 2 cents!


Patrick