Re: [PATCH 31/31] Cancel execution command on thread exit, when stepping, nexting, etc.

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> If your target has no support for TARGET_WAITKIND_NO_RESUMED events
> (and no way to support them, such as the yet-unsubmitted AMDGPU
> target), and you step over thread exit with scheduler-locking on, this
> is what you get:
>
>  (gdb) n
>  [Thread ... exited]
>  *hang*
>
> Getting back the prompt by typing Ctrl-C may not even work, since no
> inferior thread is running to receive the SIGINT.  Even if it works,
> it seems unnecessarily harsh.  If you started an execution command for
> which there's a clear thread of interest (step, next, until, etc.),
> and that thread disappears, then I think it's more user friendly if
> GDB just detects the situation and aborts the command, giving back the
> prompt.
>
> That is what this commit implements.  It does this by explicitly
> requesting the target to report thread exit events whenever the main
> resumed thread has a thread_fsm.  Note that unlike stepping over a
> breakpoint, we don't need to enable clone events in this case.
>
> With this patch, we get:
>
>  (gdb) n
>  [Thread 0x7ffff7d89700 (LWP 3961883) exited]
>  Command aborted, thread exited.
>  (gdb)

LGTM.

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

Thanks,
Andrew


>
> Change-Id: I901ab64c91d10830590b2dac217b5264635a2b95
> ---
>  gdb/infrun.c                                  | 73 ++++++++++++++---
>  .../gdb.threads/step-over-thread-exit.exp     | 79 ++++++++++++-------
>  2 files changed, 115 insertions(+), 37 deletions(-)
>
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 21e5aa0f50e..61d2a14c646 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1886,6 +1886,22 @@ displaced_step_prepare (thread_info *thread)
>    return status;
>  }
>  
> +/* True if any thread of TARGET that matches RESUME_PTID requires
> +   target_thread_events enabled.  This assumes TARGET does not support
> +   target thread options.  */
> +
> +static bool
> +any_thread_needs_target_thread_events (process_stratum_target *target,
> +				       ptid_t resume_ptid)
> +{
> +  for (thread_info *tp : all_non_exited_threads (target, resume_ptid))
> +    if (displaced_step_in_progress_thread (tp)
> +	|| schedlock_applies (tp)
> +	|| tp->thread_fsm () != nullptr)
> +      return true;
> +  return false;
> +}
> +
>  /* Maybe disable thread-{cloned,created,exited} event reporting after
>     a step-over (either in-line or displaced) finishes.  */
>  
> @@ -1909,9 +1925,10 @@ update_thread_events_after_step_over (thread_info *event_thread,
>    else
>      {
>        /* We can only control the target-wide target_thread_events
> -	 setting.  Disable it, but only if other threads don't need it
> -	 enabled.  */
> -      if (!displaced_step_in_progress_any_thread ())
> +	 setting.  Disable it, but only if other threads in the target
> +	 don't need it enabled.  */
> +      process_stratum_target *target = event_thread->inf->process_target ();
> +      if (!any_thread_needs_target_thread_events (target, minus_one_ptid))
>  	target_thread_events (false);
>      }
>  }
> @@ -2488,12 +2505,25 @@ do_target_resume (ptid_t resume_ptid, bool step, enum gdb_signal sig)
>        else
>  	target_thread_events (true);
>      }
> +  else if (tp->thread_fsm () != nullptr)
> +    {
> +      gdb_thread_options options = GDB_THREAD_OPTION_EXIT;
> +      if (target_supports_set_thread_options (options))
> +	tp->set_thread_options (options);
> +      else
> +	target_thread_events (true);
> +    }
>    else
>      {
>        if (target_supports_set_thread_options (0))
>  	tp->set_thread_options (0);
> -      else if (!displaced_step_in_progress_any_thread ())
> -	target_thread_events (false);
> +      else
> +	{
> +	  process_stratum_target *resume_target = tp->inf->process_target ();
> +	  if (!any_thread_needs_target_thread_events (resume_target,
> +						      resume_ptid))
> +	    target_thread_events (false);
> +	}
>      }
>  
>    /* If we're resuming more than one thread simultaneously, then any
> @@ -5671,6 +5701,13 @@ handle_thread_exited (execution_control_state *ecs)
>    ecs->event_thread->stepping_over_breakpoint = 0;
>    ecs->event_thread->stepping_over_watchpoint = 0;
>  
> +  /* If the thread had an FSM, then abort the command.  But only after
> +     finishing the step over, as in non-stop mode, aborting this
> +     thread's command should not interfere with other threads.  We
> +     must check this before finish_step over, however, which may
> +     update the thread list and delete the event thread.  */
> +  bool abort_cmd = (ecs->event_thread->thread_fsm () != nullptr);
> +
>    /* Maybe the thread was doing a step-over, if so release
>       resources and start any further pending step-overs.
>  
> @@ -5684,6 +5721,13 @@ handle_thread_exited (execution_control_state *ecs)
>       the event thread has exited.  */
>    gdb_assert (ret == 0);
>  
> +  if (abort_cmd)
> +    {
> +      delete_thread (ecs->event_thread);
> +      ecs->event_thread = nullptr;
> +      return false;
> +    }
> +
>    /* If finish_step_over started a new in-line step-over, don't
>       try to restart anything else.  */
>    if (step_over_info_valid_p ())
> @@ -9062,7 +9106,8 @@ normal_stop (void)
>        if (inferior_ptid != null_ptid)
>  	finish_ptid = ptid_t (inferior_ptid.pid ());
>      }
> -  else if (last.kind () != TARGET_WAITKIND_NO_RESUMED)
> +  else if (last.kind () != TARGET_WAITKIND_NO_RESUMED
> +	   && last.kind () != TARGET_WAITKIND_THREAD_EXITED)
>      finish_ptid = inferior_ptid;
>  
>    gdb::optional<scoped_finish_thread_state> maybe_finish_thread_state;
> @@ -9105,7 +9150,8 @@ normal_stop (void)
>      {
>        if ((last.kind () != TARGET_WAITKIND_SIGNALLED
>  	   && last.kind () != TARGET_WAITKIND_EXITED
> -	   && last.kind () != TARGET_WAITKIND_NO_RESUMED)
> +	   && last.kind () != TARGET_WAITKIND_NO_RESUMED
> +	   && last.kind () != TARGET_WAITKIND_THREAD_EXITED)
>  	  && target_has_execution ()
>  	  && previous_thread != inferior_thread ())
>  	{
> @@ -9121,7 +9167,8 @@ normal_stop (void)
>        update_previous_thread ();
>      }
>  
> -  if (last.kind () == TARGET_WAITKIND_NO_RESUMED)
> +  if (last.kind () == TARGET_WAITKIND_NO_RESUMED
> +      || last.kind () == TARGET_WAITKIND_THREAD_EXITED)
>      {
>        stop_print_frame = false;
>  
> @@ -9129,7 +9176,12 @@ normal_stop (void)
>  	if (current_ui->prompt_state == PROMPT_BLOCKED)
>  	  {
>  	    target_terminal::ours_for_output ();
> -	    gdb_printf (_("No unwaited-for children left.\n"));
> +	    if (last.kind () == TARGET_WAITKIND_NO_RESUMED)
> +	      gdb_printf (_("No unwaited-for children left.\n"));
> +	    else if (last.kind () == TARGET_WAITKIND_THREAD_EXITED)
> +	      gdb_printf (_("Command aborted, thread exited.\n"));
> +	    else
> +	      gdb_assert_not_reached ("unhandled");
>  	  }
>      }
>  
> @@ -9214,7 +9266,8 @@ normal_stop (void)
>      {
>        if (last.kind () != TARGET_WAITKIND_SIGNALLED
>  	  && last.kind () != TARGET_WAITKIND_EXITED
> -	  && last.kind () != TARGET_WAITKIND_NO_RESUMED)
> +	  && last.kind () != TARGET_WAITKIND_NO_RESUMED
> +	  && last.kind () != TARGET_WAITKIND_THREAD_EXITED)
>  	/* Delete the breakpoint we stopped at, if it wants to be deleted.
>  	   Delete any breakpoint that is to be deleted at the next stop.  */
>  	breakpoint_auto_delete (inferior_thread ()->control.stop_bpstat);
> diff --git a/gdb/testsuite/gdb.threads/step-over-thread-exit.exp b/gdb/testsuite/gdb.threads/step-over-thread-exit.exp
> index ed8534cf518..a0056740478 100644
> --- a/gdb/testsuite/gdb.threads/step-over-thread-exit.exp
> +++ b/gdb/testsuite/gdb.threads/step-over-thread-exit.exp
> @@ -29,7 +29,7 @@ if { [build_executable "failed to prepare" $testfile \
>  # NS_STOP_ALL is only used if testing "set non-stop on", and indicates
>  # whether to have GDB explicitly stop all threads before continuing to
>  # thread exit.
> -proc test {displaced-stepping non-stop target-non-stop schedlock ns_stop_all} {
> +proc test {displaced-stepping non-stop target-non-stop schedlock cmd ns_stop_all} {
>      if {${non-stop} == "off" && $ns_stop_all} {
>  	error "invalid arguments"
>      }
> @@ -72,31 +72,54 @@ proc test {displaced-stepping non-stop target-non-stop schedlock ns_stop_all} {
>  
>  	gdb_test_no_output "set scheduler-locking ${schedlock}"
>  
> -	gdb_test "continue" \
> -	    "No unwaited-for children left." \
> -	    "continue stops when thread exits"
> +	if {$cmd == "continue"} {
> +	    gdb_test "continue" \
> +		"No unwaited-for children left." \
> +		"continue stops when thread exits"
> +	} else {
> +	    gdb_test $cmd \
> +		"Command aborted, thread exited\\." \
> +		"command aborts when thread exits"
> +	}
>      } else {
>  	gdb_test_no_output "set scheduler-locking ${schedlock}"
>  
> -	for { set i 0 } { $i < 100 } { incr i } {
> -	    with_test_prefix "iter $i" {
> -		set ok 0
> -		set thread "<unknown>"
> -		gdb_test_multiple "continue" "" {
> -		    -re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" {
> -			set thread $expect_out(1,string)
> -			set ok 1
> -		    }
> -		}
> -		if {!${ok}} {
> -		    # Exit if there's a failure to avoid lengthy
> -		    # timeouts.
> -		    break
> +	if {$cmd != "continue"} {
> +	    set thread "<unknown>"
> +	    gdb_test_multiple "continue" "" {
> +		-re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" {
> +		    set thread $expect_out(1,string)
>  		}
> +	    }
> +	    if {${non-stop}} {
> +		gdb_test -nopass "thread $thread" "Switching to thread .*" \
> +		    "switch to event thread"
> +	    }
>  
> -		if {${non-stop}} {
> -		    gdb_test "thread $thread" "Switching to thread .*" \
> -			"switch to event thread"
> +	    gdb_test $cmd \
> +		"Command aborted, thread exited\\." \
> +		"command aborts when thread exits"
> +	} else {
> +	    for { set i 0 } { $i < 100 } { incr i } {
> +		with_test_prefix "iter $i" {
> +		    set ok 0
> +		    set thread "<unknown>"
> +		    gdb_test_multiple "continue" "" {
> +			-re -wrap "Thread ($::decimal) .*hit Breakpoint $::decimal.* my_exit_syscall .*" {
> +			    set thread $expect_out(1,string)
> +			    set ok 1
> +			}
> +		    }
> +		    if {!${ok}} {
> +			# Exit if there's a failure to avoid lengthy
> +			# timeouts.
> +			break
> +		    }
> +
> +		    if {${non-stop}} {
> +			gdb_test -nopass "thread $thread" "Switching to thread .*" \
> +			    "switch to event thread"
> +		    }
>  		}
>  	    }
>  	}
> @@ -112,13 +135,15 @@ foreach_with_prefix displaced-stepping {off auto} {
>  	    }
>  
>  	    foreach_with_prefix schedlock {off on} {
> -		if {${non-stop} == "on"} {
> -		    foreach_with_prefix ns_stop_all {0 1} {
> -			test ${displaced-stepping} ${non-stop} ${target-non-stop} \
> -			    ${schedlock} ${ns_stop_all}
> +		foreach_with_prefix cmd {"next" "continue"} {
> +		    if {${non-stop} == "on"} {
> +			foreach_with_prefix ns_stop_all {0 1} {
> +			    test ${displaced-stepping} ${non-stop} ${target-non-stop} \
> +				${schedlock} ${cmd} ${ns_stop_all}
> +			}
> +		    } else {
> +			test ${displaced-stepping} ${non-stop} ${target-non-stop} ${schedlock} ${cmd} 0
>  		    }
> -		} else {
> -		    test ${displaced-stepping} ${non-stop} ${target-non-stop} ${schedlock} 0
>  		}
>  	    }
>  	}
> -- 
> 2.36.0