Re: [PATCH v2 18/29] gdb: clear step over information on thread exit (PR gdb/27338)

[Simon Marchi <simark@simark.ca>]

> @@ -5428,6 +5443,117 @@ handle_no_resumed (struct execution_control_state *ecs)
>    return false;
>  }
>  
> +/* Handle a TARGET_WAITKIND_THREAD_EXITED event.  Return true if we
> +   handled the event and should continue waiting.  Return false if we
> +   should stop and report the event to the user.  */
> +
> +static bool
> +handle_thread_exited (execution_control_state *ecs)
> +{
> +  context_switch (ecs);
> +
> +  /* Clear these so we don't re-start the thread stepping over a
> +     breakpoint/watchpoint.  */
> +  ecs->event_thread->stepping_over_breakpoint = 0;
> +  ecs->event_thread->stepping_over_watchpoint = 0;
> +
> +  /* Maybe the thread was doing a step-over, if so release
> +     resources and start any further pending step-overs.
> +
> +     If we are on a non-stop target and the thread was doing an
> +     in-line step, this also restarts the other threads.  */
> +  int ret = finish_step_over (ecs);
> +
> +  /* finish_step_over returns true if it moves ecs' wait status
> +     back into the thread, so that we go handle another pending
> +     event before this one.  But we know it never does that if
> +     the event thread has exited.  */
> +  gdb_assert (ret == 0);
> +
> +  /* If finish_step_over started a new in-line step-over, don't
> +     try to restart anything else.  */
> +  if (step_over_info_valid_p ())
> +    {
> +      delete_thread (ecs->event_thread);
> +      return true;
> +    }
> +
> +  /* Maybe we are on an all-stop target and we got this event
> +     while doing a step-like command on another thread.  If so,
> +     go back to doing that.  If this thread was stepping,
> +     switch_back_to_stepped_thread will consider that the thread
> +     was interrupted mid-step and will try keep stepping it.  We
> +     don't want that, the thread is gone.  So clear the proceed
> +     status so it doesn't do that.  */
> +  clear_proceed_status_thread (ecs->event_thread);
> +  if (switch_back_to_stepped_thread (ecs))
> +    {
> +      delete_thread (ecs->event_thread);
> +      return true;
> +    }
> +
> +  inferior *inf = ecs->event_thread->inf;
> +  bool slock_applies = schedlock_applies (ecs->event_thread);
> +
> +  delete_thread (ecs->event_thread);
> +  ecs->event_thread = nullptr;
> +
> +  auto handle_as_no_resumed = [ecs] ()
> +  {
> +    ecs->ws.set_no_resumed ();
> +    ecs->event_thread = nullptr;
> +    ecs->ptid = minus_one_ptid;
> +    return handle_no_resumed (ecs);
> +  };

Is it really necessary to change the nature of the event?
handle_no_resumed doesn't seem to actually care about the kind in `ecs`,
so maybe you could just pass `ecs` down as-is?  I think it adds a layer
of complexity if the ecs gets modified as we handle it, it's simpler to
follow if it's immutable (other than filling in not-yet-set fields).

But the difficulty I see is that normal_stop does some things when there
are no resumed threads left.  The check there would become a bit more
complex.

> diff --git a/gdb/thread.c b/gdb/thread.c
> index 6ea05f70a41..a83db6b07fd 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -401,6 +401,8 @@ thread_info::clear_pending_waitstatus ()
>  void
>  thread_info::set_thread_options (gdb_thread_options thread_options)
>  {
> +  gdb_assert (this->state != THREAD_EXITED && !this->executing ());

I'd suggesting splitting this in two asserts.

Simon