* [PATCH v2 0/1] Refactor proceed function @ 2023-06-12 7:49 Christina Schimpe 2023-06-12 7:49 ` [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function Christina Schimpe 0 siblings, 1 reply; 6+ messages in thread From: Christina Schimpe @ 2023-06-12 7:49 UTC (permalink / raw) To: gdb-patches; +Cc: blarsen, aburgess Hi all, this patch was originally written by Mihails Strasuns. This version addresses the feedback of Andrew to improve comments. I also added myself as co-author in the commit message. V1 of this patch can be found here: https://sourceware.org/pipermail/gdb-patches/2023-June/200105.html Regards, Christina Mihails Strasuns (1): gdb, infrun: refactor part of `proceed` into separate function gdb/infrun.c | 120 ++++++++++++++++++++++++++------------------------- 1 file changed, 61 insertions(+), 59 deletions(-) -- 2.25.1 Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function 2023-06-12 7:49 [PATCH v2 0/1] Refactor proceed function Christina Schimpe @ 2023-06-12 7:49 ` Christina Schimpe 2023-06-14 10:02 ` Andrew Burgess 0 siblings, 1 reply; 6+ messages in thread From: Christina Schimpe @ 2023-06-12 7:49 UTC (permalink / raw) To: gdb-patches; +Cc: blarsen, aburgess From: Mihails Strasuns <mihails.strasuns@intel.com> Split thread resuming block into separate function. Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> --- gdb/infrun.c | 120 ++++++++++++++++++++++++++------------------------- 1 file changed, 61 insertions(+), 59 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index 58da1cef29e..ec41b658864 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target) } } + /* Helper function for `proceed`. Check if thread TP is suitable for + resuming, and, if it is, switch to the thread and call + `keep_going_pass_signal`. If TP is not suitable for resuming then + this function will just return without switching threads. */ + +static void +proceed_resume_thread_checked (thread_info *tp) +{ + if (!tp->inf->has_execution ()) + { + infrun_debug_printf ("[%s] target has no execution", + tp->ptid.to_string ().c_str ()); + return; + } + + if (tp->resumed ()) + { + infrun_debug_printf ("[%s] resumed", + tp->ptid.to_string ().c_str ()); + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); + return; + } + + if (thread_is_in_step_over_chain (tp)) + { + infrun_debug_printf ("[%s] needs step-over", + tp->ptid.to_string ().c_str ()); + return; + } + + /* There are further two scenarios for which we must not resume the thread: + - If a thread of that inferior is waiting for a vfork-done + (for a detached vfork child to exec or exit), breakpoints are + removed. We must not resume any thread of that inferior, other + than the one waiting for the vfork-done. + - In non-stop, forbid resuming a thread if some other thread of + that inferior is waiting for a vfork-done event (this means + breakpoints are out for this inferior). */ + if (tp->inf->thread_waiting_for_vfork_done != nullptr + && (tp != tp->inf->thread_waiting_for_vfork_done + || non_stop)) + { + infrun_debug_printf ("[%s] another thread of this inferior is " + "waiting for vfork-done", + tp->ptid.to_string ().c_str ()); + return; + } + + infrun_debug_printf ("resuming %s", + tp->ptid.to_string ().c_str ()); + + execution_control_state ecs (tp); + switch_to_thread (tp); + keep_going_pass_signal (&ecs); + if (!ecs.wait_some_more) + error (_("Command aborted.")); +} + /* Basic routine for continuing the program in various fashions. ADDR is the address to resume at, or -1 for resume where stopped. @@ -3456,67 +3514,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) resume_ptid)) { switch_to_thread_no_regs (tp); - - if (!tp->inf->has_execution ()) - { - infrun_debug_printf ("[%s] target has no execution", - tp->ptid.to_string ().c_str ()); - continue; - } - - if (tp->resumed ()) - { - infrun_debug_printf ("[%s] resumed", - tp->ptid.to_string ().c_str ()); - gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); - continue; - } - - if (thread_is_in_step_over_chain (tp)) - { - infrun_debug_printf ("[%s] needs step-over", - tp->ptid.to_string ().c_str ()); - continue; - } - - /* If a thread of that inferior is waiting for a vfork-done - (for a detached vfork child to exec or exit), breakpoints are - removed. We must not resume any thread of that inferior, other - than the one waiting for the vfork-done. */ - if (tp->inf->thread_waiting_for_vfork_done != nullptr - && tp != tp->inf->thread_waiting_for_vfork_done) - { - infrun_debug_printf ("[%s] another thread of this inferior is " - "waiting for vfork-done", - tp->ptid.to_string ().c_str ()); - continue; - } - - infrun_debug_printf ("resuming %s", - tp->ptid.to_string ().c_str ()); - - execution_control_state ecs (tp); - switch_to_thread (tp); - keep_going_pass_signal (&ecs); - if (!ecs.wait_some_more) - error (_("Command aborted.")); + proceed_resume_thread_checked (tp); } } - else if (!cur_thr->resumed () - && !thread_is_in_step_over_chain (cur_thr) - /* In non-stop, forbid resuming a thread if some other thread of - that inferior is waiting for a vfork-done event (this means - breakpoints are out for this inferior). */ - && !(non_stop - && cur_thr->inf->thread_waiting_for_vfork_done != nullptr)) - { - /* The thread wasn't started, and isn't queued, run it now. */ - execution_control_state ecs (cur_thr); - switch_to_thread (cur_thr); - keep_going_pass_signal (&ecs); - if (!ecs.wait_some_more) - error (_("Command aborted.")); - } + else + proceed_resume_thread_checked (cur_thr); disable_commit_resumed.reset_and_commit (); } -- 2.25.1 Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function 2023-06-12 7:49 ` [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function Christina Schimpe @ 2023-06-14 10:02 ` Andrew Burgess 2023-06-16 9:50 ` Andrew Burgess 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2023-06-14 10:02 UTC (permalink / raw) To: Christina Schimpe, gdb-patches; +Cc: blarsen, Simon Marchi Christina Schimpe <christina.schimpe@intel.com> writes: > From: Mihails Strasuns <mihails.strasuns@intel.com> > > Split thread resuming block into separate function. > > Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> > --- > gdb/infrun.c | 120 ++++++++++++++++++++++++++------------------------- > 1 file changed, 61 insertions(+), 59 deletions(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 58da1cef29e..ec41b658864 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target) > } > } > > + /* Helper function for `proceed`. Check if thread TP is suitable for > + resuming, and, if it is, switch to the thread and call > + `keep_going_pass_signal`. If TP is not suitable for resuming then > + this function will just return without switching threads. */ The indentation here is wrong -- header comments should start in the first column > + > +static void > +proceed_resume_thread_checked (thread_info *tp) > +{ > + if (!tp->inf->has_execution ()) > + { > + infrun_debug_printf ("[%s] target has no execution", > + tp->ptid.to_string ().c_str ()); > + return; > + } > + > + if (tp->resumed ()) > + { > + infrun_debug_printf ("[%s] resumed", > + tp->ptid.to_string ().c_str ()); > + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); > + return; > + } > + > + if (thread_is_in_step_over_chain (tp)) > + { > + infrun_debug_printf ("[%s] needs step-over", > + tp->ptid.to_string ().c_str ()); > + return; > + } > + > + /* There are further two scenarios for which we must not resume the thread: > + - If a thread of that inferior is waiting for a vfork-done > + (for a detached vfork child to exec or exit), breakpoints are > + removed. We must not resume any thread of that inferior, other > + than the one waiting for the vfork-done. > + - In non-stop, forbid resuming a thread if some other thread of > + that inferior is waiting for a vfork-done event (this means > + breakpoints are out for this inferior). */ > + if (tp->inf->thread_waiting_for_vfork_done != nullptr > + && (tp != tp->inf->thread_waiting_for_vfork_done > + || non_stop)) > + { > + infrun_debug_printf ("[%s] another thread of this inferior is " > + "waiting for vfork-done", > + tp->ptid.to_string ().c_str ()); > + return; > + } I tried to like this merged logic block, but I just couldn't. So in the end I went back to the original commit: commit d8bbae6ea080249c05ca90b1f8640fde48a18301 Date: Fri Jan 14 15:40:59 2022 -0500 gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) And tried to understand what's going on. The commit message on that original commit is awesome, and well worth a read. After looking at that for a while I think I have a replacement for this block, which I think achieves the same thing, but is structured, and commented slightly differently. I've included an updated version of this patch below, I'd love to hear what you think? The patch below is only partially tested at this point. I'm a little short of time right now. But it passes all the vfork related tests that I've been looking at, so I'm reasonably sure it's OK, I just wanted to get this out so you could take a look at it. Thanks, Andrew --- commit 45460db250f116956f4d75376f6637d2e4e7632f Author: Mihails Strasuns <mihails.strasuns@intel.com> Date: Mon Jun 12 09:49:27 2023 +0200 gdb, infrun: refactor part of `proceed` into separate function Split the thread resuming code from proceed into new function proceed_resume_thread_checked. Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> diff --git a/gdb/infrun.c b/gdb/infrun.c index 58da1cef29e..7e6104ffab2 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target) } } +/* Helper function for `proceed`. Check if thread TP is suitable for + resuming, and, if it is, switch to the thread and call + `keep_going_pass_signal`. If TP is not suitable for resuming then this + function will just return without switching threads. */ + +static void +proceed_resume_thread_checked (thread_info *tp) +{ + if (!tp->inf->has_execution ()) + { + infrun_debug_printf ("[%s] target has no execution", + tp->ptid.to_string ().c_str ()); + return; + } + + if (tp->resumed ()) + { + infrun_debug_printf ("[%s] resumed", + tp->ptid.to_string ().c_str ()); + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); + return; + } + + if (thread_is_in_step_over_chain (tp)) + { + infrun_debug_printf ("[%s] needs step-over", + tp->ptid.to_string ().c_str ()); + return; + } + + /* When handling a vfork GDB removes all breakpoints from the program + space in which the vfork is being handled, as such we must take care + not to resume any thread other than the vfork parent -- resuming the + vfork parent allows GDB to receive and handle the 'vfork done' + event. */ + if (tp->inf->thread_waiting_for_vfork_done != nullptr) + { + if (target_is_non_stop_p ()) + { + /* For non-stop targets, regardless of whether GDB is using + all-stop or non-stop mode, threads are controlled + individually. + + When a thread is handling a vfork, breakpoints are removed + from the inferior (well, program space in fact), so it is + critical that we don't try to resume any thread other than the + vfork parent. */ + if (tp != tp->inf->thread_waiting_for_vfork_done) + { + infrun_debug_printf ("[%s] another thread of this inferior is " + "waiting for vfork-done", + tp->ptid.to_string ().c_str ()); + return; + } + + /* In non-stop mode we will never end up trying to proceed the + thread that is handling a vfork. When we realise that a + thread is handling a vfork we immediately stop all the other + threads, and resume the vfork parent thread. As far as the + user is concerned, the vfork parent thread is running. Any + attempt by the user to interrupt the vfork parent will be + deferred until the vfork has completed, at which point the + parent will no longer be waiting for a vfork. */ + gdb_assert (!non_stop); + } + else + { + /* For all-stop targets, when we attempt to resume the inferior, + we will in fact only resume the vfork parent thread, this is + handled in internal_resume_ptid, as such, there is nothing + more that needs doing here. */ + } + } + + infrun_debug_printf ("resuming %s", + tp->ptid.to_string ().c_str ()); + + execution_control_state ecs (tp); + switch_to_thread (tp); + keep_going_pass_signal (&ecs); + if (!ecs.wait_some_more) + error (_("Command aborted.")); +} + /* Basic routine for continuing the program in various fashions. ADDR is the address to resume at, or -1 for resume where stopped. @@ -3456,67 +3540,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) resume_ptid)) { switch_to_thread_no_regs (tp); - - if (!tp->inf->has_execution ()) - { - infrun_debug_printf ("[%s] target has no execution", - tp->ptid.to_string ().c_str ()); - continue; - } - - if (tp->resumed ()) - { - infrun_debug_printf ("[%s] resumed", - tp->ptid.to_string ().c_str ()); - gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); - continue; - } - - if (thread_is_in_step_over_chain (tp)) - { - infrun_debug_printf ("[%s] needs step-over", - tp->ptid.to_string ().c_str ()); - continue; - } - - /* If a thread of that inferior is waiting for a vfork-done - (for a detached vfork child to exec or exit), breakpoints are - removed. We must not resume any thread of that inferior, other - than the one waiting for the vfork-done. */ - if (tp->inf->thread_waiting_for_vfork_done != nullptr - && tp != tp->inf->thread_waiting_for_vfork_done) - { - infrun_debug_printf ("[%s] another thread of this inferior is " - "waiting for vfork-done", - tp->ptid.to_string ().c_str ()); - continue; - } - - infrun_debug_printf ("resuming %s", - tp->ptid.to_string ().c_str ()); - - execution_control_state ecs (tp); - switch_to_thread (tp); - keep_going_pass_signal (&ecs); - if (!ecs.wait_some_more) - error (_("Command aborted.")); + proceed_resume_thread_checked (tp); } } - else if (!cur_thr->resumed () - && !thread_is_in_step_over_chain (cur_thr) - /* In non-stop, forbid resuming a thread if some other thread of - that inferior is waiting for a vfork-done event (this means - breakpoints are out for this inferior). */ - && !(non_stop - && cur_thr->inf->thread_waiting_for_vfork_done != nullptr)) - { - /* The thread wasn't started, and isn't queued, run it now. */ - execution_control_state ecs (cur_thr); - switch_to_thread (cur_thr); - keep_going_pass_signal (&ecs); - if (!ecs.wait_some_more) - error (_("Command aborted.")); - } + else + proceed_resume_thread_checked (cur_thr); disable_commit_resumed.reset_and_commit (); } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function 2023-06-14 10:02 ` Andrew Burgess @ 2023-06-16 9:50 ` Andrew Burgess 2023-06-22 14:23 ` Andrew Burgess 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2023-06-16 9:50 UTC (permalink / raw) To: Christina Schimpe, gdb-patches; +Cc: blarsen, Simon Marchi Andrew Burgess <aburgess@redhat.com> writes: > Christina Schimpe <christina.schimpe@intel.com> writes: > >> From: Mihails Strasuns <mihails.strasuns@intel.com> >> >> Split thread resuming block into separate function. >> >> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> >> --- >> gdb/infrun.c | 120 ++++++++++++++++++++++++++------------------------- >> 1 file changed, 61 insertions(+), 59 deletions(-) >> >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 58da1cef29e..ec41b658864 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target) >> } >> } >> >> + /* Helper function for `proceed`. Check if thread TP is suitable for >> + resuming, and, if it is, switch to the thread and call >> + `keep_going_pass_signal`. If TP is not suitable for resuming then >> + this function will just return without switching threads. */ > > The indentation here is wrong -- header comments should start in the > first column > >> + >> +static void >> +proceed_resume_thread_checked (thread_info *tp) >> +{ >> + if (!tp->inf->has_execution ()) >> + { >> + infrun_debug_printf ("[%s] target has no execution", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } >> + >> + if (tp->resumed ()) >> + { >> + infrun_debug_printf ("[%s] resumed", >> + tp->ptid.to_string ().c_str ()); >> + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); >> + return; >> + } >> + >> + if (thread_is_in_step_over_chain (tp)) >> + { >> + infrun_debug_printf ("[%s] needs step-over", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } >> + >> + /* There are further two scenarios for which we must not resume the thread: >> + - If a thread of that inferior is waiting for a vfork-done >> + (for a detached vfork child to exec or exit), breakpoints are >> + removed. We must not resume any thread of that inferior, other >> + than the one waiting for the vfork-done. >> + - In non-stop, forbid resuming a thread if some other thread of >> + that inferior is waiting for a vfork-done event (this means >> + breakpoints are out for this inferior). */ >> + if (tp->inf->thread_waiting_for_vfork_done != nullptr >> + && (tp != tp->inf->thread_waiting_for_vfork_done >> + || non_stop)) >> + { >> + infrun_debug_printf ("[%s] another thread of this inferior is " >> + "waiting for vfork-done", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } > > I tried to like this merged logic block, but I just couldn't. So in the > end I went back to the original commit: > > commit d8bbae6ea080249c05ca90b1f8640fde48a18301 > Date: Fri Jan 14 15:40:59 2022 -0500 > > gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) > > And tried to understand what's going on. The commit message on that > original commit is awesome, and well worth a read. > > After looking at that for a while I think I have a replacement for this > block, which I think achieves the same thing, but is structured, and > commented slightly differently. > > I've included an updated version of this patch below, I'd love to hear > what you think? > > The patch below is only partially tested at this point. I'm a little > short of time right now. But it passes all the vfork related tests that > I've been looking at, so I'm reasonably sure it's OK, I just wanted to > get this out so you could take a look at it. > > Thanks, > Andrew > > --- > > commit 45460db250f116956f4d75376f6637d2e4e7632f > Author: Mihails Strasuns <mihails.strasuns@intel.com> > Date: Mon Jun 12 09:49:27 2023 +0200 > > gdb, infrun: refactor part of `proceed` into separate function > > Split the thread resuming code from proceed into new function > proceed_resume_thread_checked. > > Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index 58da1cef29e..7e6104ffab2 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target) > } > } > > +/* Helper function for `proceed`. Check if thread TP is suitable for > + resuming, and, if it is, switch to the thread and call > + `keep_going_pass_signal`. If TP is not suitable for resuming then this > + function will just return without switching threads. */ > + > +static void > +proceed_resume_thread_checked (thread_info *tp) > +{ > + if (!tp->inf->has_execution ()) > + { > + infrun_debug_printf ("[%s] target has no execution", > + tp->ptid.to_string ().c_str ()); > + return; > + } > + > + if (tp->resumed ()) > + { > + infrun_debug_printf ("[%s] resumed", > + tp->ptid.to_string ().c_str ()); > + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); > + return; > + } > + > + if (thread_is_in_step_over_chain (tp)) > + { > + infrun_debug_printf ("[%s] needs step-over", > + tp->ptid.to_string ().c_str ()); > + return; > + } > + > + /* When handling a vfork GDB removes all breakpoints from the program > + space in which the vfork is being handled, as such we must take care > + not to resume any thread other than the vfork parent -- resuming the > + vfork parent allows GDB to receive and handle the 'vfork done' > + event. */ > + if (tp->inf->thread_waiting_for_vfork_done != nullptr) > + { > + if (target_is_non_stop_p ()) > + { > + /* For non-stop targets, regardless of whether GDB is using > + all-stop or non-stop mode, threads are controlled > + individually. > + > + When a thread is handling a vfork, breakpoints are removed > + from the inferior (well, program space in fact), so it is > + critical that we don't try to resume any thread other than the > + vfork parent. */ > + if (tp != tp->inf->thread_waiting_for_vfork_done) > + { > + infrun_debug_printf ("[%s] another thread of this inferior is " > + "waiting for vfork-done", > + tp->ptid.to_string ().c_str ()); > + return; > + } This actually fixes a bug that exists currently in GDB. I'm working on a patch which includes a test that exposes the bug. I plan to get the bug fix merged before we merge this refactoring commit. > + > + /* In non-stop mode we will never end up trying to proceed the > + thread that is handling a vfork. When we realise that a > + thread is handling a vfork we immediately stop all the other > + threads, and resume the vfork parent thread. As far as the > + user is concerned, the vfork parent thread is running. Any > + attempt by the user to interrupt the vfork parent will be > + deferred until the vfork has completed, at which point the > + parent will no longer be waiting for a vfork. */ > + gdb_assert (!non_stop); And this assertion is not correct. Again, the new test I'll add will trigger this assert. We can get this refactor merged once my bug fix has landed. I'll follow up once I have something posted. Thanks, Andrew > + } > + else > + { > + /* For all-stop targets, when we attempt to resume the inferior, > + we will in fact only resume the vfork parent thread, this is > + handled in internal_resume_ptid, as such, there is nothing > + more that needs doing here. */ > + } > + } > + > + infrun_debug_printf ("resuming %s", > + tp->ptid.to_string ().c_str ()); > + > + execution_control_state ecs (tp); > + switch_to_thread (tp); > + keep_going_pass_signal (&ecs); > + if (!ecs.wait_some_more) > + error (_("Command aborted.")); > +} > + > /* Basic routine for continuing the program in various fashions. > > ADDR is the address to resume at, or -1 for resume where stopped. > @@ -3456,67 +3540,11 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal) > resume_ptid)) > { > switch_to_thread_no_regs (tp); > - > - if (!tp->inf->has_execution ()) > - { > - infrun_debug_printf ("[%s] target has no execution", > - tp->ptid.to_string ().c_str ()); > - continue; > - } > - > - if (tp->resumed ()) > - { > - infrun_debug_printf ("[%s] resumed", > - tp->ptid.to_string ().c_str ()); > - gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); > - continue; > - } > - > - if (thread_is_in_step_over_chain (tp)) > - { > - infrun_debug_printf ("[%s] needs step-over", > - tp->ptid.to_string ().c_str ()); > - continue; > - } > - > - /* If a thread of that inferior is waiting for a vfork-done > - (for a detached vfork child to exec or exit), breakpoints are > - removed. We must not resume any thread of that inferior, other > - than the one waiting for the vfork-done. */ > - if (tp->inf->thread_waiting_for_vfork_done != nullptr > - && tp != tp->inf->thread_waiting_for_vfork_done) > - { > - infrun_debug_printf ("[%s] another thread of this inferior is " > - "waiting for vfork-done", > - tp->ptid.to_string ().c_str ()); > - continue; > - } > - > - infrun_debug_printf ("resuming %s", > - tp->ptid.to_string ().c_str ()); > - > - execution_control_state ecs (tp); > - switch_to_thread (tp); > - keep_going_pass_signal (&ecs); > - if (!ecs.wait_some_more) > - error (_("Command aborted.")); > + proceed_resume_thread_checked (tp); > } > } > - else if (!cur_thr->resumed () > - && !thread_is_in_step_over_chain (cur_thr) > - /* In non-stop, forbid resuming a thread if some other thread of > - that inferior is waiting for a vfork-done event (this means > - breakpoints are out for this inferior). */ > - && !(non_stop > - && cur_thr->inf->thread_waiting_for_vfork_done != nullptr)) > - { > - /* The thread wasn't started, and isn't queued, run it now. */ > - execution_control_state ecs (cur_thr); > - switch_to_thread (cur_thr); > - keep_going_pass_signal (&ecs); > - if (!ecs.wait_some_more) > - error (_("Command aborted.")); > - } > + else > + proceed_resume_thread_checked (cur_thr); > > disable_commit_resumed.reset_and_commit (); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function 2023-06-16 9:50 ` Andrew Burgess @ 2023-06-22 14:23 ` Andrew Burgess 2023-06-28 10:14 ` Schimpe, Christina 0 siblings, 1 reply; 6+ messages in thread From: Andrew Burgess @ 2023-06-22 14:23 UTC (permalink / raw) To: Christina Schimpe, gdb-patches; +Cc: blarsen, Simon Marchi Andrew Burgess <aburgess@redhat.com> writes: > Andrew Burgess <aburgess@redhat.com> writes: > >> Christina Schimpe <christina.schimpe@intel.com> writes: >> >>> From: Mihails Strasuns <mihails.strasuns@intel.com> >>> >>> Split thread resuming block into separate function. >>> >>> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> >>> --- >>> gdb/infrun.c | 120 ++++++++++++++++++++++++++------------------------- >>> 1 file changed, 61 insertions(+), 59 deletions(-) >>> >>> diff --git a/gdb/infrun.c b/gdb/infrun.c >>> index 58da1cef29e..ec41b658864 100644 >>> --- a/gdb/infrun.c >>> +++ b/gdb/infrun.c >>> @@ -3268,6 +3268,64 @@ check_multi_target_resumption (process_stratum_target *resume_target) >>> } >>> } >>> >>> + /* Helper function for `proceed`. Check if thread TP is suitable for >>> + resuming, and, if it is, switch to the thread and call >>> + `keep_going_pass_signal`. If TP is not suitable for resuming then >>> + this function will just return without switching threads. */ >> >> The indentation here is wrong -- header comments should start in the >> first column >> >>> + >>> +static void >>> +proceed_resume_thread_checked (thread_info *tp) >>> +{ >>> + if (!tp->inf->has_execution ()) >>> + { >>> + infrun_debug_printf ("[%s] target has no execution", >>> + tp->ptid.to_string ().c_str ()); >>> + return; >>> + } >>> + >>> + if (tp->resumed ()) >>> + { >>> + infrun_debug_printf ("[%s] resumed", >>> + tp->ptid.to_string ().c_str ()); >>> + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); >>> + return; >>> + } >>> + >>> + if (thread_is_in_step_over_chain (tp)) >>> + { >>> + infrun_debug_printf ("[%s] needs step-over", >>> + tp->ptid.to_string ().c_str ()); >>> + return; >>> + } >>> + >>> + /* There are further two scenarios for which we must not resume the thread: >>> + - If a thread of that inferior is waiting for a vfork-done >>> + (for a detached vfork child to exec or exit), breakpoints are >>> + removed. We must not resume any thread of that inferior, other >>> + than the one waiting for the vfork-done. >>> + - In non-stop, forbid resuming a thread if some other thread of >>> + that inferior is waiting for a vfork-done event (this means >>> + breakpoints are out for this inferior). */ >>> + if (tp->inf->thread_waiting_for_vfork_done != nullptr >>> + && (tp != tp->inf->thread_waiting_for_vfork_done >>> + || non_stop)) >>> + { >>> + infrun_debug_printf ("[%s] another thread of this inferior is " >>> + "waiting for vfork-done", >>> + tp->ptid.to_string ().c_str ()); >>> + return; >>> + } >> >> I tried to like this merged logic block, but I just couldn't. So in the >> end I went back to the original commit: >> >> commit d8bbae6ea080249c05ca90b1f8640fde48a18301 >> Date: Fri Jan 14 15:40:59 2022 -0500 >> >> gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) >> >> And tried to understand what's going on. The commit message on that >> original commit is awesome, and well worth a read. >> >> After looking at that for a while I think I have a replacement for this >> block, which I think achieves the same thing, but is structured, and >> commented slightly differently. >> >> I've included an updated version of this patch below, I'd love to hear >> what you think? >> >> The patch below is only partially tested at this point. I'm a little >> short of time right now. But it passes all the vfork related tests that >> I've been looking at, so I'm reasonably sure it's OK, I just wanted to >> get this out so you could take a look at it. >> >> Thanks, >> Andrew >> >> --- >> >> commit 45460db250f116956f4d75376f6637d2e4e7632f >> Author: Mihails Strasuns <mihails.strasuns@intel.com> >> Date: Mon Jun 12 09:49:27 2023 +0200 >> >> gdb, infrun: refactor part of `proceed` into separate function >> >> Split the thread resuming code from proceed into new function >> proceed_resume_thread_checked. >> >> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> >> >> diff --git a/gdb/infrun.c b/gdb/infrun.c >> index 58da1cef29e..7e6104ffab2 100644 >> --- a/gdb/infrun.c >> +++ b/gdb/infrun.c >> @@ -3268,6 +3268,90 @@ check_multi_target_resumption (process_stratum_target *resume_target) >> } >> } >> >> +/* Helper function for `proceed`. Check if thread TP is suitable for >> + resuming, and, if it is, switch to the thread and call >> + `keep_going_pass_signal`. If TP is not suitable for resuming then this >> + function will just return without switching threads. */ >> + >> +static void >> +proceed_resume_thread_checked (thread_info *tp) >> +{ >> + if (!tp->inf->has_execution ()) >> + { >> + infrun_debug_printf ("[%s] target has no execution", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } >> + >> + if (tp->resumed ()) >> + { >> + infrun_debug_printf ("[%s] resumed", >> + tp->ptid.to_string ().c_str ()); >> + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); >> + return; >> + } >> + >> + if (thread_is_in_step_over_chain (tp)) >> + { >> + infrun_debug_printf ("[%s] needs step-over", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } >> + >> + /* When handling a vfork GDB removes all breakpoints from the program >> + space in which the vfork is being handled, as such we must take care >> + not to resume any thread other than the vfork parent -- resuming the >> + vfork parent allows GDB to receive and handle the 'vfork done' >> + event. */ >> + if (tp->inf->thread_waiting_for_vfork_done != nullptr) >> + { >> + if (target_is_non_stop_p ()) >> + { >> + /* For non-stop targets, regardless of whether GDB is using >> + all-stop or non-stop mode, threads are controlled >> + individually. >> + >> + When a thread is handling a vfork, breakpoints are removed >> + from the inferior (well, program space in fact), so it is >> + critical that we don't try to resume any thread other than the >> + vfork parent. */ >> + if (tp != tp->inf->thread_waiting_for_vfork_done) >> + { >> + infrun_debug_printf ("[%s] another thread of this inferior is " >> + "waiting for vfork-done", >> + tp->ptid.to_string ().c_str ()); >> + return; >> + } > > This actually fixes a bug that exists currently in GDB. I'm working on > a patch which includes a test that exposes the bug. I plan to get the > bug fix merged before we merge this refactoring commit. > >> + >> + /* In non-stop mode we will never end up trying to proceed the >> + thread that is handling a vfork. When we realise that a >> + thread is handling a vfork we immediately stop all the other >> + threads, and resume the vfork parent thread. As far as the >> + user is concerned, the vfork parent thread is running. Any >> + attempt by the user to interrupt the vfork parent will be >> + deferred until the vfork has completed, at which point the >> + parent will no longer be waiting for a vfork. */ >> + gdb_assert (!non_stop); > > And this assertion is not correct. Again, the new test I'll add will > trigger this assert. > > We can get this refactor merged once my bug fix has landed. I'll follow > up once I have something posted. I finally got my vfork patches on the mailing list. In the end it felt more natural for this refactor to live part way through that series, so that's what I've done. You can see the series here: https://inbox.sourceware.org/gdb-patches/cover.1687438786.git.aburgess@redhat.com/ Thanks, Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function 2023-06-22 14:23 ` Andrew Burgess @ 2023-06-28 10:14 ` Schimpe, Christina 0 siblings, 0 replies; 6+ messages in thread From: Schimpe, Christina @ 2023-06-28 10:14 UTC (permalink / raw) To: Andrew Burgess, gdb-patches; +Cc: blarsen, Simon Marchi Hi Andrew, So sorry for my late response. Unfortunately, I could not have a look at it earlier and it also took me some time. >> I tried to like this merged logic block, but I just couldn't. So in the >> end I went back to the original commit: >> >> commit d8bbae6ea080249c05ca90b1f8640fde48a18301 >> Date: Fri Jan 14 15:40:59 2022 -0500 >> >> gdb: fix handling of vfork by multi-threaded program (follow-fork-mode=parent, detach-on-fork=on) >> >> And tried to understand what's going on. The commit message on that >> original commit is awesome, and well worth a read. I totally agree. I tested the latest version of your patch and it lgtm. Thanks, Christina > -----Original Message----- > From: Andrew Burgess <aburgess@redhat.com> > Sent: Thursday, June 22, 2023 4:23 PM > To: Schimpe, Christina <christina.schimpe@intel.com>; gdb- > patches@sourceware.org > Cc: blarsen@redhat.com; Simon Marchi <simark@simark.ca> > Subject: Re: [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into > separate function > > Andrew Burgess <aburgess@redhat.com> writes: > > > Andrew Burgess <aburgess@redhat.com> writes: > > > >> Christina Schimpe <christina.schimpe@intel.com> writes: > >> > >>> From: Mihails Strasuns <mihails.strasuns@intel.com> > >>> > >>> Split thread resuming block into separate function. > >>> > >>> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> > >>> --- > >>> gdb/infrun.c | 120 > >>> ++++++++++++++++++++++++++------------------------- > >>> 1 file changed, 61 insertions(+), 59 deletions(-) > >>> > >>> diff --git a/gdb/infrun.c b/gdb/infrun.c index > >>> 58da1cef29e..ec41b658864 100644 > >>> --- a/gdb/infrun.c > >>> +++ b/gdb/infrun.c > >>> @@ -3268,6 +3268,64 @@ check_multi_target_resumption > (process_stratum_target *resume_target) > >>> } > >>> } > >>> > >>> + /* Helper function for `proceed`. Check if thread TP is suitable for > >>> + resuming, and, if it is, switch to the thread and call > >>> + `keep_going_pass_signal`. If TP is not suitable for resuming then > >>> + this function will just return without switching threads. */ > >> > >> The indentation here is wrong -- header comments should start in the > >> first column > >> > >>> + > >>> +static void > >>> +proceed_resume_thread_checked (thread_info *tp) { > >>> + if (!tp->inf->has_execution ()) > >>> + { > >>> + infrun_debug_printf ("[%s] target has no execution", > >>> + tp->ptid.to_string ().c_str ()); > >>> + return; > >>> + } > >>> + > >>> + if (tp->resumed ()) > >>> + { > >>> + infrun_debug_printf ("[%s] resumed", > >>> + tp->ptid.to_string ().c_str ()); > >>> + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); > >>> + return; > >>> + } > >>> + > >>> + if (thread_is_in_step_over_chain (tp)) > >>> + { > >>> + infrun_debug_printf ("[%s] needs step-over", > >>> + tp->ptid.to_string ().c_str ()); > >>> + return; > >>> + } > >>> + > >>> + /* There are further two scenarios for which we must not resume the > thread: > >>> + - If a thread of that inferior is waiting for a vfork-done > >>> + (for a detached vfork child to exec or exit), breakpoints are > >>> + removed. We must not resume any thread of that inferior, other > >>> + than the one waiting for the vfork-done. > >>> + - In non-stop, forbid resuming a thread if some other thread of > >>> + that inferior is waiting for a vfork-done event (this means > >>> + breakpoints are out for this inferior). */ > >>> + if (tp->inf->thread_waiting_for_vfork_done != nullptr > >>> + && (tp != tp->inf->thread_waiting_for_vfork_done > >>> + || non_stop)) > >>> + { > >>> + infrun_debug_printf ("[%s] another thread of this inferior is " > >>> + "waiting for vfork-done", > >>> + tp->ptid.to_string ().c_str ()); > >>> + return; > >>> + } > >> > >> I tried to like this merged logic block, but I just couldn't. So in > >> the end I went back to the original commit: > >> > >> commit d8bbae6ea080249c05ca90b1f8640fde48a18301 > >> Date: Fri Jan 14 15:40:59 2022 -0500 > >> > >> gdb: fix handling of vfork by multi-threaded program > >> (follow-fork-mode=parent, detach-on-fork=on) > >> > >> And tried to understand what's going on. The commit message on that > >> original commit is awesome, and well worth a read. > >> > >> After looking at that for a while I think I have a replacement for > >> this block, which I think achieves the same thing, but is structured, > >> and commented slightly differently. > >> > >> I've included an updated version of this patch below, I'd love to > >> hear what you think? > >> > >> The patch below is only partially tested at this point. I'm a little > >> short of time right now. But it passes all the vfork related tests > >> that I've been looking at, so I'm reasonably sure it's OK, I just > >> wanted to get this out so you could take a look at it. > >> > >> Thanks, > >> Andrew > >> > >> --- > >> > >> commit 45460db250f116956f4d75376f6637d2e4e7632f > >> Author: Mihails Strasuns <mihails.strasuns@intel.com> > >> Date: Mon Jun 12 09:49:27 2023 +0200 > >> > >> gdb, infrun: refactor part of `proceed` into separate function > >> > >> Split the thread resuming code from proceed into new function > >> proceed_resume_thread_checked. > >> > >> Co-Authored-By: Christina Schimpe <christina.schimpe@intel.com> > >> > >> diff --git a/gdb/infrun.c b/gdb/infrun.c index > >> 58da1cef29e..7e6104ffab2 100644 > >> --- a/gdb/infrun.c > >> +++ b/gdb/infrun.c > >> @@ -3268,6 +3268,90 @@ check_multi_target_resumption > (process_stratum_target *resume_target) > >> } > >> } > >> > >> +/* Helper function for `proceed`. Check if thread TP is suitable for > >> + resuming, and, if it is, switch to the thread and call > >> + `keep_going_pass_signal`. If TP is not suitable for resuming then this > >> + function will just return without switching threads. */ > >> + > >> +static void > >> +proceed_resume_thread_checked (thread_info *tp) { > >> + if (!tp->inf->has_execution ()) > >> + { > >> + infrun_debug_printf ("[%s] target has no execution", > >> + tp->ptid.to_string ().c_str ()); > >> + return; > >> + } > >> + > >> + if (tp->resumed ()) > >> + { > >> + infrun_debug_printf ("[%s] resumed", > >> + tp->ptid.to_string ().c_str ()); > >> + gdb_assert (tp->executing () || tp->has_pending_waitstatus ()); > >> + return; > >> + } > >> + > >> + if (thread_is_in_step_over_chain (tp)) > >> + { > >> + infrun_debug_printf ("[%s] needs step-over", > >> + tp->ptid.to_string ().c_str ()); > >> + return; > >> + } > >> + > >> + /* When handling a vfork GDB removes all breakpoints from the program > >> + space in which the vfork is being handled, as such we must take care > >> + not to resume any thread other than the vfork parent -- resuming the > >> + vfork parent allows GDB to receive and handle the 'vfork done' > >> + event. */ > >> + if (tp->inf->thread_waiting_for_vfork_done != nullptr) > >> + { > >> + if (target_is_non_stop_p ()) > >> + { > >> + /* For non-stop targets, regardless of whether GDB is using > >> + all-stop or non-stop mode, threads are controlled > >> + individually. > >> + > >> + When a thread is handling a vfork, breakpoints are removed > >> + from the inferior (well, program space in fact), so it is > >> + critical that we don't try to resume any thread other than the > >> + vfork parent. */ > >> + if (tp != tp->inf->thread_waiting_for_vfork_done) > >> + { > >> + infrun_debug_printf ("[%s] another thread of this inferior is " > >> + "waiting for vfork-done", > >> + tp->ptid.to_string ().c_str ()); > >> + return; > >> + } > > > > This actually fixes a bug that exists currently in GDB. I'm working > > on a patch which includes a test that exposes the bug. I plan to get > > the bug fix merged before we merge this refactoring commit. > > > >> + > >> + /* In non-stop mode we will never end up trying to proceed the > >> + thread that is handling a vfork. When we realise that a > >> + thread is handling a vfork we immediately stop all the other > >> + threads, and resume the vfork parent thread. As far as the > >> + user is concerned, the vfork parent thread is running. Any > >> + attempt by the user to interrupt the vfork parent will be > >> + deferred until the vfork has completed, at which point the > >> + parent will no longer be waiting for a vfork. */ > >> + gdb_assert (!non_stop); > > > > And this assertion is not correct. Again, the new test I'll add will > > trigger this assert. > > > > We can get this refactor merged once my bug fix has landed. I'll > > follow up once I have something posted. > > I finally got my vfork patches on the mailing list. In the end it felt more natural > for this refactor to live part way through that series, so that's what I've done. > You can see the series here: > > https://inbox.sourceware.org/gdb- > patches/cover.1687438786.git.aburgess@redhat.com/ > > Thanks, > Andrew Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-28 10:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-12 7:49 [PATCH v2 0/1] Refactor proceed function Christina Schimpe 2023-06-12 7:49 ` [PATCH v2 1/1] gdb, infrun: refactor part of `proceed` into separate function Christina Schimpe 2023-06-14 10:02 ` Andrew Burgess 2023-06-16 9:50 ` Andrew Burgess 2023-06-22 14:23 ` Andrew Burgess 2023-06-28 10:14 ` Schimpe, Christina
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).