[PATCH] gdbserver/linux-low: use std::list to store pending signals

[PATCH] gdbserver/linux-low: use std::list to store pending signals

[Tankut Baris Aktemur]

Use std::list to store pending signals instead of a manually-managed
linked list.  This is a refactoring.

In the existing code, pending signals are kept in a manually-created
linked list with "prev" pointers.  A new pending signal is thus
inserted to the beginning of the list.  When consuming, GDB goes until
the end of the list, following the "prev" pointers, and processes the
final item.  With this patch, a new item is added to the end of the
list and the item at the front of the list is consumed.  In other
words, the list elements used to be stored in reverse order; with this
patch, they are stored in their order of arrival.  This causes a change
in the debug messages that print the pending signals.  Otherwise, no
behavioral change is expected.

gdbserver/ChangeLog:
2020-06-19  Tankut Baris Aktemur  <tankut.baris.aktemur@intel.com>

	Use std::list to stop pending signal instead of manually-created
	linked list.
	* linux-low.h: Include <list>.
	(struct pending_signal): Move here from linux-low.cc.
	(struct lwp_info) <pending_signals>
	<pending_signals_to_report>: Update the type.
	* linux-low.cc (struct pending_signals): Remove.
	(linux_process_target::delete_lwp)
	(linux_process_target::add_lwp)
	(enqueue_one_deferred_signal)
	(dequeue_one_deferred_signal)
	(enqueue_pending_signal)
	(linux_process_target::resume_one_lwp_throw)
	(linux_process_target::thread_needs_step_over)
	(linux_process_target::resume_one_thread)
	(linux_process_target::proceed_one_lwp): Update the use of pending
	signal list.
---
 gdbserver/linux-low.cc | 101 +++++++++++++----------------------------
 gdbserver/linux-low.h  |  27 ++++++++---
 2 files changed, 51 insertions(+), 77 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index bde6c767e87..7fbf9122063 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -315,13 +315,6 @@ lwp_in_step_range (struct lwp_info *lwp)
   return (pc >= lwp->step_range_start && pc < lwp->step_range_end);
 }
 
-struct pending_signals
-{
-  int signal;
-  siginfo_t info;
-  struct pending_signals *prev;
-};
-
 /* The read/write ends of the pipe registered as waitable file in the
    event loop.  */
 static int linux_event_pipe[2] = { -1, -1 };
@@ -397,7 +390,7 @@ linux_process_target::delete_lwp (lwp_info *lwp)
 
   low_delete_thread (lwp->arch_private);
 
-  free (lwp);
+  delete lwp;
 }
 
 void
@@ -914,7 +907,7 @@ linux_process_target::add_lwp (ptid_t ptid)
 {
   struct lwp_info *lwp;
 
-  lwp = XCNEW (struct lwp_info);
+  lwp = new lwp_info {};
 
   lwp->waitstatus.kind = TARGET_WAITKIND_IGNORE;
 
@@ -2119,7 +2112,6 @@ linux_process_target::maybe_move_out_of_jump_pad (lwp_info *lwp, int *wstat)
 static void
 enqueue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
 {
-  struct pending_signals *p_sig;
   struct thread_info *thread = get_lwp_thread (lwp);
 
   if (debug_threads)
@@ -2128,13 +2120,9 @@ enqueue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
 
   if (debug_threads)
     {
-      struct pending_signals *sig;
-
-      for (sig = lwp->pending_signals_to_report;
-	   sig != NULL;
-	   sig = sig->prev)
+      for (auto &sig : lwp->pending_signals_to_report)
 	debug_printf ("   Already queued %d\n",
-		      sig->signal);
+		      sig.signal);
 
       debug_printf ("   (no more currently queued signals)\n");
     }
@@ -2144,32 +2132,24 @@ enqueue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
      twice)  */
   if (WSTOPSIG (*wstat) < __SIGRTMIN)
     {
-      struct pending_signals *sig;
-
-      for (sig = lwp->pending_signals_to_report;
-	   sig != NULL;
-	   sig = sig->prev)
+      for (auto &sig : lwp->pending_signals_to_report)
 	{
-	  if (sig->signal == WSTOPSIG (*wstat))
+	  if (sig.signal == WSTOPSIG (*wstat))
 	    {
 	      if (debug_threads)
 		debug_printf ("Not requeuing already queued non-RT signal %d"
 			      " for LWP %ld\n",
-			      sig->signal,
+			      sig.signal,
 			      lwpid_of (thread));
 	      return;
 	    }
 	}
     }
 
