Re: [PATCH] MIPS Linux signals

[Michael Eager <eager@eagerm.com>]

On 06/01/2012 01:53 PM, Maciej W. Rozycki wrote:
> 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?

This code is copied from signals.c.  I don't really want to re-design
the logic since I don't have a straight-forward way to test the code.

I'll recast this in terms of MIPS_SIGRTMIN, but all of your questions
can be addressed to the code in signals.c.  I didn't find it clear, either.

>
>   Additionally, please watch your code formatting:

I'll resend with formatting changes.

>
>>    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
>


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077