From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 13009 invoked by alias); 1 Jun 2012 21:07:21 -0000 Received: (qmail 12995 invoked by uid 22791); 1 Jun 2012 21:07:20 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED X-Spam-Check-By: sourceware.org Received: from shell4.bayarea.net (HELO shell4.bayarea.net) (209.128.82.1) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 01 Jun 2012 21:07:07 +0000 Received: (qmail 2674 invoked from network); 1 Jun 2012 14:07:06 -0700 Received: from c-76-102-3-160.hsd1.ca.comcast.net (HELO redwood.eagercon.com) (76.102.3.160) by shell4.bayarea.net with SMTP; 1 Jun 2012 14:07:06 -0700 Message-ID: <4FC92EF9.3080804@eagerm.com> Date: Fri, 01 Jun 2012 21:07:00 -0000 From: Michael Eager User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1 MIME-Version: 1.0 To: "Maciej W. Rozycki" CC: "gdb-patches@sourceware.org" , Pedro Alves Subject: Re: [PATCH] MIPS Linux signals References: <4FC9244F.1080704@eagerm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes 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/msg00046.txt.bz2 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 >> >> * 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