From: Patrick Monnerat via Gdb-patches <gdb-patches@sourceware.org>
To: Simon Marchi <simon.marchi@polymtl.ca>, gdb-patches@sourceware.org
Subject: Re: [PATCH] Add a timeout parameter to gdb_do_one_event
Date: Thu, 26 Aug 2021 13:36:31 +0200 [thread overview]
Message-ID: <60a52fef-89f0-62f1-1a45-5e5a40f47df6@monnerat.net> (raw)
In-Reply-To: <4e3085bb-af40-e0dc-73aa-991f97243e06@polymtl.ca>
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
next prev parent reply other threads:[~2021-08-26 11:37 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-23 18:23 Patrick Monnerat via Gdb-patches
2021-08-26 3:24 ` Simon Marchi via Gdb-patches
2021-08-26 11:36 ` Patrick Monnerat via Gdb-patches [this message]
2021-08-26 13:47 ` Simon Marchi via Gdb-patches
2021-08-26 15:14 ` Patrick Monnerat via Gdb-patches
2022-03-14 14:49 ` [PING] " Patrick Monnerat via Gdb-patches
2021-08-27 18:08 ` [PATCH] " Tom Tromey
2021-08-28 0:07 ` Patrick Monnerat via Gdb-patches
2021-08-26 18:30 Patrick Monnerat via Gdb-patches
2022-03-14 14:49 Patrick Monnerat via Gdb-patches
2022-03-14 16:17 ` Pedro Alves
2022-03-17 13:08 Patrick Monnerat via Gdb-patches
2022-04-15 16:21 ` Tom Tromey
2022-04-16 0:38 ` Patrick Monnerat via Gdb-patches
2022-07-22 13:41 ` Simon Marchi via Gdb-patches
2022-07-22 22:45 ` Patrick Monnerat via Gdb-patches
2022-07-25 1:07 ` Simon Marchi via Gdb-patches
2022-08-18 11:16 ` Andrew Burgess via Gdb-patches
2022-08-19 11:29 ` Patrick Monnerat via Gdb-patches
2022-08-23 18:38 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=60a52fef-89f0-62f1-1a45-5e5a40f47df6@monnerat.net \
--to=gdb-patches@sourceware.org \
--cc=patrick@monnerat.net \
--cc=simon.marchi@polymtl.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox