From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8373 invoked by alias); 1 Jun 2012 20:53:54 -0000 Received: (qmail 8354 invoked by uid 22791); 1 Jun 2012 20:53:51 -0000 X-SWARE-Spam-Status: No, hits=-4.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Jun 2012 20:53:36 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1SaYqj-0000CS-PF from Maciej_Rozycki@mentor.com ; Fri, 01 Jun 2012 13:53:33 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Fri, 1 Jun 2012 13:53:33 -0700 Received: from [172.30.0.162] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.1.289.1; Fri, 1 Jun 2012 21:53:30 +0100 Date: Fri, 01 Jun 2012 20:53:00 -0000 From: "Maciej W. Rozycki" To: Michael Eager CC: "gdb-patches@sourceware.org" , Pedro Alves Subject: Re: [PATCH] MIPS Linux signals In-Reply-To: <4FC9244F.1080704@eagerm.com> Message-ID: References: <4FC9244F.1080704@eagerm.com> User-Agent: Alpine 1.10 (DEB 962 2008-03-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-06/txt/msg00044.txt.bz2 Hi Michael, > This patch adds a target function to translate MIPS Linux signals > to GDB internal signal numbers. > > 2012-06-01 Michael Eager > > * 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