Re: [PATCH v8 1/6] gdb: use schedlock_applies in user_visible_resume_ptid.

[Andrew Burgess <aburgess@redhat.com>]

Klaus Gerlicher <klaus.gerlicher@intel.com> writes:

> From: Natalia Saiapova <natalia.saiapova@intel.com>
>
> This is a refactoring.  The logic in user_visible_resume_ptid is very
> similar to schedlock_applies, but uses `step` and `record_will_replay`
> parameter instead of `tp->control.stepping_command`.
>
> Refactor schedlock_applies logic into the following two overloaded methods:
>   bool schedlock_applies (thread_info *tp)
> and
>   bool schedlock_applies (bool step, bool record_will_replay)
> such that they share the logic.
>
> Update the call-sites accordingly, where we have only the thread, use
> the former, and where we have the bool step or record_will_replay use the
> latter.
>
> Approved-By: Tom Tromey <tom@tromey.com>
> ---
>  gdb/infrun.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c0767e7f764..ace34507cfe 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -108,6 +108,7 @@ static bool start_step_over (void);
>  static bool step_over_info_valid_p (void);
>  
>  static bool schedlock_applies (struct thread_info *tp);

You dropped the 'struct' from the definition of this function.  As
you're changing the next line anyway, maybe drop the 'struct' here too?

> +static bool schedlock_applies (bool step, bool record_will_replay);
>  
>  static void handle_process_exited (struct execution_control_state *ecs);
>  
> @@ -2442,20 +2443,14 @@ user_visible_resume_ptid (int step)
>  	 individually.  */
>        resume_ptid = inferior_ptid;
>      }
> -  else if ((scheduler_mode == schedlock_on)
> -	   || (scheduler_mode == schedlock_step && step))
> +  else if (schedlock_applies (step,
> +			      target_record_will_replay (inferior_ptid,
> +							 execution_direction)))
>      {
>        /* User-settable 'scheduler' mode requires solo thread
>  	 resume.  */
>        resume_ptid = inferior_ptid;
>      }
> -  else if ((scheduler_mode == schedlock_replay)
> -	   && target_record_will_replay (inferior_ptid, execution_direction))
> -    {
> -      /* User-settable 'scheduler' mode requires solo thread resume in replay
> -	 mode.  */
> -      resume_ptid = inferior_ptid;
> -    }
>    else if (inferior_ptid != null_ptid
>  	   && inferior_thread ()->control.in_cond_eval)
>      {
> @@ -3246,17 +3241,32 @@ thread_still_needs_step_over (struct thread_info *tp)
>    return what;
>  }
>  
> +/* Returns true if scheduler locking applies to TP.  */
> +
> +static bool
> +schedlock_applies (thread_info *tp)
> +{
> +  bool step = false;
> +  bool record_will_replay = false;
> +  if (tp != nullptr)
> +    {
> +      step = tp->control.stepping_command;
> +      record_will_replay
> +	= target_record_will_replay (tp->ptid, execution_direction);
> +    }
> +  return schedlock_applies (step, record_will_replay);
> +}

As far as I can tell the old code didn't have the 'tp != nullptr' check,
and I think every call site already makes the assumption that tp is not
NULL (i.e. if it was NULL GDB would have already crashed).

I think you should just update the comment on this function to say:
"...applies to non-NULL thread TP." and add 'gdb_assert (tp !=
nullptr);' to the function body.

With that, it looks good.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew