Re: [PATCH v8 4/6] gdb: change the internal representation of scheduler locking.

[Andrew Burgess <aburgess@redhat.com>]

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

> From: Natalia Saiapova <natalia.saiapova@intel.com>
>
> Introduce a new structure to manage different options of the scheduler
> locking.  The options can coexist together and be set individually.
> In the next patch
>
>   gdb: refine commands to control scheduler locking.
>
> we introduce the commands to control these options.  In this patch we do
> not introduce new commands and keep the previous API.
>
> New scheduler locking options are:
> replay continue -- control continuing commands during replay mode.
> replay step -- control stepping commands during replay mode.
> continue -- control continuing commands during normal execution.
> step -- control stepping commands during normal execution.
>
> Internally they hold a bool value, when true the locking is enabled.
>
> Mapping to the old settings
>
> Old Settings      |  New settings
> -----------------------------------
> off               | all are false
>                   |
> replay            | continue = false, step = false,
>                   | replay continue = true, replay step = true
>                   |
> step              | continue = false, step = true,
>                   | replay continue = false, replay step = true
>                   |
> on                | all are true
> ---
>  gdb/infrun.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 134 insertions(+), 13 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index ace34507cfe..0eb890bbbc1 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -107,8 +107,12 @@ static bool start_step_over (void);
>  
>  static bool step_over_info_valid_p (void);
>  
> -static bool schedlock_applies (struct thread_info *tp);
> -static bool schedlock_applies (bool step, bool record_will_replay);
> +struct schedlock_options;
> +static bool schedlock_applies (thread_info *tp);
> +static bool schedlock_applies (bool step,
> +			       bool record_will_replay,
> +			       thread_info *tp = nullptr);
> +static bool schedlock_applies_to_opts (const schedlock_options &, bool step);
>  
>  static void handle_process_exited (struct execution_control_state *ecs);
>  
> @@ -2373,7 +2377,69 @@ infrun_thread_ptid_changed (process_stratum_target *target,
>      inferior_ptid = new_ptid;
>  }
>  
> -\f
> +/* A structure to hold scheduler locking settings for
> +   a mode, replay or normal.  */
> +struct schedlock_options
> +{
> +  struct option {

The opening '{' needs to be on the next line.

Personally, I'm a fan of nesting classes like this, but in a couple of
my recent patches I've been specifically asked not to nest classes like
this, so I think the GDB style is to avoid such nesting.

> +    const std::string name;
> +    bool value;

It would be better if these were made private.  The NAME is easy to do,
it's only used within member functions.  VALUE is a little harder, but I
had a play, and I added these member functions to the
schedlock_options::option class:

  void make_cli_option (const char *name, const char *set_doc,
			const char *show_doc, const char *help_doc,
			cmd_func_ftype *set_func,
			cmd_list_element **set_list,
			cmd_list_element **show_list)
    {
      add_setshow_boolean_cmd (name, class_run, &m_value,
			       set_doc, show_doc, help_doc,
			       set_func,
			       show_schedlock_option,
			       set_list,
			       show_list);
    }

    void make_cli_option (const char *name, const char *set_doc,
			  const char *show_doc, const char *help_doc,
			  cmd_list_element **set_list,
			  cmd_list_element **show_list)
    {
      make_cli_option (name, set_doc, show_doc, help_doc,
		       set_schedlock_callback, set_list, show_list);
    }

These can then be used instead of direct calls to
add_setshow_boolean_cmd in the 'INIT_GDB_FILE (infrun)' function.

> +
> +    option () = delete;
> +    option (std::string name, bool value)
> +      : name (std::move (name)), value (value)
> +    {}
> +
> +    /* Forbid accidential copying.  */
> +    option (const option &) = delete;
> +    option operator= (const option &) = delete;

Use: 'DISABLE_COPY_AND_ASSIGN (option);' instead.  There's also a typo:
s/accidential/accidental/, but I think the comment can go with the use
of DISABLE_COPY_AND_ASSIGN.

> +    option (option &&) = default;
> +    option &operator= (option &&) = default;
> +
> +    operator bool () const { return value; }
> +    const char *c_str () const { return value ? "on" : "off"; }
> +    /* Set new value.  Return true, if the value has changed.  */
> +    bool set (bool new_value);
> +  };
> +
> +  schedlock_options () = delete;
> +  schedlock_options (option cont, option step)
> +    : cont (std::move (cont)), step (std::move (step))
> +  {}
> +
> +  /* Forbid accidential copying.  */
> +  schedlock_options (const schedlock_options &) = delete;
> +  schedlock_options operator= (const schedlock_options &) = delete;
> +  schedlock_options (schedlock_options &&) = default;
> +  schedlock_options &operator= (schedlock_options &&) = default;

Same as above, use DISABLE_COPY_AND_ASSIGN and then remove the comment
with typo.

> +
> +  /* If true, the scheduler is locked during continuing.  */
> +  option cont;
> +  /* If true, the scheduler is locked during stepping.  */
> +  option step;
> +};
> +
> +bool
> +schedlock_options::option::set (bool new_value)
> +{
> +  if (value != new_value)
> +    {
> +      value = new_value;
> +      return true;
> +    }
> +
> +  return false;
> +}
> +
> +struct schedlock

