Re: [PATCH v8 6/6] gdb: add eval option to lock the scheduler during infcalls.

Klaus Gerlicher <[email protected]> writes:

> From: Natalia Saiapova <[email protected]>
>
> This patch adds an "eval" scheduler locking setting to control inferior
> function calls separately from other continuing commands.
>
> "continue" handles continuing commands, such as continue, until, return,
> finish, jump.
> "eval" handles inferior calls.
>
> Show scheduler locking:
>   (gdb) show scheduler-locking
>   scheduler-locking continue:  "off"  Scheduler locking for continuing
>   commands is "off" during normal execution.
>   scheduler-locking eval:  "off"  Scheduler locking for function calls
>   is "off" during normal execution.
>   scheduler-locking replay continue:  "on"  Scheduler locking for
>   continuing commands is "on" during replay mode.
>   scheduler-locking replay eval:  "on"  Scheduler locking for function
>   calls is "on" during replay mode.
>   scheduler-locking replay step:  "on"  Scheduler locking for stepping
>   commands is "on" during replay mode.
>   scheduler-locking step:  "off"  Scheduler locking for stepping commands
>   is "off" during normal execution.
>
> Reviewed-By: Eli Zaretskii <[email protected]>
> ---
>  gdb/NEWS                                      | 13 ++--
>  gdb/doc/gdb.texinfo                           | 25 +++++---
>  gdb/infrun.c                                  | 61 +++++++++++++++----
>  .../gdb.mi/user-selected-context-sync.exp     |  3 +-
>  .../gdb.threads/hand-call-in-threads.exp      |  6 +-
>  .../multiple-successive-infcall.exp           |  5 +-
>  gdb/testsuite/gdb.threads/schedlock.exp       | 41 ++++++++++---
>  gdb/testsuite/lib/gdb.exp                     |  3 +-
>  8 files changed, 118 insertions(+), 39 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 30d0637518b..0a1f15c153e 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -905,15 +905,20 @@ list .
>  set scheduler-locking <command type> (on|off)
>  show scheduler-locking <command type>
>    where <command-type> is one of the following:
> -    continue | replay continue | replay step | step.
> -  Extend the scheduler locking settings with a set of set/show
> -  commands, which can be used individually to control the scheduler during
> -  stepping and continuing commands.  Stepping commands include step, stepi,
> +    continue | eval | replay continue | replay eval | replay step | step
> +  Extend the scheduler locking settings with a set of set/show commands,
> +  which can be used individually to control the scheduler during stepping,
> +  continuing and evaluating commands.  Stepping commands include step, stepi,
>    next.  Continuing commands include continue, finish, until, jump, return.
> +  The evaluating commands are those which invoke inferior calls.
>      'continue' -- when on, the scheduler is locked during continuing commands
>      in normal mode.
> +    'eval' -- when on, the scheduler is locked during inferior calls in
> +    normal mode.

We need to be careful referencing "normal mode".  With the context of
this patch I understand you mean "not replay mode", but as a user
approaching the NEWS file without the context of this patch, "normal
mode" doesn't have much meaning.  This comment also applies to the text
for 'continue' which I didn't spot before.

For 'eval' it would be nice to be more explicit, especially as this
links to some confusion later in this patch.  Inferior calls can be
driven directly by the user, e.g. 'print some_user_function()' but can
also be more indirect, e.g. 'break LOC if (some_user_function())'.  In
this second case the call can happen at some arbitrary time in the
future.  What are the expectations for the 'eval' setting in these two
cases?

>      'replay continue' -- when on, the scheduler is locked during continuing
>      commands in replay mode.
> +    'replay eval' -- when on, the scheduler is locked during inferior calls
> +    in replay mode.
>      'replay step' -- when on, the scheduler is locked during stepping
>      commands in replay mode.
>      'step' -- when on, the scheduler is locked during stepping commands
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index b6f7721dbf3..7f390d7db25 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2563,11 +2571,13 @@ show_schedlock_option (ui_file *file, int from_tty,
>      type = "stepping commands";
>    else if (strcmp (c->name, "continue") == 0)
>      type = "continuing commands";
> +  else if (strcmp (c->name, "eval") == 0)
> +    type = "function calls";
>    else
>      gdb_assert_not_reached ("Unexpected command name.");
>  
>    gdb_printf (file, _("\"%s\"  Scheduler locking for %s is "
> -		      "\"%s\" during the %s.\n"), value, type, value, mode);
> +		      "\"%s\" during %s.\n"), value, type, value, mode);

