Re: [PATCH] MIPS Linux signals

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

Hi Michael,

> This patch adds a target function to translate MIPS Linux signals
> to GDB internal signal numbers.
> 
> 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

 Thanks, please make a unified diff (`diff -up') next time.  Further notes 
below.

Index: gdb/mips-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-tdep.c,v
retrieving revision 1.94
diff -r1.94 mips-linux-tdep.c
42a43
> #include "gdb_signals.h"
1332a1334,1428
> /* Translate signals based on MIPS signal values.  
>    Adapted from gdb/common/signals.c.  */
> 
> static enum gdb_signal
> mips_gdb_signal_from_target (struct gdbarch *gdbarch, int signo)
> {
>   switch (signo) 
>     {
>     case 0:
>       return GDB_SIGNAL_0;
>     case MIPS_SIGHUP:
>       return GDB_SIGNAL_HUP;
>     case MIPS_SIGINT:
>       return GDB_SIGNAL_INT;
>     case MIPS_SIGQUIT:
>       return GDB_SIGNAL_QUIT;
>     case MIPS_SIGILL:
>       return GDB_SIGNAL_ILL;
>     case MIPS_SIGTRAP:
>       return GDB_SIGNAL_TRAP;
>     case MIPS_SIGABRT:
>       return GDB_SIGNAL_ABRT;
>     case MIPS_SIGEMT:
>       return GDB_SIGNAL_EMT;
>     case MIPS_SIGFPE:
>       return GDB_SIGNAL_FPE;
>     case MIPS_SIGKILL:
>       return GDB_SIGNAL_KILL;
>     case MIPS_SIGBUS:
>       return GDB_SIGNAL_BUS;
>     case MIPS_SIGSEGV:
>       return GDB_SIGNAL_SEGV;
>     case MIPS_SIGSYS:
>       return GDB_SIGNAL_SYS;
>     case MIPS_SIGPIPE:
>       return GDB_SIGNAL_PIPE;
>     case MIPS_SIGALRM:
>       return GDB_SIGNAL_ALRM;
>     case MIPS_SIGTERM:
>       return GDB_SIGNAL_TERM;
>     case MIPS_SIGUSR1:
>       return GDB_SIGNAL_USR1;
>     case MIPS_SIGUSR2:
>       return GDB_SIGNAL_USR2;
>     case MIPS_SIGCHLD:
>       return GDB_SIGNAL_CHLD;
>     case MIPS_SIGPWR:
>       return GDB_SIGNAL_PWR;
>     case MIPS_SIGWINCH:
>       return GDB_SIGNAL_WINCH;
>     case MIPS_SIGURG:
>       return GDB_SIGNAL_URG;
>     case MIPS_SIGPOLL:
>       return GDB_SIGNAL_POLL;
>     case MIPS_SIGSTOP:
>       return GDB_SIGNAL_STOP;
>     case MIPS_SIGTSTP:
>       return GDB_SIGNAL_TSTP;
>     case MIPS_SIGCONT:
>       return GDB_SIGNAL_CONT;
>     case MIPS_SIGTTIN:
>       return GDB_SIGNAL_TTIN;
>     case MIPS_SIGTTOU:
>       return GDB_SIGNAL_TTOU;
>     case MIPS_SIGVTALRM:
>       return GDB_SIGNAL_VTALRM;
>     case MIPS_SIGPROF:
>       return GDB_SIGNAL_PROF;
>     case MIPS_SIGXCPU:
>       return GDB_SIGNAL_XCPU;
>     case MIPS_SIGXFSZ:
>       return GDB_SIGNAL_XFSZ;
>   }
> 
> #if defined (REALTIME_LO)

 Is this #ifdef needed?  How about you rewrite this block using 
MIPS-specific definitions (MIPS_SIGRTMIN, etc.)?  Please avoid hardcoded 
magic numbers too -- e.g. what are the magic values 32 and 33 below and 
how do they correlate to MIPS_SIGRTMIN, etc.?  I can't get it from the 
change below.  Should they be made relative to MIPS_SIGRTMIN perhaps?  
Are the values GDB_SIGNAL_REALTIME_* expand to not in order?

 Additionally, please watch your code formatting:

>   if (signo >= REALTIME_LO && signo < REALTIME_HI)
>     {
>       /* This block of GDB_SIGNAL_REALTIME value is in order.  */
>       if (33 <= signo && signo <= 63)
> 	return (enum gdb_signal)
> 	  (signo - 33 + (int) GDB_SIGNAL_REALTIME_33);

This needs extra brackets and indentation:

	return ((enum gdb_signal)
		(signo - 33 + (int) GDB_SIGNAL_REALTIME_33));

>       else if (signo == 32)
> 	return GDB_SIGNAL_REALTIME_32;
>       else if (64 <= signo && signo <= 127)
> 	return (enum gdb_signal)
> 	  (signo - 64 + (int) GDB_SIGNAL_REALTIME_64);

Likewise.

>       else
> 	error ("GDB bug: unrecognized real-time signal");
>     }
> #endif
> 
> 
>   return GDB_SIGNAL_UNKNOWN;
> }
> 
1416a1513,1515
>   set_gdbarch_gdb_signal_from_target (gdbarch,
> 				      mips_gdb_signal_from_target);
> 
Index: gdb/mips-linux-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-tdep.h,v
retrieving revision 1.13
diff -r1.13 mips-linux-tdep.h
107a108,152
> 
> /* MIPS Signals -- adapted from linux/arch/mips/include/asm/signal.h.  */
> 
> enum mips_signals 
>   {
>     MIPS_SIGHUP    =  1,	/* Hangup (POSIX).  */
>     MIPS_SIGINT    =  2,	/* Interrupt (ANSI).  */
>     MIPS_SIGQUIT   =  3,	/* Quit (POSIX).  */
>     MIPS_SIGILL    =  4,	/* Illegal instruction (ANSI).  */
>     MIPS_SIGTRAP   =  5,	/* Trace trap (POSIX).  */
>     MIPS_SIGIOT    =  6,	/* IOT trap (4.2 BSD).  */
>     MIPS_SIGABRT   =  MIPS_SIGIOT,	/* Abort (ANSI).  */
>     MIPS_SIGEMT    =  7,
>     MIPS_SIGFPE    =  8,	/* Floating-point exception (ANSI).  */
>     MIPS_SIGKILL   =  9,	/* Kill, unblockable (POSIX).  */
>     MIPS_SIGBUS    = 10,	/* BUS error (4.2 BSD).  */
>     MIPS_SIGSEGV   = 11,	/* Segmentation violation (ANSI).  */
>     MIPS_SIGSYS    = 12,
>     MIPS_SIGPIPE   = 13,	/* Broken pipe (POSIX).  */
>     MIPS_SIGALRM   = 14,	/* Alarm clock (POSIX).  */
>     MIPS_SIGTERM   = 15,	/* Termination (ANSI).  */
>     MIPS_SIGUSR1   = 16,	/* User-defined signal 1 (POSIX).  */
>     MIPS_SIGUSR2   = 17,	/* User-defined signal 2 (POSIX).  */
>     MIPS_SIGCHLD   = 18,	/* Child status has changed (POSIX).  */
>     MIPS_SIGCLD    = MIPS_SIGCHLD,	/* Same as SIGCHLD (System V).  */
>     MIPS_SIGPWR    = 19,	/* Power failure restart (System V).  */
>     MIPS_SIGWINCH  = 20,	/* Window size change (4.3 BSD, Sun).  */
>     MIPS_SIGURG    = 21,	/* Urgent condition on socket (4.2 BSD).  */
>     MIPS_SIGIO     = 22,	/* I/O now possible (4.2 BSD).  */
>     MIPS_SIGPOLL   = MIPS_SIGIO,/* Pollable event occurred (System V).  */

Please add a space after the comma here, I suggest replacing the tabs 
after MIPS_SIGIOT and MIPS_SIGCHLD above for consistency too.

>     MIPS_SIGSTOP   = 23,	/* Stop, unblockable (POSIX).  */
>     MIPS_SIGTSTP   = 24,	/* Keyboard stop (POSIX).  */
>     MIPS_SIGCONT   = 25,	/* Continue (POSIX).  */
>     MIPS_SIGTTIN   = 26,	/* Background read from tty (POSIX).  */
>     MIPS_SIGTTOU   = 27,	/* Background write to tty (POSIX).  */
>     MIPS_SIGVTALRM = 28,	/* Virtual alarm clock (4.2 BSD).  */
>     MIPS_SIGPROF   = 29,	/* Profiling alarm clock (4.2 BSD).  */
>     MIPS_SIGXCPU   = 30,	/* CPU limit exceeded (4.2 BSD).  */
>     MIPS_SIGXFSZ   = 31,	/* File size limit exceeded (4.2 BSD).  */
>     MIPS_SIGRTMIN  = 32,	/* Minimum RT signal. */
>     MIPS_SIGRTMAX  = 128-1	/* Maximum RT signal. */

Missing spaces after full stops and around "-" above.

>   };
> 
> #define REALTIME_LO MIPS_SIGRTMIN
> #define REALTIME_HI MIPS_SIGRTMAX

 Please resend with these clean-ups, as a unified diff, and I'll see if I 
need to infer any other changes from the context then.  Thanks.

  Maciej