[PATCH 1/1] gdb, infrun: refactor part of `proceed` into separate function

Bruno Larsen blarsen@redhat.com
Wed Jun 7 09:26:34 GMT 2023


On 07/06/2023 09:09, Christina Schimpe via Gdb-patches wrote:
> From: Mihails Strasuns <mihails.strasuns@intel.com>
>
> Split thread resuming block into separate function.

Hi Christina, thanks for picking this one up. Unrelated to the changes, 
I think your email client got some sort of hiccup, since patch 0 isn't 
shown as related to this one. Not sure what you did different from your 
previous patches, since the other ones were fine, but just thought I 
would mention it :)

I also have one comment on the patch, inlined.

> ---
>   gdb/infrun.c | 119 ++++++++++++++++++++++++++-------------------------
>   1 file changed, 60 insertions(+), 59 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..571cf29ef32 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -3268,6 +3268,63 @@ check_multi_target_resumption (process_stratum_target *resume_target)
>       }
>   }
>   
> +/* Helper function for `proceed`, does bunch of checks to see
> +   if a thread can be resumed right now, switches to that thread
> +   and moves on to `keep_going_pass_signal`.  */
> +
> +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;
> +    }
> +
> +  /* 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))

I'm not sure this if statement is entirely correct. Let me explain how I 
understood it, and correct me if I am wrong anywhere, please.

That statement seems to be a mix between the one on line 3486 and the 
one on line 3505, first one being:

(tp->inf->thread_waiting_for_vfork_done != nullptr && tp != 
tp->inf_thread_waiting_for_vfork_done)

And the second is

(!tp->resumed () && !thread_is_in_step_over_chain (tp) && !(non_stop && 
tp->inf->thread_waiting_for_vfork_done != nullptr))

To save my sanity, I'll shorten them to a single letter. So my 
understanding is that that condition triggers on:

(A && B) || (C && D && !(E && A))

The new condition, on the other hand, is (A && (B || E)), which expands 
to (A && B) || (!(A + E)).  The first half is correct, but the second 
one is nowhere near the second part. Along with that, there are multiple 
early exits that I don't understand the code well enough to know if they 
could be triggered in the else call.

All this is to say, I think the final else if in the original function 
should not be changed, and this if should be simplified to the original 
condition.

If you would still like to avoid code repetition, I think the best is 
taking the lines that set a thread running into its own function, but 
that is up to you.

I hope this makes sense... and if I am mistaken, please explain it to me :-)

-- 
Cheers,
Bruno

> +    {
> +      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 +3513,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 ();
>     }



More information about the Gdb-patches mailing list