Ah, that's why the text in the previous commit didn't match the code.
We already discussed this text in the last commit, but whatever else
happens, this fix shouldn't live here.

>  }
>  
>  /* True if execution commands resume all threads of all processes by
> @@ -3410,13 +3420,20 @@ thread_still_needs_step_over (struct thread_info *tp)
>  
>  /* Return true if OPTS lock the scheduler.
>     STEP indicates whether a thread is about to step.
> +   While the stepping info we take from STEP argument, the inferior call
> +   state we get from the thread TP.
>     Note, this does not take into the account the mode (replay or
>     normal execution).  */
>  
>  static bool
> -schedlock_applies_to_opts (const schedlock_options &opts, bool step)
> +schedlock_applies_to_opts (const schedlock_options &opts, bool step,
> +			   thread_info *tp)
>  {
> -  return ((opts.cont && !step) || (opts.step && step));
> +  bool in_infcall = (tp != nullptr) && tp->control.in_infcall;
> +
> +  return ((opts.cont && !step && !in_infcall)
> +	  || (opts.step && step)

We don't consider IN_INFCALL here.  Does this trigger if we are stepping
over a conditional breakpoint where the condition includes an inferior
function call?  What is the expected and actual behaviour here?  Is this
case tested?  It seems like this change is worth discussing even if the
code as written is correct, as it's surprising (at least to me).

> +	  || (opts.eval && in_infcall));
>  }
>  
>  /* Returns true if scheduler locking applies to TP.  */

In a previous comment we discussed the schedlock_applies_to_opts call in
clear_proceed_status.  I was surprised that this doesn't now pass
through the thread pointer.  If it did then schedlock_applies_to_opts
would no longer need to default the thread pointer to NULL.

In fact, I would go as far as to say that even if passing NULL in that
case is correct then schedlock_applies_to_op should remove the default
argument value and clear_proceed_status should explicitly pass NULL and
should gain a comment explaining why passing NULL is the correct
solution.

> diff --git a/gdb/testsuite/gdb.threads/schedlock.exp b/gdb/testsuite/gdb.threads/schedlock.exp
> index cda4585ca08..58f8d9ae4de 100644
> --- a/gdb/testsuite/gdb.threads/schedlock.exp
> +++ b/gdb/testsuite/gdb.threads/schedlock.exp
> @@ -364,17 +365,39 @@ proc test_schedlock_opts {cont step} {
>  	my_continue "continue"
>  	check_result "continue" $curthread $cont_args $locked
>      }
> +
> +    # Infcall tests.
> +    set locked 0
> +    if {$eval eq "on"} {
> +	set locked 1
> +    }
> +    with_test_prefix "cmd=infcall" {
> +	# Use whichever we stopped in.
> +	set curthread [get_current_thread "before-infcall"]
> +	set cont_args [get_args "before-infcall"]
> +
> +	for {set i 0} {[expr $i < 10]} {set i [expr $i + 1]} {

This should be:

  for { set i 0 } { $i < 10 } { incr i } {

> +	    with_test_prefix "infcall #$i" {
> +		gdb_test "print some_function()" ".*"
> +	    }
> +	}
> +
> +	check_result "infcall" $curthread $cont_args $locked
> +    }
>  }
>  
>  gdb_test_no_output "set scheduler-locking off"
>  
>  # Test different options of scheduler locking.
>  foreach cont {"off" "on"} {
> -    foreach step {"off" "on"} {
> -	with_test_prefix "continue=$cont step=$step" {
> -	    gdb_test_no_output "set scheduler-locking continue $cont"
> -	    gdb_test_no_output "set scheduler-locking step $step"
> -	    test_schedlock_opts $cont $step
> +    foreach eval {"off" "on"} {
> +	foreach step {"off" "on"} {
> +	    with_test_prefix "continue=$cont eval=$eval step=$step" {
> +		gdb_test_no_output "set scheduler-locking continue $cont"
> +		gdb_test_no_output "set scheduler-locking eval $eval"
> +		gdb_test_no_output "set scheduler-locking step $step"
> +		test_schedlock_opts $cont $eval $step
> +	    }
>  	}
>      }
>  }

Thanks,
Andrew