* [PATCH] gdb: make use of SCOPE_EXIT to manage thread executing state
@ 2021-12-03 13:51 Andrew Burgess
2021-12-13 11:15 ` PING: " Andrew Burgess
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Burgess @ 2021-12-03 13:51 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
While working on another patch relating to how GDB manages threads
executing and resumed state, I spotted the following code in
record-btrace.c:
executing = tp->executing ();
set_executing (proc_target, inferior_ptid, false);
id = null_frame_id;
try
{
id = get_frame_id (get_current_frame ());
}
catch (const gdb_exception &except)
{
/* Restore the previous execution state. */
set_executing (proc_target, inferior_ptid, executing);
throw;
}
/* Restore the previous execution state. */
set_executing (proc_target, inferior_ptid, executing);
return id;
I notice that we only catch the exception so we can call
set_executing, and this is the same call to set_executing that we need
to perform in the non-exception return path.
This would be much cleaner if we could use SCOPE_EXIT to avoid the
try/catch, so lets do that.
While cleaning this up, I also applied a similar patch to
record-full.c, though there's no try/catch in that case, but using
SCOPE_EXIT makes the code safe if, in the future, we do start throwing
exceptions.
There should be no user visible changes after this commit.
---
gdb/record-btrace.c | 24 ++++--------------------
gdb/record-full.c | 8 +++++---
2 files changed, 9 insertions(+), 23 deletions(-)
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3fcfd6a4761..a6ce3db64e5 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1980,9 +1980,6 @@ record_btrace_resume_thread (struct thread_info *tp,
static struct frame_id
get_thread_current_frame_id (struct thread_info *tp)
{
- struct frame_id id;
- bool executing;
-
/* Set current thread, which is implicitly used by
get_current_frame. */
scoped_restore_current_thread restore_thread;
@@ -1998,26 +1995,13 @@ get_thread_current_frame_id (struct thread_info *tp)
For the former, EXECUTING is true and we're in wait, about to
move the thread. Since we need to recompute the stack, we temporarily
set EXECUTING to false. */
- executing = tp->executing ();
+ bool executing = tp->executing ();
set_executing (proc_target, inferior_ptid, false);
-
- id = null_frame_id;
- try
- {
- id = get_frame_id (get_current_frame ());
- }
- catch (const gdb_exception &except)
+ SCOPE_EXIT
{
- /* Restore the previous execution state. */
set_executing (proc_target, inferior_ptid, executing);
-
- throw;
- }
-
- /* Restore the previous execution state. */
- set_executing (proc_target, inferior_ptid, executing);
-
- return id;
+ };
+ return get_frame_id (get_current_frame ());
}
/* Start replaying a thread. */
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 971c0f568b4..11a9457027c 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -1249,11 +1249,13 @@ record_full_wait_1 (struct target_ops *ops,
/* Try to insert the software single step breakpoint.
If insert success, set step to 0. */
set_executing (proc_target, inferior_ptid, false);
- reinit_frame_cache ();
+ SCOPE_EXIT
+ {
+ set_executing (proc_target, inferior_ptid, true);
+ };
+ reinit_frame_cache ();
step = !insert_single_step_breakpoints (gdbarch);
-
- set_executing (proc_target, inferior_ptid, true);
}
if (record_debug)
--
2.25.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* PING: [PATCH] gdb: make use of SCOPE_EXIT to manage thread executing state
2021-12-03 13:51 [PATCH] gdb: make use of SCOPE_EXIT to manage thread executing state Andrew Burgess
@ 2021-12-13 11:15 ` Andrew Burgess
2021-12-23 11:48 ` Andrew Burgess
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Burgess @ 2021-12-13 11:15 UTC (permalink / raw)
To: gdb-patches
Ping!
Thanks,
Andrew
* Andrew Burgess <aburgess@redhat.com> [2021-12-03 13:51:51 +0000]:
> While working on another patch relating to how GDB manages threads
> executing and resumed state, I spotted the following code in
> record-btrace.c:
>
> executing = tp->executing ();
> set_executing (proc_target, inferior_ptid, false);
>
> id = null_frame_id;
> try
> {
> id = get_frame_id (get_current_frame ());
> }
> catch (const gdb_exception &except)
> {
> /* Restore the previous execution state. */
> set_executing (proc_target, inferior_ptid, executing);
>
> throw;
> }
>
> /* Restore the previous execution state. */
> set_executing (proc_target, inferior_ptid, executing);
>
> return id;
>
> I notice that we only catch the exception so we can call
> set_executing, and this is the same call to set_executing that we need
> to perform in the non-exception return path.
>
> This would be much cleaner if we could use SCOPE_EXIT to avoid the
> try/catch, so lets do that.
>
> While cleaning this up, I also applied a similar patch to
> record-full.c, though there's no try/catch in that case, but using
> SCOPE_EXIT makes the code safe if, in the future, we do start throwing
> exceptions.
>
> There should be no user visible changes after this commit.
> ---
> gdb/record-btrace.c | 24 ++++--------------------
> gdb/record-full.c | 8 +++++---
> 2 files changed, 9 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index 3fcfd6a4761..a6ce3db64e5 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -1980,9 +1980,6 @@ record_btrace_resume_thread (struct thread_info *tp,
> static struct frame_id
> get_thread_current_frame_id (struct thread_info *tp)
> {
> - struct frame_id id;
> - bool executing;
> -
> /* Set current thread, which is implicitly used by
> get_current_frame. */
> scoped_restore_current_thread restore_thread;
> @@ -1998,26 +1995,13 @@ get_thread_current_frame_id (struct thread_info *tp)
> For the former, EXECUTING is true and we're in wait, about to
> move the thread. Since we need to recompute the stack, we temporarily
> set EXECUTING to false. */
> - executing = tp->executing ();
> + bool executing = tp->executing ();
> set_executing (proc_target, inferior_ptid, false);
> -
> - id = null_frame_id;
> - try
> - {
> - id = get_frame_id (get_current_frame ());
> - }
> - catch (const gdb_exception &except)
> + SCOPE_EXIT
> {
> - /* Restore the previous execution state. */
> set_executing (proc_target, inferior_ptid, executing);
> -
> - throw;
> - }
> -
> - /* Restore the previous execution state. */
> - set_executing (proc_target, inferior_ptid, executing);
> -
> - return id;
> + };
> + return get_frame_id (get_current_frame ());
> }
>
> /* Start replaying a thread. */
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 971c0f568b4..11a9457027c 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -1249,11 +1249,13 @@ record_full_wait_1 (struct target_ops *ops,
> /* Try to insert the software single step breakpoint.
> If insert success, set step to 0. */
> set_executing (proc_target, inferior_ptid, false);
> - reinit_frame_cache ();
> + SCOPE_EXIT
> + {
> + set_executing (proc_target, inferior_ptid, true);
> + };
>
> + reinit_frame_cache ();
> step = !insert_single_step_breakpoints (gdbarch);
> -
> - set_executing (proc_target, inferior_ptid, true);
> }
>
> if (record_debug)
> --
> 2.25.4
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: PING: [PATCH] gdb: make use of SCOPE_EXIT to manage thread executing state
2021-12-13 11:15 ` PING: " Andrew Burgess
@ 2021-12-23 11:48 ` Andrew Burgess
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Burgess @ 2021-12-23 11:48 UTC (permalink / raw)
To: gdb-patches
Given the limited scope of this change, I've gone ahead an pushed this
patch.
If there's any feedback later, I'm happy to make follow up changes as
needed.
Thanks,
Andrew
* Andrew Burgess <aburgess@redhat.com> [2021-12-13 11:15:32 +0000]:
> Ping!
>
> Thanks,
> Andrew
>
> * Andrew Burgess <aburgess@redhat.com> [2021-12-03 13:51:51 +0000]:
>
> > While working on another patch relating to how GDB manages threads
> > executing and resumed state, I spotted the following code in
> > record-btrace.c:
> >
> > executing = tp->executing ();
> > set_executing (proc_target, inferior_ptid, false);
> >
> > id = null_frame_id;
> > try
> > {
> > id = get_frame_id (get_current_frame ());
> > }
> > catch (const gdb_exception &except)
> > {
> > /* Restore the previous execution state. */
> > set_executing (proc_target, inferior_ptid, executing);
> >
> > throw;
> > }
> >
> > /* Restore the previous execution state. */
> > set_executing (proc_target, inferior_ptid, executing);
> >
> > return id;
> >
> > I notice that we only catch the exception so we can call
> > set_executing, and this is the same call to set_executing that we need
> > to perform in the non-exception return path.
> >
> > This would be much cleaner if we could use SCOPE_EXIT to avoid the
> > try/catch, so lets do that.
> >
> > While cleaning this up, I also applied a similar patch to
> > record-full.c, though there's no try/catch in that case, but using
> > SCOPE_EXIT makes the code safe if, in the future, we do start throwing
> > exceptions.
> >
> > There should be no user visible changes after this commit.
> > ---
> > gdb/record-btrace.c | 24 ++++--------------------
> > gdb/record-full.c | 8 +++++---
> > 2 files changed, 9 insertions(+), 23 deletions(-)
> >
> > diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> > index 3fcfd6a4761..a6ce3db64e5 100644
> > --- a/gdb/record-btrace.c
> > +++ b/gdb/record-btrace.c
> > @@ -1980,9 +1980,6 @@ record_btrace_resume_thread (struct thread_info *tp,
> > static struct frame_id
> > get_thread_current_frame_id (struct thread_info *tp)
> > {
> > - struct frame_id id;
> > - bool executing;
> > -
> > /* Set current thread, which is implicitly used by
> > get_current_frame. */
> > scoped_restore_current_thread restore_thread;
> > @@ -1998,26 +1995,13 @@ get_thread_current_frame_id (struct thread_info *tp)
> > For the former, EXECUTING is true and we're in wait, about to
> > move the thread. Since we need to recompute the stack, we temporarily
> > set EXECUTING to false. */
> > - executing = tp->executing ();
> > + bool executing = tp->executing ();
> > set_executing (proc_target, inferior_ptid, false);
> > -
> > - id = null_frame_id;
> > - try
> > - {
> > - id = get_frame_id (get_current_frame ());
> > - }
> > - catch (const gdb_exception &except)
> > + SCOPE_EXIT
> > {
> > - /* Restore the previous execution state. */
> > set_executing (proc_target, inferior_ptid, executing);
> > -
> > - throw;
> > - }
> > -
> > - /* Restore the previous execution state. */
> > - set_executing (proc_target, inferior_ptid, executing);
> > -
> > - return id;
> > + };
> > + return get_frame_id (get_current_frame ());
> > }
> >
> > /* Start replaying a thread. */
> > diff --git a/gdb/record-full.c b/gdb/record-full.c
> > index 971c0f568b4..11a9457027c 100644
> > --- a/gdb/record-full.c
> > +++ b/gdb/record-full.c
> > @@ -1249,11 +1249,13 @@ record_full_wait_1 (struct target_ops *ops,
> > /* Try to insert the software single step breakpoint.
> > If insert success, set step to 0. */
> > set_executing (proc_target, inferior_ptid, false);
> > - reinit_frame_cache ();
> > + SCOPE_EXIT
> > + {
> > + set_executing (proc_target, inferior_ptid, true);
> > + };
> >
> > + reinit_frame_cache ();
> > step = !insert_single_step_breakpoints (gdbarch);
> > -
> > - set_executing (proc_target, inferior_ptid, true);
> > }
> >
> > if (record_debug)
> > --
> > 2.25.4
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-23 11:48 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-03 13:51 [PATCH] gdb: make use of SCOPE_EXIT to manage thread executing state Andrew Burgess
2021-12-13 11:15 ` PING: " Andrew Burgess
2021-12-23 11:48 ` Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).