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

[Pedro Alves <palves@redhat.com>]

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