New types should get a header comment.

> +{
> +  schedlock (schedlock_options opt, schedlock_options replay_opt)
> +    : normal (std::move (opt)), replay (std::move (replay_opt))
> +  {}
> +
> +  schedlock_options normal;
> +  schedlock_options replay;
> +};
>  
>  static const char schedlock_off[] = "off";
>  static const char schedlock_on[] = "on";
> @@ -2386,7 +2452,42 @@ static const char *const scheduler_enums[] = {
>    schedlock_replay,
>    nullptr
>  };
> +
>  static const char *scheduler_mode = schedlock_replay;
> +
> +schedlock schedlock {

Make this static maybe?  Rather than overloading the name 'schedlock'
for both the type and the variable, could we not find a better name for
one or both of these?

Actually, I'm not entirely convinced that the 'schedlock' type adds much
value above just having two separate variables with better names, e.g.:

  schedlock_options schedlock_options_normal;
  schedlock_options schedlock_options_replay;

This isn't a hard requirement though, if you are committed to the
current structure then I'll not object.

> +  {
> +    {"cont", false},
> +    {"step", false}
> +  },
> +  {
> +    {"replay cont", true},
> +    {"replay step", true}
> +  }
> +};
> +
> +/* A helper function to set scheduler locking shortcuts:
> +   set scheduler-locking on: all options are on.
> +   set scheduler-locking off: all options are off.
> +   set scheduler-locking replay: only replay options are on.
> +   set scheduler-locking step: only "step" and "replay step" are on.  */
> +
> +static void
> +set_schedlock_shortcut_option (const char *shortcut)
> +{
> +  bool is_on = (shortcut == schedlock_on);
> +  bool is_step = (shortcut == schedlock_step);
> +  bool is_replay = (shortcut == schedlock_replay);
> +  bool is_off = (shortcut == schedlock_off);
> +  /* Check that we got a valid shortcut option.  */
> +  gdb_assert (is_on || is_step || is_replay || is_off);
> +
> +  schedlock.normal.cont.set (is_on);
> +  schedlock.normal.step.set (is_on || is_step);
> +  schedlock.replay.cont.set (is_on || is_replay);
> +  schedlock.replay.step.set (is_on || is_replay || is_step);
> +}
> +
>  static void
>  show_scheduler_mode (struct ui_file *file, int from_tty,
>  		     struct cmd_list_element *c, const char *value)
> @@ -2403,9 +2504,13 @@ set_schedlock_func (const char *args, int from_tty, struct cmd_list_element *c)
>    if (!target_can_lock_scheduler ())
>      {
>        scheduler_mode = schedlock_off;
> +      /* Set scheduler locking off.  */
> +      set_schedlock_shortcut_option (schedlock_off);
>        error (_("Target '%s' cannot support this command."),
>  	     target_shortname ());
>      }
> +
> +  set_schedlock_shortcut_option (scheduler_mode);
>  }
>  
>  /* True if execution commands resume all threads of all processes by
> @@ -2436,6 +2541,10 @@ ptid_t
>  user_visible_resume_ptid (int step)
>  {
>    ptid_t resume_ptid;
> +  thread_info *tp = nullptr;
> +
> +  if (inferior_ptid != null_ptid)
> +    tp = inferior_thread ();
>  
>    if (non_stop)
>      {
> @@ -2445,14 +2554,14 @@ user_visible_resume_ptid (int step)
>      }
>    else if (schedlock_applies (step,
>  			      target_record_will_replay (inferior_ptid,
> -							 execution_direction)))
> +							 execution_direction),
> +			      tp))
>      {
>        /* User-settable 'scheduler' mode requires solo thread
>  	 resume.  */
>        resume_ptid = inferior_ptid;
>      }
> -  else if (inferior_ptid != null_ptid
> -	   && inferior_thread ()->control.in_cond_eval)
> +  else if (tp != nullptr && tp->control.in_cond_eval)
>      {
>        /* The inferior thread is evaluating a BP condition.  Other threads
>  	 might be stopped or running and we do not want to change their
> @@ -3164,8 +3273,9 @@ clear_proceed_status (int step, bool about_to_proceed)
>       This is a convenience feature to not require the user to explicitly
>       stop replaying the other threads.  We're assuming that the user's
>       intent is to resume tracing the recorded process.  */
> -  if (!non_stop && scheduler_mode == schedlock_replay
> -      && !target_record_will_replay (inferior_ptid, execution_direction))
> +  if (!non_stop && schedlock_applies_to_opts (schedlock.replay, step)
> +      && !target_record_will_replay (inferior_ptid,
> +				     execution_direction))

There's a change in behaviour here which isn't called out in the commit
message.  In fact the commit message gives the impression that this
commit is a refactor and retains the existing behaviour.

Previously we called `target_record_stop_replaying` only when in reply
mode, but now we call it when in both replay mode and the more general
'on' mode.

The comment above this `if` is definitely out of date.

I had an LLM give a summary of what changed, here's what it said:

  Setup: Several threads have been recorded. The current thread (thread
         1) has reached the end of its replay log and is no longer
         replaying. Threads 2 and 3 are still mid-replay. set
         scheduler-locking on is active.
                             
  Old behavior: target_record_stop_replaying() is not called because
                scheduler_mode != schedlock_replay. Threads 2 and 3
                remain in replay mode. They don't run (they're locked),
                but their replay state is preserved. If the user later
                does set scheduler-locking off and continues, threads 2
                and 3 resume replaying from where they left off.

  New behavior: schedlock_applies_to_opts(schedlock.replay, step) is
                true (because schedlock_on sets replay.cont = true,
                replay.step = true), so target_record_stop_replaying()
                is called. Threads 2 and 3 are pulled out of replay
                mode. If the user later switches to set
                scheduler-locking off and continues, those threads
                execute live instead of replaying — their replay state
                has been silently lost.
                                                                                                                       
  The same applies to schedlock_step when stepping: threads that were
  replaying get their replay state cleared even though they're locked
  and wouldn't have run during the step.

