Re: [PATCH 24/31] Report thread exit event for leader if reporting thread exit events

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> If GDB sets the GDB_THREAD_OPTION_EXIT option on a thread, then if the
> thread disappears from the thread list, GDB expects to shortly see a
> thread exit event for it.  See e.g., here, in
> remote_target::update_thread_list():
>
>     /* Do not remove the thread if we've requested to be
>        notified of its exit.  For example, the thread may be
>        displaced stepping, infrun will need to handle the
>        exit event, and displaced stepping info is recorded
>        in the thread object.  If we deleted the thread now,
>        we'd lose that info.  */
>     if ((tp->thread_options () & GDB_THREAD_OPTION_EXIT) != 0)
>       continue;
>
> There's one scenario that is deleting a thread from the
> remote/gdbserver thread list without ever reporting a corresponding
> thread exit event though -- check_zombie_leaders.  This can lead to
> GDB getting confused.  For example, with a following patch that
> enables GDB_THREAD_OPTION_EXIT whenever schedlock is enabled, we'd see
> this regression:
>
>  $ make check RUNTESTFLAGS="--target_board=native-extended-gdbserver" TESTS="gdb.threads/no-unwaited-for-left.exp"
>  ...
>  Running src/gdb/testsuite/gdb.threads/no-unwaited-for-left.exp ...
>  FAIL: gdb.threads/no-unwaited-for-left.exp: continue stops when the main thread exits (timeout)
>  ... some more cascading FAILs ...
>
> gdb.log shows:
>
>  (gdb) continue
>  Continuing.
>  FAIL: gdb.threads/no-unwaited-for-left.exp: continue stops when the main thread exits (timeout)
>
> A passing run would have resulted in:
>
>  (gdb) continue
>  Continuing.
>  No unwaited-for children left.
>  (gdb) PASS: gdb.threads/no-unwaited-for-left.exp: continue stops when the main thread exits
>
> note how the leader thread is not listed in the remote-reported XML
> thread list below:
>
>  (gdb) set debug remote 1
>  (gdb) set debug infrun 1
>  (gdb) info threads
>    Id   Target Id                                Frame
>  * 1    Thread 1163850.1163850 "no-unwaited-for" main () at /home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.threads/no-unwaited-for-left.c:65
>    3    Thread 1163850.1164130 "no-unwaited-for" [remote] Sending packet: $Hgp11c24a.11c362#39
>  (gdb) c
>  Continuing.
>  [infrun] clear_proceed_status_thread: 1163850.1163850.0
>  ...
>      [infrun] resume_1: step=1, signal=GDB_SIGNAL_0, trap_expected=1, current thread [1163850.1163850.0] at 0x55555555534f
>      [remote] Sending packet: $QPassSignals:#f3
>      [remote] Packet received: OK
>      [remote] Sending packet: $QThreadOptions;3:p11c24a.11c24a#f3
>      [remote] Packet received: OK
>  ...
>      [infrun] target_set_thread_options: [options for Thread 1163850.1163850 are now 0x3]
>  ...
>    [infrun] do_target_resume: resume_ptid=1163850.1163850.0, step=0, sig=GDB_SIGNAL_0
>    [remote] Sending packet: $vCont;c:p11c24a.11c24a#98
>    [infrun] prepare_to_wait: prepare_to_wait
>    [infrun] reset: reason=handling event
>    [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target extended-remote
>    [infrun] maybe_call_commit_resumed_all_targets: calling commit_resumed for target extended-remote
>  [infrun] fetch_inferior_event: exit
>  [infrun] fetch_inferior_event: enter
>    [infrun] scoped_disable_commit_resumed: reason=handling event
>    [infrun] random_pending_event_thread: None found.
>    [remote] wait: enter
>      [remote] Packet received: N
>    [remote] wait: exit
>    [infrun] print_target_wait_results: target_wait (-1.0.0 [process -1], status) =
>    [infrun] print_target_wait_results:   -1.0.0 [process -1],
>    [infrun] print_target_wait_results:   status->kind = NO_RESUMED
>    [infrun] handle_inferior_event: status->kind = NO_RESUMED
>    [remote] Sending packet: $Hgp0.0#ad
>    [remote] Packet received: OK
>    [remote] Sending packet: $qXfer:threads:read::0,1000#92
>    [remote] Packet received: l<threads>\n<thread id="p11c24a.11c362" core="0" name="no-unwaited-for" handle="0097d8f7ff7f0000"/>\n</threads>\n
>    [infrun] handle_no_resumed: TARGET_WAITKIND_NO_RESUMED (ignoring: found resumed)
>  ...
>
> ... however, infrun decided there was a resumed thread still, so
> ignored the TARGET_WAITKIND_NO_RESUMED event.  Debugging GDB, we see
> that the "found resumed" thread that GDB finds, is the leader thread.
> Even though that thread is not on the remote-reported thread list, it
> is still on the GDB thread list, due to the special case in remote.c
> mentioned above.
>
> This commit addresses the issue by fixing GDBserver to report a thread
> exit event for the zombie leader too, i.e., making GDBserver respect
> the "if thread has GDB_THREAD_OPTION_EXIT set, report a thread exit"
> invariant.  To do that, it takes a bit more code than one would
> imagine off hand, due to the fact that we currently always report LWP
> exit pending events as TARGET_WAITKIND_EXITED, and then decide whether
> to convert it to TARGET_WAITKIND_THREAD_EXITED just before reporting
> the event to GDBserver core.  For the zombie leader scenario
> described, we need to record early on that we want to report a
> THREAD_EXITED event, and then make sure that decision isn't lost along
> the way to reporting the event to GDBserver core.

LGTM.

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

Thanks,
Andrew

>
> Change-Id: I1e68fccdbc9534434dee07163d3fd19744c8403b
> ---
>  gdbserver/linux-low.cc | 75 ++++++++++++++++++++++++++++++++++++------
>  gdbserver/linux-low.h  |  5 +--
>  2 files changed, 68 insertions(+), 12 deletions(-)
>
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index bf6dc1d995a..c197846810c 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -279,7 +279,8 @@ int using_threads = 1;
>  static int stabilizing_threads;
>  
>  static void unsuspend_all_lwps (struct lwp_info *except);
> -static void mark_lwp_dead (struct lwp_info *lwp, int wstat);
> +static void mark_lwp_dead (struct lwp_info *lwp, int wstat,
> +			   bool thread_event);
>  static int lwp_is_marked_dead (struct lwp_info *lwp);
>  static int kill_lwp (unsigned long lwpid, int signo);
>  static void enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info);
> @@ -1800,10 +1801,12 @@ iterate_over_lwps (ptid_t filter,
>    return get_thread_lwp (thread);
>  }
>  
> -void
> +bool
>  linux_process_target::check_zombie_leaders ()
>  {
> -  for_each_process ([this] (process_info *proc)
> +  bool new_pending_event = false;
> +
> +  for_each_process ([&] (process_info *proc)
>      {
>        pid_t leader_pid = pid_of (proc);
>        lwp_info *leader_lp = find_lwp_pid (ptid_t (leader_pid));
> @@ -1872,9 +1875,19 @@ linux_process_target::check_zombie_leaders ()
>  				"(it exited, or another thread execd), "
>  				"deleting it.",
>  				leader_pid);
> -	  delete_lwp (leader_lp);
> +
> +	  thread_info *leader_thread = get_lwp_thread (leader_lp);
> +	  if (report_exit_events_for (leader_thread))
> +	    {
> +	      mark_lwp_dead (leader_lp, W_EXITCODE (0, 0), true);
> +	      new_pending_event = true;
> +	    }
> +	  else
> +	    delete_lwp (leader_lp);
>  	}
>      });
> +
> +  return new_pending_event;
>  }
>  
>  /* Callback for `find_thread'.  Returns the first LWP that is not
> @@ -2333,7 +2346,7 @@ linux_process_target::filter_event (int lwpid, int wstat)
>  	  /* Since events are serialized to GDB core, and we can't
>  	     report this one right now.  Leave the status pending for
>  	     the next time we're able to report it.  */
> -	  mark_lwp_dead (child, wstat);
> +	  mark_lwp_dead (child, wstat, false);
>  	  return;
>  	}
>        else
> @@ -2646,7 +2659,8 @@ linux_process_target::wait_for_event_filtered (ptid_t wait_ptid,
>  
>        /* Check for zombie thread group leaders.  Those can't be reaped
>  	 until all other threads in the thread group are.  */
> -      check_zombie_leaders ();
> +      if (check_zombie_leaders ())
> +	goto retry;
>  
>        auto not_stopped = [&] (thread_info *thread)
>  	{
> @@ -2893,6 +2907,17 @@ linux_process_target::filter_exit_event (lwp_info *event_child,
>    struct thread_info *thread = get_lwp_thread (event_child);
>    ptid_t ptid = ptid_of (thread);
>  
> +  if (ourstatus->kind () == TARGET_WAITKIND_THREAD_EXITED)
> +    {
> +      /* We're reporting a thread exit for the leader.  The exit was
> +	 detected by check_zombie_leaders.  */
> +      gdb_assert (is_leader (thread));
> +      gdb_assert (report_exit_events_for (thread));
> +
> +      delete_lwp (event_child);
> +      return ptid;
> +    }
> +
>    /* Note we must filter TARGET_WAITKIND_SIGNALLED as well, otherwise
>       if a non-leader thread exits with a signal, we'd report it to the
>       core which would interpret it as the whole-process exiting.
> @@ -3012,7 +3037,20 @@ linux_process_target::wait_1 (ptid_t ptid, target_waitstatus *ourstatus,
>      {
>        if (WIFEXITED (w))
>  	{
> -	  ourstatus->set_exited (WEXITSTATUS (w));
> +	  /* If we already have the exit recorded in waitstatus, use
> +	     it.  This will happen when we detect a zombie leader,
> +	     when we had GDB_THREAD_OPTION_EXIT enabled for it.  We
> +	     want to report its exit as TARGET_WAITKIND_THREAD_EXITED,
> +	     as the whole process hasn't exited yet.  */
> +	  const target_waitstatus &ws = event_child->waitstatus;
> +	  if (ws.kind () != TARGET_WAITKIND_IGNORE)
> +	    {
> +	      gdb_assert (ws.kind () == TARGET_WAITKIND_EXITED
> +			  || ws.kind () == TARGET_WAITKIND_THREAD_EXITED);
> +	      *ourstatus = ws;
> +	    }
> +	  else
> +	    ourstatus->set_exited (WEXITSTATUS (w));
>  
>  	  threads_debug_printf
>  	    ("ret = %s, exited with retcode %d",
> @@ -3720,8 +3758,15 @@ suspend_and_send_sigstop (thread_info *thread, lwp_info *except)
>    send_sigstop (thread, except);
>  }
>  
> +/* Mark LWP dead, with WSTAT as exit status pending to report later.
> +   If THREAD_EVENT is true, interpret WSTAT as a thread exit event
> +   instead of a process exit event.  This is meaningful for the leader
> +   thread, as we normally report a process-wide exit event when we see
> +   the leader exit, and a thread exit event when we see any other
> +   thread exit.  */
> +
>  static void
> -mark_lwp_dead (struct lwp_info *lwp, int wstat)
> +mark_lwp_dead (struct lwp_info *lwp, int wstat, bool thread_event)
>  {
>    /* Store the exit status for later.  */
>    lwp->status_pending_p = 1;
> @@ -3730,9 +3775,19 @@ mark_lwp_dead (struct lwp_info *lwp, int wstat)
>    /* Store in waitstatus as well, as there's nothing else to process
>       for this event.  */
>    if (WIFEXITED (wstat))
> -    lwp->waitstatus.set_exited (WEXITSTATUS (wstat));
> +    {
> +      if (thread_event)
> +	lwp->waitstatus.set_thread_exited (WEXITSTATUS (wstat));
> +      else
> +	lwp->waitstatus.set_exited (WEXITSTATUS (wstat));
> +    }
>    else if (WIFSIGNALED (wstat))
> -    lwp->waitstatus.set_signalled (gdb_signal_from_host (WTERMSIG (wstat)));
> +    {
> +      gdb_assert (!thread_event);
> +      lwp->waitstatus.set_signalled (gdb_signal_from_host (WTERMSIG (wstat)));
> +    }
> +  else
> +    gdb_assert_not_reached ("unknown status kind");
>  
>    /* Prevent trying to stop it.  */
>    lwp->stopped = 1;
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 33a14e15173..ffbc3c6f095 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -574,8 +574,9 @@ class linux_process_target : public process_stratum_target
>  
>    /* Detect zombie thread group leaders, and "exit" them.  We can't
>       reap their exits until all other threads in the group have
> -     exited.  */
> -  void check_zombie_leaders ();
> +     exited.  Returns true if we left any new event pending, false
> +     otherwise.  */
> +  bool check_zombie_leaders ();
>  
>    /* Convenience function that is called when we're about to return an
>       event to the core.  If the event is an exit or signalled event,
> -- 
> 2.36.0