Re: [PATCH] MIPS Linux signals

["Maciej W. Rozycki" <macro@codesourcery.com>]

On Fri, 1 Jun 2012, Michael Eager wrote:

> Some of the awkward code in translating REALTIME signals is due to
> the fact that the GDB_SIGNAL_REALTIME block is not contiguous.

 OK, fair enough, it's always good to include any such explanations with 
patch submissions so as to make the review process easier for the 
reviewers.  It makes sense to note the peculiarity around code concerned 
too.

> I recast the section of code copied from signals.c in terms of
> MIPS_SIGRTMIN & MIPS_SIGRTMAX, but it isn't clear to me that this
> is an improvement, since it is now different from the similar code
> in signals.c.

 This is target-specific code, its goal is not to be as close as to 
generic code as possible, but (as with any code, except where critical 
performance or any other requirements mandate otherwise) straightforward 
and matching the requirements of the specific target concerned.

> Index: gdb/ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/gdb/ChangeLog,v
> retrieving revision 1.14309
> diff -u -p -r1.14309 ChangeLog
> --- gdb/ChangeLog	1 Jun 2012 16:37:56 -0000	1.14309
> +++ gdb/ChangeLog	1 Jun 2012 22:43:51 -0000
> @@ -1,3 +1,8 @@
> +2012-06-01  Michael Eager  <eager@eagercon.com>
> +
> +	* mips-linux-tdep.c (mips_gdb_signal_from_target): New
> +	* mips-linux-tdep.h (mips_signals): New
> +
>  2012-06-01  Siddhesh Poyarekar  <siddhesh@redhat.com>
>  
>  	* target.c (target_read_memory): Make LEN argument as size_t.

 Please supply ChangeLog entries separately, they're not going to work 
when included as a diff and will require hand-editing anyway.

> Index: gdb/mips-linux-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mips-linux-tdep.c,v
> retrieving revision 1.94
> diff -u -p -r1.94 mips-linux-tdep.c
> --- gdb/mips-linux-tdep.c	22 May 2012 17:12:07 -0000	1.94
> +++ gdb/mips-linux-tdep.c	1 Jun 2012 22:43:51 -0000
> @@ -1330,6 +1331,98 @@ mips_linux_get_syscall_number (struct gd
[...]
> +
> +  if (signo >= MIPS_SIGRTMIN && signo <= MIPS_SIGRTMAX)
> +    {
> +      /* This block of GDB_SIGNAL_REALTIME value is in order.  */
> +      if (MIPS_SIGRTMIN <= signo && signo <= (MIPS_SIGRTMIN + 32))
> +	return ((enum gdb_signal)
> +		(signo - MIPS_SIGRTMIN + 1 + (int) GDB_SIGNAL_REALTIME_33));
> +      else if (signo == MIPS_SIGRTMIN)
> +	return GDB_SIGNAL_REALTIME_32;
> +      else if ((MIPS_SIGRTMIN + 32) <= signo && signo <= MIPS_SIGRTMAX)
> +	return ((enum gdb_signal)
> +		(signo - (MIPS_SIGRTMIN + 32) + (int) GDB_SIGNAL_REALTIME_64));
> +      else
> +	error ("GDB bug: unrecognized real-time signal");
> +    }
> +
> +  return GDB_SIGNAL_UNKNOWN;
> +}
> +
>  /* Initialize one of the GNU/Linux OS ABIs.  */
>  
>  static void

 I find this unreadable (and buggy too, which is likely a consequence), 
per observations above please rewrite this as below:

  if (signo >= MIPS_SIGRTMIN && signo <= MIPS_SIGRTMAX)
    {
      /* GDB_SIGNAL_REALTIME values are not contiguous, map parts of
         the MIPS block to the respective GDB_SIGNAL_REALTIME blocks.  */
      signo -= MIPS_SIGRTMIN;
      if (signo == 0)
	return GDB_SIGNAL_REALTIME_32;
      else if (signo < 32)
	return ((enum gdb_signal) (signo - 1 + (int) GDB_SIGNAL_REALTIME_33));
      else
	return ((enum gdb_signal) (signo - 32 + (int) GDB_SIGNAL_REALTIME_64));
    }

  return GDB_SIGNAL_UNKNOWN;
}

-- the casts are probably redundant, but let them stay.

 OK with this change, the rest is fine with me.  Thanks for tackling this 
problem.

  Maciej