This seems like it makes sense, but I don't claim to be an expert at the
record/replay logic.

Maybe this change is intentional?  If it is then I think it should be
called out and described in the commit message.  The comment definitely
needs to be updated.

>      target_record_stop_replaying ();
>  
>    if (!non_stop && inferior_ptid != null_ptid)
> @@ -3241,6 +3351,17 @@ thread_still_needs_step_over (struct thread_info *tp)
>    return what;
>  }
>  
> +/* Return true if OPTS lock the scheduler.
> +   STEP indicates whether a thread is about to step.
> +   Note, this does not take into the account the mode (replay or
> +   normal execution).  */

Rephrase the last sentence to avoid 'Note, ' and remove an extra 'the':

  This function does not take into account the mode (replay or normal
  execution).

Thanks,
Andrew


> +
> +static bool
> +schedlock_applies_to_opts (const schedlock_options &opts, bool step)
> +{
> +  return ((opts.cont && !step) || (opts.step && step));
> +}
> +
>  /* Returns true if scheduler locking applies to TP.  */
>  
>  static bool
> @@ -3254,7 +3375,7 @@ schedlock_applies (thread_info *tp)
>        record_will_replay
>  	= target_record_will_replay (tp->ptid, execution_direction);
>      }
> -  return schedlock_applies (step, record_will_replay);
> +  return schedlock_applies (step, record_will_replay, tp);
>  }
>  
>  /* Returns true if scheduler locking applies.  STEP indicates whether
> @@ -3262,11 +3383,11 @@ schedlock_applies (thread_info *tp)
>     indicates whether we're about to replay.  */
>  
>  static bool
> -schedlock_applies (bool step, bool record_will_replay)
> +schedlock_applies (bool step, bool record_will_replay, thread_info *tp)
>  {
> -  return (scheduler_mode == schedlock_on
> -	  || (scheduler_mode == schedlock_step && step)
> -	  || (scheduler_mode == schedlock_replay && record_will_replay));
> +  schedlock_options &opts
> +    = record_will_replay ? schedlock.replay : schedlock.normal;
> +  return schedlock_applies_to_opts (opts, step);
>  }
>  
>  /* When FORCE_P is false, set process_stratum_target::COMMIT_RESUMED_STATE
> -- 
> 2.34.1
>
> Intel Deutschland GmbH
>
> Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
> Tel: +49 89 991 430, www.intel.de
> Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Seat: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928