Re: [PATCH] MIPS Linux signals

[Michael Eager <eager@eagerm.com>]

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

I forgot to edit the ChangeLog out of the diff.

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

OK.

>
> -- the casts are probably redundant, but let them stay.
>
>   OK with this change, the rest is fine with me.  Thanks for tackling this
> problem.

Thanks.


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