-  p_sig = XCNEW (struct pending_signals);
-  p_sig->prev = lwp->pending_signals_to_report;
-  p_sig->signal = WSTOPSIG (*wstat);
+  lwp->pending_signals_to_report.emplace_back (WSTOPSIG (*wstat));
 
   ptrace (PTRACE_GETSIGINFO, lwpid_of (thread), (PTRACE_TYPE_ARG3) 0,
-	  &p_sig->info);
-
-  lwp->pending_signals_to_report = p_sig;
+	  &(lwp->pending_signals_to_report.back ().info));
 }
 
 /* Dequeue one signal from the "signals to report later when out of
@@ -2180,20 +2160,16 @@ dequeue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
 {
   struct thread_info *thread = get_lwp_thread (lwp);
 
-  if (lwp->pending_signals_to_report != NULL)
+  if (!lwp->pending_signals_to_report.empty ())
     {
-      struct pending_signals **p_sig;
+      const pending_signal &p_sig = lwp->pending_signals_to_report.front ();
 
-      p_sig = &lwp->pending_signals_to_report;
-      while ((*p_sig)->prev != NULL)
-	p_sig = &(*p_sig)->prev;
-
-      *wstat = W_STOPCODE ((*p_sig)->signal);
-      if ((*p_sig)->info.si_signo != 0)
+      *wstat = W_STOPCODE (p_sig.signal);
+      if (p_sig.info.si_signo != 0)
 	ptrace (PTRACE_SETSIGINFO, lwpid_of (thread), (PTRACE_TYPE_ARG3) 0,
-		&(*p_sig)->info);
-      free (*p_sig);
-      *p_sig = NULL;
+		&(p_sig.info));
+
+      lwp->pending_signals_to_report.pop_front ();
 
       if (debug_threads)
 	debug_printf ("Reporting deferred signal %d for LWP %ld.\n",
@@ -2201,13 +2177,9 @@ dequeue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
 
       if (debug_threads)
 	{
-	  struct pending_signals *sig;
-
-	  for (sig = lwp->pending_signals_to_report;
-	       sig != NULL;
-	       sig = sig->prev)
+	  for (auto &sig : lwp->pending_signals_to_report)
 	    debug_printf ("   Still queued %d\n",
-			  sig->signal);
+			  sig.signal);
 
 	  debug_printf ("   (no more queued signals)\n");
 	}
@@ -4050,15 +4022,9 @@ linux_process_target::stop_all_lwps (int suspend, lwp_info *except)
 static void
 enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info)
 {
-  struct pending_signals *p_sig = XNEW (struct pending_signals);
-
-  p_sig->prev = lwp->pending_signals;
-  p_sig->signal = signal;
-  if (info == NULL)
-    memset (&p_sig->info, 0, sizeof (siginfo_t));
-  else
-    memcpy (&p_sig->info, info, sizeof (siginfo_t));
-  lwp->pending_signals = p_sig;
+  lwp->pending_signals.emplace_back (signal);
+  if (info != nullptr)
+    lwp->pending_signals.back ().info = *info;
 }
 
 void
@@ -4154,7 +4120,7 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
      inferior right now.  */
   if (signal != 0
       && (lwp->status_pending_p
-	  || lwp->pending_signals != NULL
+	  || !lwp->pending_signals.empty ()
 	  || !lwp_signal_can_be_delivered (lwp)))
     {
       enqueue_pending_signal (lwp, signal, info);
@@ -4263,21 +4229,16 @@ linux_process_target::resume_one_lwp_throw (lwp_info *lwp, int step,
 
   /* If we have pending signals, consume one if it can be delivered to
      the inferior.  */
-  if (lwp->pending_signals != NULL && lwp_signal_can_be_delivered (lwp))
+  if (!lwp->pending_signals.empty () && lwp_signal_can_be_delivered (lwp))
     {
-      struct pending_signals **p_sig;
-
-      p_sig = &lwp->pending_signals;
-      while ((*p_sig)->prev != NULL)
-	p_sig = &(*p_sig)->prev;
+      const pending_signal &p_sig = lwp->pending_signals.front ();
 
-      signal = (*p_sig)->signal;
-      if ((*p_sig)->info.si_signo != 0)
+      signal = p_sig.signal;
+      if (p_sig.info.si_signo != 0)
 	ptrace (PTRACE_SETSIGINFO, lwpid_of (thread), (PTRACE_TYPE_ARG3) 0,
-		&(*p_sig)->info);
+		&p_sig.info);
 
-      free (*p_sig);
-      *p_sig = NULL;
+      lwp->pending_signals.pop_front ();
     }
 
   if (debug_threads)
@@ -4570,7 +4531,7 @@ linux_process_target::thread_needs_step_over (thread_info *thread)
   /* On software single step target, resume the inferior with signal
      rather than stepping over.  */
   if (supports_software_single_step ()
-      && lwp->pending_signals != NULL
+      && !lwp->pending_signals.empty ()
       && lwp_signal_can_be_delivered (lwp))
     {
       if (debug_threads)
@@ -4787,7 +4748,7 @@ linux_process_target::resume_one_thread (thread_info *thread,
 	     midway through moving the LWP out of the jumppad, and we
 	     will report the pending signal as soon as that is
 	     finished.  */
-	  if (lwp->pending_signals_to_report == NULL)
+	  if (lwp->pending_signals_to_report.empty ())
 	    send_sigstop (lwp);
 	}
 
@@ -4966,7 +4927,7 @@ linux_process_target::proceed_one_lwp (thread_info *thread, lwp_info *except)
     }
 
   if (thread->last_resume_kind == resume_stop
-      && lwp->pending_signals_to_report == NULL
+      && lwp->pending_signals_to_report.empty ()
       && (lwp->collecting_fast_tracepoint
 	  == fast_tpoint_collect_result::not_collecting))
     {
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 5fed2ee2ca2..1cd2c5f16e9 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -31,6 +31,8 @@
 #include "target/waitstatus.h" /* For enum target_stop_reason.  */
 #include "tracepoint.h"
 
+#include <list>
+
 #define PTRACE_XFER_TYPE long
 
 #ifdef HAVE_LINUX_REGSETS
@@ -695,6 +697,18 @@ extern linux_process_target *the_linux_target;
 #define get_thread_lwp(thr) ((struct lwp_info *) (thread_target_data (thr)))
 #define get_lwp_thread(lwp) ((lwp)->thread)
 
+/* Information about a signal that is to be delivered to a thread.  */
+
+struct pending_signal
+{
+  pending_signal (int signal)
+    : signal {signal}, info {}
+  {};
+
+  int signal;
+  siginfo_t info;
+};
+
 /* This struct is recorded in the target_data field of struct thread_info.
 
    On linux ``all_threads'' is keyed by the LWP ID, which we use as the
@@ -786,9 +800,8 @@ struct lwp_info
      next time we see this LWP stop.  */
   int must_set_ptrace_flags;
 
-  /* If this is non-zero, it points to a chain of signals which need to
-     be delivered to this process.  */
-  struct pending_signals *pending_signals;
+  /* A chain of signals that need to be delivered to this process.  */
+  std::list<pending_signal> pending_signals;
 
   /* A link used when resuming.  It is initialized from the resume request,
      and then processed and cleared in linux_resume_one_lwp.  */
@@ -800,10 +813,10 @@ struct lwp_info
      if a signal arrives to this lwp while it is collecting.  */
   fast_tpoint_collect_result collecting_fast_tracepoint;
 
-  /* If this is non-zero, it points to a chain of signals which need
-     to be reported to GDB.  These were deferred because the thread
-     was doing a fast tracepoint collect when they arrived.  */
-  struct pending_signals *pending_signals_to_report;
+  /* A chain of signals that need to be reported to GDB.  These were
+     deferred because the thread was doing a fast tracepoint collect
+     when they arrived.  */
+  std::list<pending_signal> pending_signals_to_report;
 
   /* When collecting_fast_tracepoint is first found to be 1, we insert
      a exit-jump-pad-quickly breakpoint.  This is it.  */
-- 
2.17.1

Re: [PATCH] gdbserver/linux-low: use std::list to store pending signals

[Pedro Alves]

LGTM with a few nits below.

On 6/19/20 1:12 PM, Tankut Baris Aktemur via Gdb-patches wrote:

> @@ -2144,32 +2132,24 @@ enqueue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
>       twice)  */
>    if (WSTOPSIG (*wstat) < __SIGRTMIN)
>      {
> -      struct pending_signals *sig;
> -
> -      for (sig = lwp->pending_signals_to_report;
> -	   sig != NULL;
> -	   sig = sig->prev)
> +      for (auto &sig : lwp->pending_signals_to_report)

const auto ?

> -  p_sig = XCNEW (struct pending_signals);
> -  p_sig->prev = lwp->pending_signals_to_report;
> -  p_sig->signal = WSTOPSIG (*wstat);
> +  lwp->pending_signals_to_report.emplace_back (WSTOPSIG (*wstat));
>  
>    ptrace (PTRACE_GETSIGINFO, lwpid_of (thread), (PTRACE_TYPE_ARG3) 0,
> -	  &p_sig->info);
> -
> -  lwp->pending_signals_to_report = p_sig;
> +	  &(lwp->pending_signals_to_report.back ().info));

Redundant parens.

>  }
>  
>  /* Dequeue one signal from the "signals to report later when out of
> @@ -2180,20 +2160,16 @@ dequeue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
>  {
>    struct thread_info *thread = get_lwp_thread (lwp);
>  
> -  if (lwp->pending_signals_to_report != NULL)
> +  if (!lwp->pending_signals_to_report.empty ())
>      {
> -      struct pending_signals **p_sig;
> +      const pending_signal &p_sig = lwp->pending_signals_to_report.front ();
>  
> -      p_sig = &lwp->pending_signals_to_report;
> -      while ((*p_sig)->prev != NULL)
> -	p_sig = &(*p_sig)->prev;
> -
> -      *wstat = W_STOPCODE ((*p_sig)->signal);
> -      if ((*p_sig)->info.si_signo != 0)
> +      *wstat = W_STOPCODE (p_sig.signal);
> +      if (p_sig.info.si_signo != 0)
>  	ptrace (PTRACE_SETSIGINFO, lwpid_of (thread), (PTRACE_TYPE_ARG3) 0,
> -		&(*p_sig)->info);
> -      free (*p_sig);
> -      *p_sig = NULL;
> +		&(p_sig.info));

Redundant parens.

> +
> +      lwp->pending_signals_to_report.pop_front ();
>  
>        if (debug_threads)
>  	debug_printf ("Reporting deferred signal %d for LWP %ld.\n",
> @@ -2201,13 +2177,9 @@ dequeue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
>  
>        if (debug_threads)
>  	{
> -	  struct pending_signals *sig;
> -
> -	  for (sig = lwp->pending_signals_to_report;
> -	       sig != NULL;
> -	       sig = sig->prev)
> +	  for (auto &sig : lwp->pending_signals_to_report)

const ?

>  	    debug_printf ("   Still queued %d\n",
> -			  sig->signal);
> +			  sig.signal);
>  
>  	  debug_printf ("   (no more queued signals)\n");
>  	}
> @@ -4050,15 +4022,9 @@ linux_process_target::stop_all_lwps (int suspend, lwp_info *except)
>  static void
>  enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info)
>  {
> -  struct pending_signals *p_sig = XNEW (struct pending_signals);
> -
> -  p_sig->prev = lwp->pending_signals;
> -  p_sig->signal = signal;
> -  if (info == NULL)
> -    memset (&p_sig->info, 0, sizeof (siginfo_t));
> -  else
> -    memcpy (&p_sig->info, info, sizeof (siginfo_t));
> -  lwp->pending_signals = p_sig;
> +  lwp->pending_signals.emplace_back (signal);
> +  if (info != nullptr)
> +    lwp->pending_signals.back ().info = *info;

I noticed that the pending_signal ctor always initializes the
info field.  Seems wasteful when we're practically always going to
overwrite it.   How about not initializing it in the ctor,
and then here do the memset if info == NULL, as before?

Thanks,
Pedro Alves

RE: [PATCH] gdbserver/linux-low: use std::list to store pending signals

[Aktemur, Tankut Baris]

On Saturday, June 20, 2020 9:26 PM, Pedro Alves wrote:
> LGTM with a few nits below.
> 
> On 6/19/20 1:12 PM, Tankut Baris Aktemur via Gdb-patches wrote:
> 
> > @@ -2144,32 +2132,24 @@ enqueue_one_deferred_signal (struct lwp_info *lwp, int *wstat)
> >       twice)  */
> >    if (WSTOPSIG (*wstat) < __SIGRTMIN)
> >      {
> > -      struct pending_signals *sig;
> > -
> > -      for (sig = lwp->pending_signals_to_report;
> > -	   sig != NULL;
> > -	   sig = sig->prev)
> > +      for (auto &sig : lwp->pending_signals_to_report)
> 
> const auto ?

Made this a const.  There is another loop, right above, to print debug info.
Changed it, too.

> > -  lwp->pending_signals_to_report = p_sig;
> > +	  &(lwp->pending_signals_to_report.back ().info));
> 
> Redundant parens.

