Re: [PATCH 20/31] gdb: clear step over information on thread exit (PR gdb/27338)

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> GDB doesn't handle correctly the case where a thread steps over a
> breakpoint (using either in-line or displaced stepping), and the
> executed instruction causes the thread to exit.
>
> Using the test program included later in the series, this is what it
> looks like with displaced-stepping, on x86-64 Linux, where we have two
> displaced-step buffers:
>
>   $ ./gdb -q -nx --data-directory=data-directory build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit -ex "b my_exit_syscall" -ex r
>   Reading symbols from build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit...
>   Breakpoint 1 at 0x123c: file src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 68.
>   Starting program: build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit
>   [Thread debugging using libthread_db enabled]
>   Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
>   [New Thread 0x7ffff7c5f640 (LWP 2915510)]
>   [Switching to Thread 0x7ffff7c5f640 (LWP 2915510)]
>
>   Thread 2 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
>   68              syscall
>   (gdb) c
>   Continuing.
>   [New Thread 0x7ffff7c5f640 (LWP 2915524)]
>   [Thread 0x7ffff7c5f640 (LWP 2915510) exited]
>   [Switching to Thread 0x7ffff7c5f640 (LWP 2915524)]
>
>   Thread 3 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
>   68              syscall
>   (gdb) c
>   Continuing.
>   [New Thread 0x7ffff7c5f640 (LWP 2915616)]
>   [Thread 0x7ffff7c5f640 (LWP 2915524) exited]
>   [Switching to Thread 0x7ffff7c5f640 (LWP 2915616)]
>
>   Thread 4 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
>   68              syscall
>   (gdb) c
>   Continuing.
>   ... hangs ...
>
> The first two times we do "continue", we displaced-step the syscall
> instruction that causes the thread to exit.  When the thread exits,
> the main thread, waiting on pthread_join, is unblocked.  It spawns a
> new thread, which hits the breakpoint on the syscall again.  However,
> infrun was never notified that the displaced-stepping threads are done
> using the displaced-step buffer, so now both buffers are marked as
> used.  So when we do the third continue, there are no buffers
> available to displaced-step the syscall, so the thread waits forever
> for its turn.
>
> When trying the same but with in-line step over (displaced-stepping
> disabled):
>
>   $ ./gdb -q -nx --data-directory=data-directory \
>   build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit \
>     -ex "b my_exit_syscall" -ex "set displaced-stepping off" -ex r
>   Reading symbols from build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit...
>   Breakpoint 1 at 0x123c: file src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S, line 68.
>   Starting program: build/binutils-gdb/gdb/testsuite/outputs/gdb.threads/step-over-thread-exit/step-over-thread-exit
>   [Thread debugging using libthread_db enabled]
>   Using host libthread_db library "/usr/lib/../lib/libthread_db.so.1".
>   [New Thread 0x7ffff7c5f640 (LWP 2928290)]
>   [Switching to Thread 0x7ffff7c5f640 (LWP 2928290)]
>
>   Thread 2 "step-over-threa" hit Breakpoint 1, my_exit_syscall () at src/binutils-gdb/gdb/testsuite/lib/my-syscalls.S:68
>   68              syscall
>   (gdb) c
>   Continuing.
>   [Thread 0x7ffff7c5f640 (LWP 2928290) exited]
>   No unwaited-for children left.
>   (gdb) i th
>     Id   Target Id                                             Frame
>     1    Thread 0x7ffff7c60740 (LWP 2928285) "step-over-threa" 0x00007ffff7f7c9b7 in __pthread_clockjoin_ex () from /usr/lib/libpthread.so.0
>
>   The current thread <Thread ID 2> has terminated.  See `help thread'.
>   (gdb) thread 1
>   [Switching to thread 1 (Thread 0x7ffff7c60740 (LWP 2928285))]
>   #0  0x00007ffff7f7c9b7 in __pthread_clockjoin_ex () from /usr/lib/libpthread.so.0
>   (gdb) c
>   Continuing.
>   ^C^C
>   ... hangs ...
>
> The "continue" causes an in-line step to occur, meaning the main
> thread is stopped while we step the syscall.  The stepped thread exits
> when executing the syscall, the linux-nat target notices there are no
> more resumed threads to be waited for, so returns
> TARGET_WAITKIND_NO_RESUMED, which causes the prompt to return.  But
> infrun never clears the in-line step over info.  So if we try
> continuing the main thread, GDB doesn't resume it, because it thinks
> there's an in-line step in progress that we need to wait for to
> finish, and we are stuck there.
>
> To fix this, infrun needs to be informed when a thread doing a
> displaced or in-line step over exits.  We can do that with the new
> target_set_thread_options mechanism which is optimal for only enabling
> exit events of the thread that needs it; or, if that is not supported,
> by using target_thread_events, which enables thread exit events for
> all threads.  This is done by this commit.
>
> This patch then modifies handle_inferior_event in infrun.c to clean up
> any step-over the exiting thread might have been doing at the time of
> the exit.  The cases to consider are:
>
>  - the exiting thread was doing an in-line step-over with an all-stop
>    target
>  - the exiting thread was doing an in-line step-over with a non-stop
>    target
>  - the exiting thread was doing a displaced step-over with a non-stop
>    target
>
> The displaced-stepping buffer implementation in displaced-stepping.c
> is modified to account for the fact that it's possible that we
> "finish" a displaced step after a thread exit event.  The buffer that
> the exiting thread was using is marked as available again and the
> original instructions under the scratch pad are restored.  However, it
> skips applying the fixup, which wouldn't make sense since the thread
> does not exist anymore.
>
> Another case that needs handling is if a displaced-stepping thread
> exits, and the event is reported while we are in stop_all_threads.  We
> should call displaced_step_finish in the handle_one function, in that
> case.  It was already called in other code paths, just not the "thread
> exited" path.
>
> This commit doesn't make infrun ask the target to report the
> TARGET_WAITKIND_THREAD_EXITED events yet, that'll be done later in the
> series.
>
> Note that "stop_print_frame = false;" line is moved to normal_stop,
> because TARGET_WAITKIND_THREAD_EXITED can also end up with the event
> transmorphed into TARGET_WAITKIND_NO_RESUMED.  Moving it to
> normal_stop keeps it centralized.

LGTM.

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

Thanks,
Andrew

>
> Co-authored-by: Simon Marchi <simon.marchi@efficios.com>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=27338
> Change-Id: I745c6955d7ef90beb83bcf0ff1d1ac8b9b6285a5
> ---
>  gdb/displaced-stepping.c  |   7 ++
>  gdb/gdbarch-components.py |   4 +
>  gdb/gdbarch-gen.h         |   6 +-
>  gdb/infrun.c              | 171 ++++++++++++++++++++++++++++++++++----
>  gdb/thread.c              |   3 +
>  5 files changed, 174 insertions(+), 17 deletions(-)
>
> diff --git a/gdb/displaced-stepping.c b/gdb/displaced-stepping.c
> index 7b5d327008d..aa8571d51e2 100644
> --- a/gdb/displaced-stepping.c
> +++ b/gdb/displaced-stepping.c
> @@ -254,6 +254,13 @@ displaced_step_buffers::finish (gdbarch *arch, thread_info *thread,
>  			  thread->ptid.to_string ().c_str (),
>  			  paddress (arch, buffer->addr));
>  
> +  /* If the thread exited while stepping, we are done.  The code above
> +     made the buffer available again, and we restored the bytes in the
> +     buffer.  We don't want to run the fixup: since the thread is now
> +     dead there's nothing to adjust.  */
> +  if (status.kind () == TARGET_WAITKIND_THREAD_EXITED)
> +    return DISPLACED_STEP_FINISH_STATUS_OK;
> +
>    regcache *rc = get_thread_regcache (thread);
>  
>    bool instruction_executed_successfully
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 5d60f7677f0..875c784dc0f 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -1826,6 +1826,10 @@ Throw an exception if any unexpected error happens.
>  Method(
>      comment="""
>  Clean up after a displaced step of THREAD.
> +
> +It is possible for the displaced-stepped instruction to have caused
> +the thread to exit.  The implementation can detect this case by
> +checking if WS.kind is TARGET_WAITKIND_THREAD_EXITED.
>  """,
>      type="displaced_step_finish_status",
>      name="displaced_step_finish",
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 5c9390ea6b3..69212216f03 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -1078,7 +1078,11 @@ typedef displaced_step_prepare_status (gdbarch_displaced_step_prepare_ftype) (st
>  extern displaced_step_prepare_status gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, thread_info *thread, CORE_ADDR &displaced_pc);
>  extern void set_gdbarch_displaced_step_prepare (struct gdbarch *gdbarch, gdbarch_displaced_step_prepare_ftype *displaced_step_prepare);
>  
> -/* Clean up after a displaced step of THREAD. */
> +/* Clean up after a displaced step of THREAD.
> +
> +   It is possible for the displaced-stepped instruction to have caused
> +   the thread to exit.  The implementation can detect this case by
> +   checking if WS.kind is TARGET_WAITKIND_THREAD_EXITED. */
>  
>  typedef displaced_step_finish_status (gdbarch_displaced_step_finish_ftype) (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws);
>  extern displaced_step_finish_status gdbarch_displaced_step_finish (struct gdbarch *gdbarch, thread_info *thread, const target_waitstatus &ws);
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index e47e3c688e7..2866962d2dc 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1888,13 +1888,15 @@ displaced_step_prepare (thread_info *thread)
>     a step-over (either in-line or displaced) finishes.  */
>  
>  static void
> -update_thread_events_after_step_over (thread_info *event_thread)
> +update_thread_events_after_step_over (thread_info *event_thread,
> +				      const target_waitstatus &event_status)
>  {
>    if (target_supports_set_thread_options (0))
>      {
>        /* We can control per-thread options.  Disable events for the
> -	 event thread.  */
> -      event_thread->set_thread_options (0);
> +	 event thread, unless the thread is gone.  */
> +      if (event_status.kind () != TARGET_WAITKIND_THREAD_EXITED)
> +	event_thread->set_thread_options (0);
>      }
>    else
>      {
> @@ -1950,7 +1952,7 @@ displaced_step_finish (thread_info *event_thread,
>    if (!displaced->in_progress ())
>      return DISPLACED_STEP_FINISH_STATUS_OK;
>  
> -  update_thread_events_after_step_over (event_thread);
> +  update_thread_events_after_step_over (event_thread, event_status);
>  
>    gdb_assert (event_thread->inf->displaced_step_state.in_progress_count > 0);
>    event_thread->inf->displaced_step_state.in_progress_count--;
> @@ -4054,6 +4056,7 @@ struct wait_one_event
>  };
>  
>  static bool handle_one (const wait_one_event &event);
> +static int finish_step_over (struct execution_control_state *ecs);
>  
>  /* Prepare and stabilize the inferior for detaching it.  E.g.,
>     detaching while a thread is displaced stepping is a recipe for
> @@ -5181,6 +5184,16 @@ handle_one (const wait_one_event &event)
>  				      event.ws);
>  	  save_waitstatus (t, event.ws);
>  	  t->stop_requested = false;
> +
> +	  if (event.ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
> +	    {
> +	      if (displaced_step_finish (t, event.ws)
> +		  != DISPLACED_STEP_FINISH_STATUS_OK)
> +		{
> +		  gdb_assert_not_reached ("displaced_step_finish on "
> +					  "exited thread failed");
> +		}
> +	    }
>  	}
>      }
>    else
> @@ -5392,7 +5405,9 @@ stop_all_threads (const char *reason, inferior *inf)
>      }
>  }
>  
> -/* Handle a TARGET_WAITKIND_NO_RESUMED event.  */
> +/* Handle a TARGET_WAITKIND_NO_RESUMED 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_no_resumed (struct execution_control_state *ecs)
> @@ -5520,6 +5535,125 @@ 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;
> +
> +  /* Continue handling the event as if we had gotten a
> +     TARGET_WAITKIND_NO_RESUMED.  */
> +  auto handle_as_no_resumed = [ecs] ()
> +  {
> +    /* handle_no_resumed doesn't really look at the event kind, but
> +       normal_stop does.  */
> +    ecs->ws.set_no_resumed ();
> +    ecs->event_thread = nullptr;
> +    ecs->ptid = minus_one_ptid;
> +
> +    /* Re-record the last target status.  */
> +    set_last_target_status (ecs->target, ecs->ptid, ecs->ws);
> +
> +    return handle_no_resumed (ecs);
> +  };
> +
> +  /* If we are on an all-stop target, the target has stopped all
> +     threads to report the event.  We don't actually want to
> +     stop, so restart the threads.  */
> +  if (!target_is_non_stop_p ())
> +    {
> +      if (slock_applies)
> +	{
> +	  /* Since the target is !non-stop, then everything is stopped
> +	     at this point, and we can't assume we'll get further
> +	     events until we resume the target again.  Handle this
> +	     event like if it were a TARGET_WAITKIND_NO_RESUMED.  Note
> +	     this refreshes the thread list and checks whether there
> +	     are other resumed threads before deciding whether to
> +	     print "no-unwaited-for left".  This is important because
> +	     the user could have done:
> +
> +	      (gdb) set scheduler-locking on
> +	      (gdb) thread 1
> +	      (gdb) c&
> +	      (gdb) thread 2
> +	      (gdb) c
> +
> +	     ... and only one of the threads exited.  */
> +	  return handle_as_no_resumed ();
> +	}
> +      else
> +	{
> +	  /* Switch to the first non-exited thread we can find, and
> +	     resume.  */
> +	  auto range = inf->non_exited_threads ();
> +	  if (range.begin () == range.end ())
> +	    {
> +	      /* Looks like the target reported a
> +		 TARGET_WAITKIND_THREAD_EXITED for its last known
> +		 thread.  */
> +	      return handle_as_no_resumed ();
> +	    }
> +	  thread_info *non_exited_thread = *range.begin ();
> +	  switch_to_thread (non_exited_thread);
> +	  insert_breakpoints ();
> +	  resume (GDB_SIGNAL_0);
> +	}
> +    }
> +
> +  prepare_to_wait (ecs);
> +  return true;
> +}
> +
>  /* Given an execution control state that has been freshly filled in by
>     an event from the inferior, figure out what it means and take
>     appropriate action.
> @@ -5558,15 +5692,6 @@ handle_inferior_event (struct execution_control_state *ecs)
>        return;
>      }
>  
> -  if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
> -    {
> -      ecs->event_thread = find_thread_ptid (ecs->target, ecs->ptid);
> -      gdb_assert (ecs->event_thread != nullptr);
> -      delete_thread (ecs->event_thread);
> -      prepare_to_wait (ecs);
> -      return;
> -    }
> -
>    if (ecs->ws.kind () == TARGET_WAITKIND_NO_RESUMED
>        && handle_no_resumed (ecs))
>      return;
> @@ -5581,7 +5706,6 @@ handle_inferior_event (struct execution_control_state *ecs)
>      {
>        /* No unwaited-for children left.  IOW, all resumed children
>  	 have exited.  */
> -      stop_print_frame = false;
>        stop_waiting (ecs);
>        return;
>      }
> @@ -5730,6 +5854,12 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	keep_going (ecs);
>        return;
>  
> +    case TARGET_WAITKIND_THREAD_EXITED:
> +      if (handle_thread_exited (ecs))
> +	return;
> +      stop_waiting (ecs);
> +      break;
> +
>      case TARGET_WAITKIND_EXITED:
>      case TARGET_WAITKIND_SIGNALLED:
>        {
> @@ -6175,7 +6305,7 @@ finish_step_over (struct execution_control_state *ecs)
>  	 back an event.  */
>        gdb_assert (ecs->event_thread->control.trap_expected);
>  
> -      update_thread_events_after_step_over (ecs->event_thread);
> +      update_thread_events_after_step_over (ecs->event_thread, ecs->ws);
>  
>        clear_step_over_info ();
>      }
> @@ -6221,6 +6351,13 @@ finish_step_over (struct execution_control_state *ecs)
>        if (ecs->event_thread->stepping_over_watchpoint)
>  	return 0;
>  
> +      /* The code below is meant to avoid one thread hogging the event
> +	 loop by doing constant in-line step overs.  If the stepping
> +	 thread exited, there's no risk for this to happen, so we can
> +	 safely let our caller process the event immediately.  */
> +      if (ecs->ws.kind () == TARGET_WAITKIND_THREAD_EXITED)
> +       return 0;
> +
>        pending = iterate_over_threads (resumed_thread_with_pending_status,
>  				      nullptr);
>        if (pending != nullptr)
> @@ -8859,6 +8996,8 @@ normal_stop (void)
>  
>    if (last.kind () == TARGET_WAITKIND_NO_RESUMED)
>      {
> +      stop_print_frame = false;
> +
>        SWITCH_THRU_ALL_UIS ()
>  	if (current_ui->prompt_state == PROMPT_BLOCKED)
>  	  {
> diff --git a/gdb/thread.c b/gdb/thread.c
> index d607ad9303a..2c45d528bba 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -401,6 +401,9 @@ thread_info::clear_pending_waitstatus ()
>  void
>  thread_info::set_thread_options (gdb_thread_options thread_options)
>  {
> +  gdb_assert (this->state != THREAD_EXITED);
> +  gdb_assert (!this->executing ());
> +
>    if (m_thread_options == thread_options)
>      return;
>  
> -- 
> 2.36.0