Fixed.

> > +		&(p_sig.info));
> 
> Redundant parens.

Fixed.

> > +	  for (auto &sig : lwp->pending_signals_to_report)
> 
> const ?

Made this one const, too.

> 
> >  	    debug_printf ("   Still queued %d\n",
> > -			  sig->signal);
> > +			  sig.signal);
> >
> >  	  debug_printf ("   (no more queued signals)\n");
> >  	}
> > @@ -4050,15 +4022,9 @@ linux_process_target::stop_all_lwps (int suspend, lwp_info *except)
> >  static void
> >  enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info)
> >  {
> > -  struct pending_signals *p_sig = XNEW (struct pending_signals);
> > -
> > -  p_sig->prev = lwp->pending_signals;
> > -  p_sig->signal = signal;
> > -  if (info == NULL)
> > -    memset (&p_sig->info, 0, sizeof (siginfo_t));
> > -  else
> > -    memcpy (&p_sig->info, info, sizeof (siginfo_t));
> > -  lwp->pending_signals = p_sig;
> > +  lwp->pending_signals.emplace_back (signal);
> > +  if (info != nullptr)
> > +    lwp->pending_signals.back ().info = *info;
> 
> I noticed that the pending_signal ctor always initializes the
> info field.  Seems wasteful when we're practically always going to
> overwrite it.   How about not initializing it in the ctor,
> and then here do the memset if info == NULL, as before?
> 
> Thanks,
> Pedro Alves

The pending_signal struct in linux-low.h became

  /* Information about a signal that is to be delivered to a thread.  */

  struct pending_signal
  {
    pending_signal (int signal)
      : signal {signal}
    {};

    int signal;
    siginfo_t info;
  };

and the enqueue_pending_signal function became:

  static void
  enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info)
  {
    lwp->pending_signals.emplace_back (signal);
    if (info == nullptr)
      memset (&lwp->pending_signals.back ().info, 0, sizeof (siginfo_t));
    else
      lwp->pending_signals.back ().info = *info;
  }

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928