Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* MIPS Linux signals
@ 2012-05-20  2:03 Michael Eager
  2012-05-21 11:04 ` Pedro Alves
                   ` (3 more replies)
  0 siblings, 4 replies; 29+ messages in thread
From: Michael Eager @ 2012-05-20  2:03 UTC (permalink / raw)
  To: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

MIPS Linux has different values assigned to signals than
X86 or PPC Linux.   The result is that a SIGBUS on MIPS
(value 0xA) is reported as SIGUSR1, since that signal has
the that value on X86.  This causes confusion (obviously).
Telling gdb to handle SIGUSR1 will cause it to respond to
SIGBUS but ignore SIGUSR1 (which has the value 0x16 on MIPS).

I didn't find a bug report for this in Bugzilla.  I
have trouble believing that this problem has gone unnoticed.

The attached patch adds target_signal->internal_signal
translation, and vice versa, for MIPS Linux targets.  I
adapted the code from target_symbol_from_host() and
do_target_symbol_to_host()in gdb/common/signals.c.  Other
than being poor names, these functions and the wrappers
around them seem unnecessarily complicated.  In particular,
I left out EXE_BAD_* translations because they appear to
be darwin-specific and translations for REALTIME_LO/HI
seem to be obsolete.  ChangeLog-2006 mentions removing
these defines.

Which leads me to some questions:
   1 -- Is there a bug report for this somewhere?
   2 -- Is there any reason to implement EXE_BAD_* translations?
   3 -- Is the REALTIME_LO/HI translation obsolete cruft?
   4 -- Do the multiple layers of wrappers around target_
        signal_{to,from}_host in signals.c serve any purpose?

I'm still testing the patch.  I have a core file which now
displays the correct signal, but I have to create a test
program where I can test the translation in the other direction.

Any comments on the patch?

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

[-- Attachment #2: mips-signal.patch --]
[-- Type: text/x-patch, Size: 7283 bytes --]

Index: gdb/gdb/mips-linux-tdep.c
===================================================================
--- gdb/.CC/cache/mips-linux-tdep.c@@/main/gdb_7_2_dev/3	2012-05-19 18:25:39.000077000 -0700
+++ gdb/mips-linux-tdep.c	2012-05-19 18:25:27.000033000 -0700
@@ -1130,6 +1130,101 @@ mips_linux_syscall_next_pc (struct frame
   return pc + 4;
 }
 
+/* Translate signals based on MIPS signal values.  
+   Adapted from gdb/common/signals.c.  */
+
+static enum target_signal
+mips_target_signal_from_host (struct gdbarch *gdbarch, int signo)
+{
+  printf ("MJE: mips_target_signal_from_host (<gdbarch>, signo=%d)\n", signo);
+
+  switch (signo) {
+    case 0:		return TARGET_SIGNAL_0;
+    case MIPS_SIGHUP: 	return TARGET_SIGNAL_HUP;
+    case MIPS_SIGINT: 	return TARGET_SIGNAL_INT;
+    case MIPS_SIGQUIT: 	return TARGET_SIGNAL_QUIT;
+    case MIPS_SIGILL: 	return TARGET_SIGNAL_ILL;
+    case MIPS_SIGTRAP: 	return TARGET_SIGNAL_TRAP;
+    case MIPS_SIGABRT: 	return TARGET_SIGNAL_ABRT;
+    case MIPS_SIGEMT: 	return TARGET_SIGNAL_EMT;
+    case MIPS_SIGFPE: 	return TARGET_SIGNAL_FPE;
+    case MIPS_SIGKILL: 	return TARGET_SIGNAL_KILL;
+    case MIPS_SIGBUS: 	return TARGET_SIGNAL_BUS;
+    case MIPS_SIGSEGV: 	return TARGET_SIGNAL_SEGV;
+    case MIPS_SIGSYS: 	return TARGET_SIGNAL_SYS;
+    case MIPS_SIGPIPE: 	return TARGET_SIGNAL_PIPE;
+    case MIPS_SIGALRM: 	return TARGET_SIGNAL_ALRM;
+    case MIPS_SIGTERM: 	return TARGET_SIGNAL_TERM;
+    case MIPS_SIGUSR1: 	return TARGET_SIGNAL_USR1;
+    case MIPS_SIGUSR2: 	return TARGET_SIGNAL_USR2;
+    case MIPS_SIGCHLD: 	return TARGET_SIGNAL_CHLD;
+    case MIPS_SIGPWR: 	return TARGET_SIGNAL_PWR;
+    case MIPS_SIGWINCH: return TARGET_SIGNAL_WINCH;
+    case MIPS_SIGURG: 	return TARGET_SIGNAL_URG;
+    case MIPS_SIGPOLL: 	return TARGET_SIGNAL_POLL;
+    case MIPS_SIGSTOP: 	return TARGET_SIGNAL_STOP;
+    case MIPS_SIGTSTP: 	return TARGET_SIGNAL_TSTP;
+    case MIPS_SIGCONT: 	return TARGET_SIGNAL_CONT;
+    case MIPS_SIGTTIN: 	return TARGET_SIGNAL_TTIN;
+    case MIPS_SIGTTOU: 	return TARGET_SIGNAL_TTOU;
+    case MIPS_SIGVTALRM: return TARGET_SIGNAL_VTALRM;
+    case MIPS_SIGPROF: 	return TARGET_SIGNAL_PROF;
+    case MIPS_SIGXCPU: 	return TARGET_SIGNAL_XCPU;
+    case MIPS_SIGXFSZ: 	return TARGET_SIGNAL_XFSZ;
+  }
+    
+#ifdef _SIG
+#endif
+
+    return TARGET_SIGNAL_UNKNOWN;
+}
+
+static int
+mips_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal signo)
+{
+  printf ("MJE: mips_target_signal_to_host (<gdbarch>, signo=%d)\n", signo);
+
+  switch (signo) {
+    case TARGET_SIGNAL_0: 	return 0;
+    case TARGET_SIGNAL_HUP:   	return MIPS_SIGHUP;
+    case TARGET_SIGNAL_INT:   	return MIPS_SIGINT;
+    case TARGET_SIGNAL_QUIT:   	return MIPS_SIGQUIT;
+    case TARGET_SIGNAL_ILL:   	return MIPS_SIGILL;
+    case TARGET_SIGNAL_TRAP:   	return MIPS_SIGTRAP;
+    case TARGET_SIGNAL_ABRT:   	return MIPS_SIGABRT;
+    case TARGET_SIGNAL_EMT:   	return MIPS_SIGEMT;
+    case TARGET_SIGNAL_FPE:   	return MIPS_SIGFPE;
+    case TARGET_SIGNAL_KILL:   	return MIPS_SIGKILL;
+    case TARGET_SIGNAL_BUS:   	return MIPS_SIGBUS;
+    case TARGET_SIGNAL_SEGV:   	return MIPS_SIGSEGV;
+    case TARGET_SIGNAL_SYS:   	return MIPS_SIGSYS;
+    case TARGET_SIGNAL_PIPE:   	return MIPS_SIGPIPE;
+    case TARGET_SIGNAL_ALRM:   	return MIPS_SIGALRM;
+    case TARGET_SIGNAL_TERM:   	return MIPS_SIGTERM;
+    case TARGET_SIGNAL_USR1:   	return MIPS_SIGUSR1;
+    case TARGET_SIGNAL_USR2:   	return MIPS_SIGUSR2;
+    case TARGET_SIGNAL_CHLD:   	return MIPS_SIGCHLD;
+    case TARGET_SIGNAL_PWR:   	return MIPS_SIGPWR;
+    case TARGET_SIGNAL_WINCH:   return MIPS_SIGWINCH;
+    case TARGET_SIGNAL_URG:   	return MIPS_SIGURG;
+    case TARGET_SIGNAL_POLL:   	return MIPS_SIGPOLL;
+    case TARGET_SIGNAL_STOP:   	return MIPS_SIGSTOP;
+    case TARGET_SIGNAL_TSTP:   	return MIPS_SIGTSTP;
+    case TARGET_SIGNAL_CONT:   	return MIPS_SIGCONT;
+    case TARGET_SIGNAL_TTIN:   	return MIPS_SIGTTIN;
+    case TARGET_SIGNAL_TTOU:   	return MIPS_SIGTTOU;
+    case TARGET_SIGNAL_VTALRM:  return MIPS_SIGVTALRM;
+    case TARGET_SIGNAL_PROF:   	return MIPS_SIGPROF;
+    case TARGET_SIGNAL_XCPU:   	return MIPS_SIGXCPU;
+    case TARGET_SIGNAL_XFSZ:   	return MIPS_SIGXFSZ;
+    
+    default:
+      warning ("Signal %s does not exist on this system.\n",
+	       target_signal_to_name (signo));
+      return 0;
+  }
+}
+
 /* Initialize one of the GNU/Linux OS ABIs.  */
 
 static void
@@ -1203,6 +1298,11 @@ mips_linux_init_abi (struct gdbarch_info
   set_gdbarch_core_read_description (gdbarch,
 				     mips_linux_core_read_description);
 
+  set_gdbarch_target_signal_from_host (gdbarch,
+                                       mips_target_signal_from_host);
+  set_gdbarch_target_signal_to_host (gdbarch,
+                                       mips_target_signal_to_host);
+
   tdep->syscall_next_pc = mips_linux_syscall_next_pc;
 
   if (tdesc_data)

Index: gdb/gdb/mips-linux-tdep.h
===================================================================
--- gdb/.CC/cache/mips-linux-tdep.h@@/main/gdb_7_2_dev/1	2012-05-19 18:25:40.000041000 -0700
+++ gdb/mips-linux-tdep.h	2012-05-19 10:53:12.000064000 -0700
@@ -100,3 +100,43 @@ enum {
 /* Return 1 if MIPS_RESTART_REGNUM is usable.  */
 
 int mips_linux_restart_reg_p (struct gdbarch *gdbarch);
+
+/* 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).  */
+  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).  */
+  };
+

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-20  2:03 MIPS Linux signals Michael Eager
@ 2012-05-21 11:04 ` Pedro Alves
  2012-05-21 14:51   ` Michael Eager
  2012-05-21 18:19 ` Maciej W. Rozycki
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-21 11:04 UTC (permalink / raw)
  To: Michael Eager; +Cc: gdb-patches

On 05/20/2012 03:02 AM, Michael Eager wrote:

> Which leads me to some questions:


>   2 -- Is there any reason to implement EXE_BAD_* translations?


No.

>   3 -- Is the REALTIME_LO/HI translation obsolete cruft?


ChangeLog-2006 points at:

http://sourceware.org/ml/gdb-patches/2006-11/msg00340.html

where we see that REALTIME_LO/HI were defined in target headers,
while they should really be native/host defines.  So it's not that they
got deprecated, but a target/host confusion bug was fixed.  The
macros are still there in common/signals.c.

>   4 -- Do the multiple layers of wrappers around target_
>        signal_{to,from}_host in signals.c serve any purpose?


Can you be more specific?  What multiple layers?

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 11:04 ` Pedro Alves
@ 2012-05-21 14:51   ` Michael Eager
  2012-05-21 17:37     ` Pedro Alves
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Eager @ 2012-05-21 14:51 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 05/21/2012 04:03 AM, Pedro Alves wrote:
> On 05/20/2012 03:02 AM, Michael Eager wrote:

>>    4 -- Do the multiple layers of wrappers around target_
>>         signal_{to,from}_host in signals.c serve any purpose?
>
> Can you be more specific?  What multiple layers?

target_signal_to_host() is a wrapper around do_target_signal_to_host().

target_signal_to_host_p() is a wrapper around do_target_signal_to_host().

This appears to be only used in gdbserver, where, on MIPS, it will
return the wrong result.  Incidentally, gdbserver on MIPS calls
the X86 target_signal_to_host() to translate signals.  This seems
confused.  If gdb is translating internal signal numbers to the
target signal numbers, then this seems to be a second translation.

default_target_signal_to_host() is a wrapper around target_signal_to_host().

default_target_signal_from_host() is a wrapper around target_signal_from_host().

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 14:51   ` Michael Eager
@ 2012-05-21 17:37     ` Pedro Alves
  2012-05-21 18:06       ` Michael Eager
  0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-21 17:37 UTC (permalink / raw)
  To: Michael Eager; +Cc: gdb-patches

On 05/21/2012 03:50 PM, Michael Eager wrote:

> On 05/21/2012 04:03 AM, Pedro Alves wrote:
>> On 05/20/2012 03:02 AM, Michael Eager wrote:
> 
>>>    4 -- Do the multiple layers of wrappers around target_
>>>         signal_{to,from}_host in signals.c serve any purpose?
>>
>> Can you be more specific?  What multiple layers?
> 
> target_signal_to_host() is a wrapper around do_target_signal_to_host().
> 
> target_signal_to_host_p() is a wrapper around do_target_signal_to_host().


Yes, do_target_signal_to_host is a helper for both target_signal_to_host and
target_signal_to_host_p.

> This appears to be only used in gdbserver, where, on MIPS, it will

> return the wrong result.  


Why will it return the wrong result on MIPS/gdbserver?  The common/signals.c
copy built into gdbserver will naturally be built with a MIPS headers, and
should pick up the correct signal defines (SIGINT, etc.).

> Incidentally, gdbserver on MIPS calls
> the X86 target_signal_to_host() to translate signals.  This seems
> confused.  


Now I'm confused.  What do you mean that MIPS gdbserver calls the
X86 target_signal_to_host ?  There's only one target_signal_to_host
function AFAICS.

> If gdb is translating internal signal numbers to the

> target signal numbers, then this seems to be a second translation.


gdbserver internally translates Linux signal numbers to the host
independent internal signal numbers.  We call the latter "target signals",
like we call every method that goes through the "target abstraction"
(target_ops vector) in gdb target_something.  The native signals that
are host dependent (host in the same sense as build vs host vs target
on autoconf), we call them host signals.

On the "wire", RSP packets should contain only the abstracted, internal,
target signal numbers.  So code on gdbserver that talks to ptrace
will need to convert the (native) host signal numbers to target signals
when pushing events to gdbserver common code, and ultimately to GDB.

AFAIK, there's no such second internal signal numbers to target
signal numbers translation in GDB.  The remote target already works with
GDB's own internal signal numbers.

> default_target_signal_to_host() is a wrapper around target_signal_to_host().
> 
> default_target_signal_from_host() is a wrapper around target_signal_from_host().


Right.  gdbarch_target_signal_from_host was invented long after target_signal_from_host
already existed to solve the similar case you ran into.  That is, of when
cross debugging core files.  In that case there's no gdbserver involved to do the
host -> gdb/internal signal conversion for us, so GDB needs to do it, according to
the core's gdbarch.  If there's no such gdbarch callback installed, then we fallback
to the host's default conversion, which with work when debugging native core files.
So you do have the right fix, but the reason why nobody has tripped on this before is
that this only affects cross core debugging, not live gdbserver debugging.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 17:37     ` Pedro Alves
@ 2012-05-21 18:06       ` Michael Eager
  0 siblings, 0 replies; 29+ messages in thread
From: Michael Eager @ 2012-05-21 18:06 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 05/21/2012 10:36 AM, Pedro Alves wrote:
> On 05/21/2012 03:50 PM, Michael Eager wrote:

>> This appears to be only used in gdbserver, where, on MIPS, it will
>> return the wrong result.
>
> Why will it return the wrong result on MIPS/gdbserver?  The common/signals.c
> copy built into gdbserver will naturally be built with a MIPS headers, and
> should pick up the correct signal defines (SIGINT, etc.).

Yes, you are right.  Gdbserver will be built with the correct MIPS signal defs.

> So you do have the right fix, but the reason why nobody has tripped on this before is
> that this only affects cross core debugging, not live gdbserver debugging.

Thanks.


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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-20  2:03 MIPS Linux signals Michael Eager
  2012-05-21 11:04 ` Pedro Alves
@ 2012-05-21 18:19 ` Maciej W. Rozycki
       [not found] ` <alpine.DEB.1.10.1205211232260.11227@tp.orcam.me.uk>
  2012-05-21 21:35 ` Pedro Alves
  3 siblings, 0 replies; 29+ messages in thread
From: Maciej W. Rozycki @ 2012-05-21 18:19 UTC (permalink / raw)
  To: Michael Eager; +Cc: gdb-patches

Hi Michael,

On Sun, 20 May 2012, Michael Eager wrote:

> MIPS Linux has different values assigned to signals than
> X86 or PPC Linux.   The result is that a SIGBUS on MIPS
> (value 0xA) is reported as SIGUSR1, since that signal has
> the that value on X86.  This causes confusion (obviously).
> Telling gdb to handle SIGUSR1 will cause it to respond to
> SIGBUS but ignore SIGUSR1 (which has the value 0x16 on MIPS).
> 
> I didn't find a bug report for this in Bugzilla.  I
> have trouble believing that this problem has gone unnoticed.

 What target are you specifically referring to?  Can you describe a 
scenario where it happens?

 As far as I know it does not apply for the native target (for obvious 
reasons) and the remote target uses generic signal numbers (as defined in 
the Remote Serial Protocol aka RSP) so they are reformed uniformly as 
well.  Did I miss anything?

> The attached patch adds target_signal->internal_signal
> translation, and vice versa, for MIPS Linux targets.  I
> adapted the code from target_symbol_from_host() and
> do_target_symbol_to_host()in gdb/common/signals.c.  Other
> than being poor names, these functions and the wrappers
> around them seem unnecessarily complicated.  In particular,
> I left out EXE_BAD_* translations because they appear to
> be darwin-specific and translations for REALTIME_LO/HI
> seem to be obsolete.  ChangeLog-2006 mentions removing
> these defines.
> 
> Which leads me to some questions:
>   1 -- Is there a bug report for this somewhere?
>   2 -- Is there any reason to implement EXE_BAD_* translations?
>   3 -- Is the REALTIME_LO/HI translation obsolete cruft?
>   4 -- Do the multiple layers of wrappers around target_
>        signal_{to,from}_host in signals.c serve any purpose?

 I'll leave answers to these questions to our general maintainers.

> I'm still testing the patch.  I have a core file which now
> displays the correct signal, but I have to create a test
> program where I can test the translation in the other direction.

 Ah, so that is with the core file target used in a cross-debugger, right?  
Well, no wonder nobody has noticed, this is a very unusual scenario -- I 
have used GDB for at least 15 years now and may have had a need to examine 
a core file with a cross-debugger perhaps once or twice -- and then some 
common signals looked for like SIGSEGV or SIGILL are identity mapped (and 
I knew the signal that caused the core file dumped beforehand anyway as 
it's printed as the program is being killed anyway).

 From experience what matters the most when examining a core file -- 
beside the dump of the process's address space of course -- are the 
contents of registers.  The rest -- I would expect hardly anybody to look 
at.

> Any comments on the patch?

 Overall it looks good to me, but has some coding style problems and some 
other small issues, listed below.

 Also by the look of it, such changes will actually be required for all 
processor/OS targets as obviously when you have e.g. a MIPS/IRIX-host 
cross-debugger targetting i386/Linux then the signal numbers won't match 
again, except that in the other direction.  That certainly begs for a 
general solution, perhaps like one used by the RSP where the GDB core uses 
generic signal numbers the target side translate to/from them as 
appropriate (obviously the host OS may have no notion of signals at all).

 That looks like a post-7.5 material to me though -- we've lived with this 
problem for long enough that I don't think we need to rush and correct it 
a little more than a week before the release branch.

> Index: gdb/gdb/mips-linux-tdep.c
> ===================================================================
> --- gdb/.CC/cache/mips-linux-tdep.c@@/main/gdb_7_2_dev/3	2012-05-19 18:25:39.000077000 -0700
> +++ gdb/mips-linux-tdep.c	2012-05-19 18:25:27.000033000 -0700
> @@ -1130,6 +1130,101 @@ mips_linux_syscall_next_pc (struct frame
>    return pc + 4;
>  }
>  
> +/* Translate signals based on MIPS signal values.  
> +   Adapted from gdb/common/signals.c.  */
> +
> +static enum target_signal
> +mips_target_signal_from_host (struct gdbarch *gdbarch, int signo)
> +{
> +  printf ("MJE: mips_target_signal_from_host (<gdbarch>, signo=%d)\n", signo);

 I'm assuming this is a debug message, please drop it.  Otherwise we use 
calls like fprintf_unfiltered (...) to produce output; see other sources 
for a reference.

> +
> +  switch (signo) {
> +    case 0:		return TARGET_SIGNAL_0;
> +    case MIPS_SIGHUP: 	return TARGET_SIGNAL_HUP;
> +    case MIPS_SIGINT: 	return TARGET_SIGNAL_INT;
> +    case MIPS_SIGQUIT: 	return TARGET_SIGNAL_QUIT;
> +    case MIPS_SIGILL: 	return TARGET_SIGNAL_ILL;
> +    case MIPS_SIGTRAP: 	return TARGET_SIGNAL_TRAP;
> +    case MIPS_SIGABRT: 	return TARGET_SIGNAL_ABRT;
> +    case MIPS_SIGEMT: 	return TARGET_SIGNAL_EMT;
> +    case MIPS_SIGFPE: 	return TARGET_SIGNAL_FPE;
> +    case MIPS_SIGKILL: 	return TARGET_SIGNAL_KILL;
> +    case MIPS_SIGBUS: 	return TARGET_SIGNAL_BUS;
> +    case MIPS_SIGSEGV: 	return TARGET_SIGNAL_SEGV;
> +    case MIPS_SIGSYS: 	return TARGET_SIGNAL_SYS;
> +    case MIPS_SIGPIPE: 	return TARGET_SIGNAL_PIPE;
> +    case MIPS_SIGALRM: 	return TARGET_SIGNAL_ALRM;
> +    case MIPS_SIGTERM: 	return TARGET_SIGNAL_TERM;
> +    case MIPS_SIGUSR1: 	return TARGET_SIGNAL_USR1;
> +    case MIPS_SIGUSR2: 	return TARGET_SIGNAL_USR2;
> +    case MIPS_SIGCHLD: 	return TARGET_SIGNAL_CHLD;
> +    case MIPS_SIGPWR: 	return TARGET_SIGNAL_PWR;
> +    case MIPS_SIGWINCH: return TARGET_SIGNAL_WINCH;
> +    case MIPS_SIGURG: 	return TARGET_SIGNAL_URG;
> +    case MIPS_SIGPOLL: 	return TARGET_SIGNAL_POLL;
> +    case MIPS_SIGSTOP: 	return TARGET_SIGNAL_STOP;
> +    case MIPS_SIGTSTP: 	return TARGET_SIGNAL_TSTP;
> +    case MIPS_SIGCONT: 	return TARGET_SIGNAL_CONT;
> +    case MIPS_SIGTTIN: 	return TARGET_SIGNAL_TTIN;
> +    case MIPS_SIGTTOU: 	return TARGET_SIGNAL_TTOU;
> +    case MIPS_SIGVTALRM: return TARGET_SIGNAL_VTALRM;
> +    case MIPS_SIGPROF: 	return TARGET_SIGNAL_PROF;
> +    case MIPS_SIGXCPU: 	return TARGET_SIGNAL_XCPU;
> +    case MIPS_SIGXFSZ: 	return TARGET_SIGNAL_XFSZ;
> +  }

 All the case statements need to be on the next line, correctly indented, 
e.g.:

   case MIPS_SIGXFSZ:
     return TARGET_SIGNAL_XFSZ;

> +    
> +#ifdef _SIG
> +#endif

 Some debug leftover I presume, please drop it.

> +
> +    return TARGET_SIGNAL_UNKNOWN;
> +}

 Indentation:

  return TARGET_SIGNAL_UNKNOWN;
}

> +
> +static int
> +mips_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal signo)
> +{
> +  printf ("MJE: mips_target_signal_to_host (<gdbarch>, signo=%d)\n", signo);

 Same as above.

> +
> +  switch (signo) {
> +    case TARGET_SIGNAL_0: 	return 0;
> +    case TARGET_SIGNAL_HUP:   	return MIPS_SIGHUP;
> +    case TARGET_SIGNAL_INT:   	return MIPS_SIGINT;
> +    case TARGET_SIGNAL_QUIT:   	return MIPS_SIGQUIT;
> +    case TARGET_SIGNAL_ILL:   	return MIPS_SIGILL;
> +    case TARGET_SIGNAL_TRAP:   	return MIPS_SIGTRAP;
> +    case TARGET_SIGNAL_ABRT:   	return MIPS_SIGABRT;
> +    case TARGET_SIGNAL_EMT:   	return MIPS_SIGEMT;
> +    case TARGET_SIGNAL_FPE:   	return MIPS_SIGFPE;
> +    case TARGET_SIGNAL_KILL:   	return MIPS_SIGKILL;
> +    case TARGET_SIGNAL_BUS:   	return MIPS_SIGBUS;
> +    case TARGET_SIGNAL_SEGV:   	return MIPS_SIGSEGV;
> +    case TARGET_SIGNAL_SYS:   	return MIPS_SIGSYS;
> +    case TARGET_SIGNAL_PIPE:   	return MIPS_SIGPIPE;
> +    case TARGET_SIGNAL_ALRM:   	return MIPS_SIGALRM;
> +    case TARGET_SIGNAL_TERM:   	return MIPS_SIGTERM;
> +    case TARGET_SIGNAL_USR1:   	return MIPS_SIGUSR1;
> +    case TARGET_SIGNAL_USR2:   	return MIPS_SIGUSR2;
> +    case TARGET_SIGNAL_CHLD:   	return MIPS_SIGCHLD;
> +    case TARGET_SIGNAL_PWR:   	return MIPS_SIGPWR;
> +    case TARGET_SIGNAL_WINCH:   return MIPS_SIGWINCH;
> +    case TARGET_SIGNAL_URG:   	return MIPS_SIGURG;
> +    case TARGET_SIGNAL_POLL:   	return MIPS_SIGPOLL;
> +    case TARGET_SIGNAL_STOP:   	return MIPS_SIGSTOP;
> +    case TARGET_SIGNAL_TSTP:   	return MIPS_SIGTSTP;
> +    case TARGET_SIGNAL_CONT:   	return MIPS_SIGCONT;
> +    case TARGET_SIGNAL_TTIN:   	return MIPS_SIGTTIN;
> +    case TARGET_SIGNAL_TTOU:   	return MIPS_SIGTTOU;
> +    case TARGET_SIGNAL_VTALRM:  return MIPS_SIGVTALRM;
> +    case TARGET_SIGNAL_PROF:   	return MIPS_SIGPROF;
> +    case TARGET_SIGNAL_XCPU:   	return MIPS_SIGXCPU;
> +    case TARGET_SIGNAL_XFSZ:   	return MIPS_SIGXFSZ;

 Ditto.

> +    
> +    default:
> +      warning ("Signal %s does not exist on this system.\n",
> +	       target_signal_to_name (signo));
> +      return 0;
> +  }
> +}
> +
>  /* Initialize one of the GNU/Linux OS ABIs.  */
>  
>  static void
> @@ -1203,6 +1298,11 @@ mips_linux_init_abi (struct gdbarch_info
>    set_gdbarch_core_read_description (gdbarch,
>  				     mips_linux_core_read_description);
>  
> +  set_gdbarch_target_signal_from_host (gdbarch,
> +                                       mips_target_signal_from_host);
> +  set_gdbarch_target_signal_to_host (gdbarch,
> +                                       mips_target_signal_to_host);
> +

 Indentation, leading tabs must be used:

  set_gdbarch_target_signal_from_host (gdbarch,
				       mips_target_signal_from_host);
  set_gdbarch_target_signal_to_host (gdbarch,
				     mips_target_signal_to_host);

>    tdep->syscall_next_pc = mips_linux_syscall_next_pc;
>  
>    if (tdesc_data)
> 
> Index: gdb/gdb/mips-linux-tdep.h
> ===================================================================
> --- gdb/.CC/cache/mips-linux-tdep.h@@/main/gdb_7_2_dev/1	2012-05-19 18:25:40.000041000 -0700
> +++ gdb/mips-linux-tdep.h	2012-05-19 10:53:12.000064000 -0700
> @@ -100,3 +100,43 @@ enum {
>  /* Return 1 if MIPS_RESTART_REGNUM is usable.  */
>  
>  int mips_linux_restart_reg_p (struct gdbarch *gdbarch);
> +
> +/* MIPS Signals -- adapted from linux/arch/mips/include/asm/signal.h.  */
> +
> +enum mips_signals {
> +  MIPS_SIGHUP	=	 1,	/* Hangup (POSIX).  */

 Indentation:

enum mips_signals
  {
    MIPS_SIGHUP		=	 1,	/* Hangup (POSIX).  */

etc.

> +  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).  */
> +  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).  */

 Use a space between MIPS_SIGVTALRM and = here.

> +  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).  */
> +  };
> +

 Otherwise OK, but let's wait for feedback from the others on the general 
problem I noted above.

  Maciej


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
       [not found] ` <alpine.DEB.1.10.1205211232260.11227@tp.orcam.me.uk>
@ 2012-05-21 18:21   ` Michael Eager
  2012-05-21 22:34     ` Maciej W. Rozycki
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Eager @ 2012-05-21 18:21 UTC (permalink / raw)
  To: Maciej W. Rozycki, gdb-patches

On 05/21/2012 10:53 AM, Maciej W. Rozycki wrote:
> Hi Michael,
>
> On Sun, 20 May 2012, Michael Eager wrote:
>
>> MIPS Linux has different values assigned to signals than
>> X86 or PPC Linux.   The result is that a SIGBUS on MIPS
>> (value 0xA) is reported as SIGUSR1, since that signal has
>> the that value on X86.  This causes confusion (obviously).
>> Telling gdb to handle SIGUSR1 will cause it to respond to
>> SIGBUS but ignore SIGUSR1 (which has the value 0x16 on MIPS).
>>
>> I didn't find a bug report for this in Bugzilla.  I
>> have trouble believing that this problem has gone unnoticed.
>
>   What target are you specifically referring to?  Can you describe a
> scenario where it happens?

Debugging a MIPS core file.  Pedro suggested that no one
would ever see this running gdbserver.

>   Ah, so that is with the core file target used in a cross-debugger, right?
> Well, no wonder nobody has noticed, this is a very unusual scenario -- I
> have used GDB for at least 15 years now and may have had a need to examine
> a core file with a cross-debugger perhaps once or twice -- and then some
> common signals looked for like SIGSEGV or SIGILL are identity mapped (and
> I knew the signal that caused the core file dumped beforehand anyway as
> it's printed as the program is being killed anyway).

Yes, this may be less common than I first thought.  SEGVs are much
more common than SIGBUS.

>   Overall it looks good to me, but has some coding style problems and some
> other small issues, listed below.
>
>   Also by the look of it, such changes will actually be required for all
> processor/OS targets as obviously when you have e.g. a MIPS/IRIX-host
> cross-debugger targetting i386/Linux then the signal numbers won't match
> again, except that in the other direction.  That certainly begs for a
> general solution, perhaps like one used by the RSP where the GDB core uses
> generic signal numbers the target side translate to/from them as
> appropriate (obviously the host OS may have no notion of signals at all).

Yes.

I'm not sure about creating a generic solution.  The same solution can
be adapted to Solaris or other OS's which have different signal value
assignments, but a solution for a target which doesn't have signals,
that would call for a target-based (i.e., gdbserver or core generation)
solution.

>   That looks like a post-7.5 material to me though -- we've lived with this
> problem for long enough that I don't think we need to rush and correct it
> a little more than a week before the release branch.

Right.

>
>> Index: gdb/gdb/mips-linux-tdep.c
>> ===================================================================
>> --- gdb/.CC/cache/mips-linux-tdep.c@@/main/gdb_7_2_dev/3	2012-05-19 18:25:39.000077000 -0700
>> +++ gdb/mips-linux-tdep.c	2012-05-19 18:25:27.000033000 -0700
>> @@ -1130,6 +1130,101 @@ mips_linux_syscall_next_pc (struct frame
>>     return pc + 4;
>>   }
>>
>> +/* Translate signals based on MIPS signal values.
>> +   Adapted from gdb/common/signals.c.  */
>> +
>> +static enum target_signal
>> +mips_target_signal_from_host (struct gdbarch *gdbarch, int signo)
>> +{
>> +  printf ("MJE: mips_target_signal_from_host (<gdbarch>, signo=%d)\n", signo);
>
>   I'm assuming this is a debug message, please drop it.  Otherwise we use
> calls like fprintf_unfiltered (...) to produce output; see other sources
> for a reference.

Yes, it's my internal debugging message.  I forgot to remove it.
Mostly, I wanted to get an idea of whether I was heading in the
wrong direction.

I'll clean up the formatting issue you mentioned when I post the
finished patch.

>
>> +
>> +  switch (signo) {
>> +    case 0:		return TARGET_SIGNAL_0;
>> +    case MIPS_SIGHUP: 	return TARGET_SIGNAL_HUP;
>> +    case MIPS_SIGINT: 	return TARGET_SIGNAL_INT;
>> +    case MIPS_SIGQUIT: 	return TARGET_SIGNAL_QUIT;
>> +    case MIPS_SIGILL: 	return TARGET_SIGNAL_ILL;
>> +    case MIPS_SIGTRAP: 	return TARGET_SIGNAL_TRAP;
>> +    case MIPS_SIGABRT: 	return TARGET_SIGNAL_ABRT;
>> +    case MIPS_SIGEMT: 	return TARGET_SIGNAL_EMT;
>> +    case MIPS_SIGFPE: 	return TARGET_SIGNAL_FPE;
>> +    case MIPS_SIGKILL: 	return TARGET_SIGNAL_KILL;
>> +    case MIPS_SIGBUS: 	return TARGET_SIGNAL_BUS;
>> +    case MIPS_SIGSEGV: 	return TARGET_SIGNAL_SEGV;
>> +    case MIPS_SIGSYS: 	return TARGET_SIGNAL_SYS;
>> +    case MIPS_SIGPIPE: 	return TARGET_SIGNAL_PIPE;
>> +    case MIPS_SIGALRM: 	return TARGET_SIGNAL_ALRM;
>> +    case MIPS_SIGTERM: 	return TARGET_SIGNAL_TERM;
>> +    case MIPS_SIGUSR1: 	return TARGET_SIGNAL_USR1;
>> +    case MIPS_SIGUSR2: 	return TARGET_SIGNAL_USR2;
>> +    case MIPS_SIGCHLD: 	return TARGET_SIGNAL_CHLD;
>> +    case MIPS_SIGPWR: 	return TARGET_SIGNAL_PWR;
>> +    case MIPS_SIGWINCH: return TARGET_SIGNAL_WINCH;
>> +    case MIPS_SIGURG: 	return TARGET_SIGNAL_URG;
>> +    case MIPS_SIGPOLL: 	return TARGET_SIGNAL_POLL;
>> +    case MIPS_SIGSTOP: 	return TARGET_SIGNAL_STOP;
>> +    case MIPS_SIGTSTP: 	return TARGET_SIGNAL_TSTP;
>> +    case MIPS_SIGCONT: 	return TARGET_SIGNAL_CONT;
>> +    case MIPS_SIGTTIN: 	return TARGET_SIGNAL_TTIN;
>> +    case MIPS_SIGTTOU: 	return TARGET_SIGNAL_TTOU;
>> +    case MIPS_SIGVTALRM: return TARGET_SIGNAL_VTALRM;
>> +    case MIPS_SIGPROF: 	return TARGET_SIGNAL_PROF;
>> +    case MIPS_SIGXCPU: 	return TARGET_SIGNAL_XCPU;
>> +    case MIPS_SIGXFSZ: 	return TARGET_SIGNAL_XFSZ;
>> +  }
>
>   All the case statements need to be on the next line, correctly indented,
> e.g.:
>
>     case MIPS_SIGXFSZ:
>       return TARGET_SIGNAL_XFSZ;
>
>> +
>> +#ifdef _SIG
>> +#endif
>
>   Some debug leftover I presume, please drop it.
>
>> +
>> +    return TARGET_SIGNAL_UNKNOWN;
>> +}
>
>   Indentation:
>
>    return TARGET_SIGNAL_UNKNOWN;
> }
>
>> +
>> +static int
>> +mips_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal signo)
>> +{
>> +  printf ("MJE: mips_target_signal_to_host (<gdbarch>, signo=%d)\n", signo);
>
>   Same as above.
>
>> +
>> +  switch (signo) {
>> +    case TARGET_SIGNAL_0: 	return 0;
>> +    case TARGET_SIGNAL_HUP:   	return MIPS_SIGHUP;
>> +    case TARGET_SIGNAL_INT:   	return MIPS_SIGINT;
>> +    case TARGET_SIGNAL_QUIT:   	return MIPS_SIGQUIT;
>> +    case TARGET_SIGNAL_ILL:   	return MIPS_SIGILL;
>> +    case TARGET_SIGNAL_TRAP:   	return MIPS_SIGTRAP;
>> +    case TARGET_SIGNAL_ABRT:   	return MIPS_SIGABRT;
>> +    case TARGET_SIGNAL_EMT:   	return MIPS_SIGEMT;
>> +    case TARGET_SIGNAL_FPE:   	return MIPS_SIGFPE;
>> +    case TARGET_SIGNAL_KILL:   	return MIPS_SIGKILL;
>> +    case TARGET_SIGNAL_BUS:   	return MIPS_SIGBUS;
>> +    case TARGET_SIGNAL_SEGV:   	return MIPS_SIGSEGV;
>> +    case TARGET_SIGNAL_SYS:   	return MIPS_SIGSYS;
>> +    case TARGET_SIGNAL_PIPE:   	return MIPS_SIGPIPE;
>> +    case TARGET_SIGNAL_ALRM:   	return MIPS_SIGALRM;
>> +    case TARGET_SIGNAL_TERM:   	return MIPS_SIGTERM;
>> +    case TARGET_SIGNAL_USR1:   	return MIPS_SIGUSR1;
>> +    case TARGET_SIGNAL_USR2:   	return MIPS_SIGUSR2;
>> +    case TARGET_SIGNAL_CHLD:   	return MIPS_SIGCHLD;
>> +    case TARGET_SIGNAL_PWR:   	return MIPS_SIGPWR;
>> +    case TARGET_SIGNAL_WINCH:   return MIPS_SIGWINCH;
>> +    case TARGET_SIGNAL_URG:   	return MIPS_SIGURG;
>> +    case TARGET_SIGNAL_POLL:   	return MIPS_SIGPOLL;
>> +    case TARGET_SIGNAL_STOP:   	return MIPS_SIGSTOP;
>> +    case TARGET_SIGNAL_TSTP:   	return MIPS_SIGTSTP;
>> +    case TARGET_SIGNAL_CONT:   	return MIPS_SIGCONT;
>> +    case TARGET_SIGNAL_TTIN:   	return MIPS_SIGTTIN;
>> +    case TARGET_SIGNAL_TTOU:   	return MIPS_SIGTTOU;
>> +    case TARGET_SIGNAL_VTALRM:  return MIPS_SIGVTALRM;
>> +    case TARGET_SIGNAL_PROF:   	return MIPS_SIGPROF;
>> +    case TARGET_SIGNAL_XCPU:   	return MIPS_SIGXCPU;
>> +    case TARGET_SIGNAL_XFSZ:   	return MIPS_SIGXFSZ;
>
>   Ditto.
>
>> +
>> +    default:
>> +      warning ("Signal %s does not exist on this system.\n",
>> +	       target_signal_to_name (signo));
>> +      return 0;
>> +  }
>> +}
>> +
>>   /* Initialize one of the GNU/Linux OS ABIs.  */
>>
>>   static void
>> @@ -1203,6 +1298,11 @@ mips_linux_init_abi (struct gdbarch_info
>>     set_gdbarch_core_read_description (gdbarch,
>>   				     mips_linux_core_read_description);
>>
>> +  set_gdbarch_target_signal_from_host (gdbarch,
>> +                                       mips_target_signal_from_host);
>> +  set_gdbarch_target_signal_to_host (gdbarch,
>> +                                       mips_target_signal_to_host);
>> +
>
>   Indentation, leading tabs must be used:
>
>    set_gdbarch_target_signal_from_host (gdbarch,
> 				       mips_target_signal_from_host);
>    set_gdbarch_target_signal_to_host (gdbarch,
> 				     mips_target_signal_to_host);
>
>>     tdep->syscall_next_pc = mips_linux_syscall_next_pc;
>>
>>     if (tdesc_data)
>>
>> Index: gdb/gdb/mips-linux-tdep.h
>> ===================================================================
>> --- gdb/.CC/cache/mips-linux-tdep.h@@/main/gdb_7_2_dev/1	2012-05-19 18:25:40.000041000 -0700
>> +++ gdb/mips-linux-tdep.h	2012-05-19 10:53:12.000064000 -0700
>> @@ -100,3 +100,43 @@ enum {
>>   /* Return 1 if MIPS_RESTART_REGNUM is usable.  */
>>
>>   int mips_linux_restart_reg_p (struct gdbarch *gdbarch);
>> +
>> +/* MIPS Signals -- adapted from linux/arch/mips/include/asm/signal.h.  */
>> +
>> +enum mips_signals {
>> +  MIPS_SIGHUP	=	 1,	/* Hangup (POSIX).  */
>
>   Indentation:
>
> enum mips_signals
>    {
>      MIPS_SIGHUP		=	 1,	/* Hangup (POSIX).  */
>
> etc.
>
>> +  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).  */
>> +  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).  */
>
>   Use a space between MIPS_SIGVTALRM and = here.
>
>> +  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).  */
>> +  };
>> +
>
>   Otherwise OK, but let's wait for feedback from the others on the general
> problem I noted above.

Yep.


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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-20  2:03 MIPS Linux signals Michael Eager
                   ` (2 preceding siblings ...)
       [not found] ` <alpine.DEB.1.10.1205211232260.11227@tp.orcam.me.uk>
@ 2012-05-21 21:35 ` Pedro Alves
  2012-05-21 21:53   ` Michael Eager
  3 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-21 21:35 UTC (permalink / raw)
  To: Michael Eager; +Cc: gdb-patches

On 05/20/2012 03:02 AM, Michael Eager wrote:

> +static int
> +mips_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal signo)
> +{
> +  printf ("MJE: mips_target_signal_to_host (<gdbarch>, signo=%d)\n", signo);
> +
> +  switch (signo) {
> +    case TARGET_SIGNAL_0: 	return 0;
> +    case TARGET_SIGNAL_HUP:   	return MIPS_SIGHUP;


BTW, I wouldn't bother with gdbarch_target_signal_to_host.  Nothing ever calls it.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 21:35 ` Pedro Alves
@ 2012-05-21 21:53   ` Michael Eager
  2012-05-21 22:48     ` Maciej W. Rozycki
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Eager @ 2012-05-21 21:53 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 05/21/2012 02:34 PM, Pedro Alves wrote:
> On 05/20/2012 03:02 AM, Michael Eager wrote:
>
>> +static int
>> +mips_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal signo)
>> +{
>> +  printf ("MJE: mips_target_signal_to_host (<gdbarch>, signo=%d)\n", signo);
>> +
>> +  switch (signo) {
>> +    case TARGET_SIGNAL_0: 	return 0;
>> +    case TARGET_SIGNAL_HUP:   	return MIPS_SIGHUP;
>
>
> BTW, I wouldn't bother with gdbarch_target_signal_to_host.  Nothing ever calls it.

I hadn't noticed that.  I thought that it was called to translate
the signal number when sent to the target.  Instead, target_signal_to_host()
is called.

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 18:21   ` Michael Eager
@ 2012-05-21 22:34     ` Maciej W. Rozycki
  2012-05-22  9:38       ` Pedro Alves
  0 siblings, 1 reply; 29+ messages in thread
From: Maciej W. Rozycki @ 2012-05-21 22:34 UTC (permalink / raw)
  To: Michael Eager; +Cc: gdb-patches

On Mon, 21 May 2012, Michael Eager wrote:

> >   What target are you specifically referring to?  Can you describe a
> > scenario where it happens?
> 
> Debugging a MIPS core file.  Pedro suggested that no one
> would ever see this running gdbserver.

 Well, these are mutually exclusive -- you can't open a remote target and 
a core file both at a time, and it's only the signal number that's 
recorded in the core image as the cause of termination that's causing 
trouble.  I wonder if there's more stuff there actually that needs 
mapping.

> >   Ah, so that is with the core file target used in a cross-debugger, right?
> > Well, no wonder nobody has noticed, this is a very unusual scenario -- I
> > have used GDB for at least 15 years now and may have had a need to examine
> > a core file with a cross-debugger perhaps once or twice -- and then some
> > common signals looked for like SIGSEGV or SIGILL are identity mapped (and
> > I knew the signal that caused the core file dumped beforehand anyway as
> > it's printed as the program is being killed anyway).
> 
> Yes, this may be less common than I first thought.  SEGVs are much
> more common than SIGBUS.

 Actually you'll only see SIGBUS in two cases under MIPS/Linux:

1. An unaligned memory access -- these trap into the kernel if not made 
   correctly (with the special unaligned access instructions) and are 
   normally emulated by the kernel at a large performance expense for data 
   accesses.  You can still get one if you try to execute unaligned 
   instructions (e.g. MIPS16 code on a processor that does not support 
   that mode of execution; MIPS16 code addresses have the lowest bit set), 
   try to access unaligned data outside the user address space (USEG or 
   XUSEG as applicable; the lack of alignment takes precedence over MMU 
   protection) or if you disable the kernel emulation with the obscure 
   syscall that's there for special cases.

2. You actually get a true bus error signalled by system logic in an 
   external bus transaction, e.g. an uncorrected memory ECC error or 
   suchlike.

As you can see it's all pretty obscure and uncommon stuff.

> >   Also by the look of it, such changes will actually be required for all
> > processor/OS targets as obviously when you have e.g. a MIPS/IRIX-host
> > cross-debugger targetting i386/Linux then the signal numbers won't match
> > again, except that in the other direction.  That certainly begs for a
> > general solution, perhaps like one used by the RSP where the GDB core uses
> > generic signal numbers the target side translate to/from them as
> > appropriate (obviously the host OS may have no notion of signals at all).
> 
> Yes.
> 
> I'm not sure about creating a generic solution.  The same solution can
> be adapted to Solaris or other OS's which have different signal value
> assignments, but a solution for a target which doesn't have signals,
> that would call for a target-based (i.e., gdbserver or core generation)
> solution.

 Targets with no signals are not a problem, they simply won't produce or 
accept any.  It's the hosts that are a problem -- generic code in 
common/signals.c has stuff like this:

#if defined (SIGHUP)
    case TARGET_SIGNAL_HUP:
      return SIGHUP;
#endif

that obviosuly won't work on on a host that doesn't have such a signal 
defined (I'm not sure if hosts like MinGW or DJGPP pretend that these 
signals exist there, that would make little sense; there may be others as 
well).  As long as every target that supports signals provides its mapping 
similar to one you did, then I think we should be fine.

 And actually, as far as Linux is concerned, I suppose we can actually 
have a common mapping that will cover most of the processor architectures 
Linux supports as most if not all of the modern ports have standardised on 
a common set of magic numbers, as far as I know.  So linux-tdep.c would 
default to that standard mapping and then any oddball port would override 
that choice with their own.

 It's only the few older ports that had their reasons, legitimate or not, 
to stick to other definitions that are different -- for the MIPS port for 
example there used to be an idea once to run IRIX binaries using a foreign 
kernel personality and an ABI compatibility layer (similar to iBCS the 
i386/Linux port used to support) -- reusing the same magic numbers would 
certainly make the effort easier (many kernel structures exposed via 
syscalls are like those from IRIX rather than other Linux ports for the 
very same reason).  To the best of my knowledge this idea never fully 
materialised, but these provisions have stayed for backwards compatibility 
with the early decisions.

 Another example is the Alpha/Linux port and its actual possibility to run 
DEC OSF/1 (or Digital Unix or Tru64 Unix or whatever, hmm, lost track...) 
binaries; these are not ELF even, but ECOFF.  There are probably a few 
more (<linux/personality.h> suggests SunOS and Solaris, presumably on 
SPARC and HP-UX presumably on HP-PA), but the rest should be pretty 
uniform.

  Maciej


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 21:53   ` Michael Eager
@ 2012-05-21 22:48     ` Maciej W. Rozycki
  2012-05-22  0:16       ` Michael Eager
                         ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Maciej W. Rozycki @ 2012-05-21 22:48 UTC (permalink / raw)
  To: Michael Eager; +Cc: Pedro Alves, gdb-patches

On Mon, 21 May 2012, Michael Eager wrote:

> > BTW, I wouldn't bother with gdbarch_target_signal_to_host.  Nothing ever
> > calls it.
> 
> I hadn't noticed that.  I thought that it was called to translate
> the signal number when sent to the target.  Instead, target_signal_to_host()
> is called.

 Shall we drop the unused gdbarch API so as to avoid further confusion 
then?  Shouldn't target_signal_from_host be renamed to something closer to 
what it really does, e.g. signal_from_target?  It's not that host signals 
really ever matter unless host == target in which case they're still 
target signals too (this observation applies to gdbserver as well).

  Maciej


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 22:48     ` Maciej W. Rozycki
@ 2012-05-22  0:16       ` Michael Eager
  2012-05-22 10:17       ` Pedro Alves
  2012-05-22 10:58       ` Pedro Alves
  2 siblings, 0 replies; 29+ messages in thread
From: Michael Eager @ 2012-05-22  0:16 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Pedro Alves, gdb-patches

On 05/21/2012 03:48 PM, Maciej W. Rozycki wrote:
> On Mon, 21 May 2012, Michael Eager wrote:
>
>>> BTW, I wouldn't bother with gdbarch_target_signal_to_host.  Nothing ever
>>> calls it.
>>
>> I hadn't noticed that.  I thought that it was called to translate
>> the signal number when sent to the target.  Instead, target_signal_to_host()
>> is called.
>
>   Shall we drop the unused gdbarch API so as to avoid further confusion
> then?  Shouldn't target_signal_from_host be renamed to something closer to
> what it really does, e.g. signal_from_target?  It's not that host signals
> really ever matter unless host == target in which case they're still
> target signals too (this observation applies to gdbserver as well).

This would be OK with me.  The gdbarch_target_signal_to_host interface
suggests that this should differ for different targets.

There is an old bug report about the unclear name for the function.
gdbarch_signal_from_target would be a better name.

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 22:34     ` Maciej W. Rozycki
@ 2012-05-22  9:38       ` Pedro Alves
  0 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-22  9:38 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Michael Eager, gdb-patches

On 05/21/2012 11:33 PM, Maciej W. Rozycki wrote:

>  And actually, as far as Linux is concerned, I suppose we can actually 
> have a common mapping that will cover most of the processor architectures 
> Linux supports as most if not all of the modern ports have standardised on 
> a common set of magic numbers, as far as I know.  


Assuming so ...

> So linux-tdep.c would 
> default to that standard mapping and then any oddball port would override 
> that choice with their own.


... then very much agreed.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 22:48     ` Maciej W. Rozycki
  2012-05-22  0:16       ` Michael Eager
@ 2012-05-22 10:17       ` Pedro Alves
  2012-05-22 13:16         ` Maciej W. Rozycki
                           ` (3 more replies)
  2012-05-22 10:58       ` Pedro Alves
  2 siblings, 4 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 10:17 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Michael Eager, gdb-patches

On 05/21/2012 11:48 PM, Maciej W. Rozycki wrote:

> On Mon, 21 May 2012, Michael Eager wrote:
> 
>>> BTW, I wouldn't bother with gdbarch_target_signal_to_host.  Nothing ever
>>> calls it.
>>
>> I hadn't noticed that.  I thought that it was called to translate
>> the signal number when sent to the target.  Instead, target_signal_to_host()
>> is called.
> 
>  Shall we drop the unused gdbarch API so as to avoid further confusion 
> then?  Shouldn't target_signal_from_host be renamed  to something closer to what

> it really does, e.g. signal_from_target?

I agree that the current naming is confusing, but I'll point out why I think
target_signal_from_host is actually correct, and then conclude with renaming
suggestions to avoid the confusion, and fix the naming of the gdbarch hook, which
I think is not correct.  We have

enum target_signal
{
...
  TARGET_SIGNAL_FOO,
  TARGET_SIGNAL_BAR,
...
};

which represents GDB's internal signal numbers.  AFAIK, the "target_" prefix
naming in this case is just a natural choice, given that that's how we name
everything behind the target_ops abstraction -- target_read_memory,
target_resume, etc., etc. (target.h) -- the mechanism that is mostly about
abstracting the details of handling the target's debug interface API.

So this explains the "target_signal_from_" part of the function's signature.

The "from_host" part is really correct: this really is a host function -- think in
terms of autoconf's notion of build vs host vs target.  It only makes sense to call
it in native code (native == host).  And in fact, that's what happens.  Grepping
for calls in GDB we see:

=====================
arch-utils.c
arch-utils.h

 This is default_target_signal_from_host .  Which is the gdbarch fallback,
 which only works on native cores.

=====================
corelow.c

      /* NOTE: target_signal_from_host() converts a target signal
         value into gdb's internal signal value.  Unfortunately gdb's
         internal value is called ``target_signal'' and this function
         got the name ..._from_host().  */
      enum target_signal sig = (core_gdbarch != NULL
                       ? gdbarch_target_signal_from_host (core_gdbarch,
                                                          siggy)
                       : target_signal_from_host (siggy));

      printf_filtered (_("Program terminated with signal %d, %s.\n"),
                       siggy, target_signal_to_string (sig));

This is the only place the gdbarch method is called, which is handling
the cross-core scenario.  If we don't have a gdbarch hook to do the
translation for us, then we fallback to assuming we're debugging a native
core, one that was generated on the host (or a similar machine).

=====================
darwin-nat.c

 Native code.

=====================
gdbarch.c
gdbarch.h
gdbarch.sh

 The gdbarch hook.

=====================
gnu-nat.c
inf-ttrace.c
linux-nat.c
linux-thread-db.c
nto-procfs.c
procfs.c

 Native code.

=====================
remote-mips.c

 Just a comment, but actually interesting in the MIPS context.

/* Return the signal corresponding to SIG, where SIG is the number which
   the MIPS protocol uses for the signal.  */

static enum target_signal
mips_signal_from_protocol (int sig)
{
  /* We allow a few more signals than the IDT board actually returns, on
     the theory that there is at least *some* hope that perhaps the numbering
     for these signals is widely agreed upon.  */
  if (sig <= 0
      || sig > 31)
    return TARGET_SIGNAL_UNKNOWN;

  /* Don't want to use target_signal_from_host because we are converting
     from MIPS signal numbers, not host ones.  Our internal numbers
     match the MIPS numbers for the signals the board can return, which
     are: SIGINT, SIGSEGV, SIGBUS, SIGILL, SIGFPE, SIGTRAP.  */
  return (enum target_signal) sig;
}

=====================
target.c

Actually native code.  I think this could move to inf-child.c.

/* Helper function for child_wait and the derivatives of child_wait.
   HOSTSTATUS is the waitstatus from wait() or the equivalent; store our
   translation of that in OURSTATUS.  */
void
store_waitstatus (struct target_waitstatus *ourstatus, int hoststatus)
{
  if (WIFEXITED (hoststatus))
    {
      ourstatus->kind = TARGET_WAITKIND_EXITED;
      ourstatus->value.integer = WEXITSTATUS (hoststatus);
    }
  else if (!WIFSTOPPED (hoststatus))
    {
      ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
      ourstatus->value.sig = target_signal_from_host (WTERMSIG (hoststatus));
    }
  else
    {
      ourstatus->kind = TARGET_WAITKIND_STOPPED;
      ourstatus->value.sig = target_signal_from_host (WSTOPSIG (hoststatus));
    }
}

=====================
windows-nat.c

 Native code.

=====================

gdbserver is always native, so always host.


Now, it's the mix between somewhat different etymologies (enum target_signal,
vs build/host/target) that leads to confusion.  Renaming enum target_signal
to enum gdb_signal would fix that side of the problem.

= The gdbarch hook =

Another source of confusion is that the gdbarch hook is naturally not a host
function, it concerns with converting a foreign target's signal numbers (for cross
debugging, not host numbers, unless in the special case of host == target) to
gdb's internal numbers.  So in that sense,

  gdbarch_target_signal_from_host

is the wrong name.  It should be called gdbarch_target_signal_from_target (!)
with the default, as today, being target_signal_from_host.

Obviously, gdbarch_target_signal_from_target is even more confusing.  :-)  The two
"targets" in the name refer to somewhat different ideas.

So I think that to sort all of this out, we should:

enum target_signal => enum gdb_signal

target_signal_from_host => gdb_signal_from_host (or gdb_signal_from_host_signal)
target_signal_to_host => gdb_signal_to_host (or gdb_signal_to_host_signal)

gdbarch_target_signal_from_host => gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
gdbarch_target_signal_to_host => gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-21 22:48     ` Maciej W. Rozycki
  2012-05-22  0:16       ` Michael Eager
  2012-05-22 10:17       ` Pedro Alves
@ 2012-05-22 10:58       ` Pedro Alves
  2012-05-22 19:31         ` Aleksandar Ristovski
  2 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 10:58 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Michael Eager, gdb-patches, Aleksandar Ristovski

[Adding Aleksandar]

Answering this one.

On 05/21/2012 11:48 PM, Maciej W. Rozycki wrote:

>  Shall we drop the unused gdbarch API so as to avoid further confusion 
> then?  


If we don't find a use for it, yes.

Aleksandar, we're discussing gdbarch_target_signal_from_host and
gdbarch_target_signal_to_host.  It turns out that uses to either of those
were never added to GDB.  gdbarch_target_signal_FROM_host's purpose is clear,
and we're about to add a (new) use to fix the same situation you ran into at the
time (cross core debugging).  I'm wondering if you ever found a use for
gdbarch_target_signal_TO_host that we should consider, though.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 10:17       ` Pedro Alves
@ 2012-05-22 13:16         ` Maciej W. Rozycki
  2012-05-22 13:32           ` Pedro Alves
  2012-05-22 15:10         ` Move store_waitstatus to inf-child.c (was: Re: MIPS Linux signals) Pedro Alves
                           ` (2 subsequent siblings)
  3 siblings, 1 reply; 29+ messages in thread
From: Maciej W. Rozycki @ 2012-05-22 13:16 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Michael Eager, gdb-patches

On Tue, 22 May 2012, Pedro Alves wrote:

> So I think that to sort all of this out, we should:
> 
> enum target_signal => enum gdb_signal
> 
> target_signal_from_host => gdb_signal_from_host (or gdb_signal_from_host_signal)
> target_signal_to_host => gdb_signal_to_host (or gdb_signal_to_host_signal)
> 
> gdbarch_target_signal_from_host => gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
> gdbarch_target_signal_to_host => gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)

 How about we call the context of the signal "inferior" here and therefore 
avoid the confusion which target is the host and which host is the target 
altogether?  I.e.:

enum target_signal => enum gdb_signal

target_signal_from_host => gdb_signal_from_inferior
target_signal_to_host => gdb_signal_to_inferior

gdbarch_target_signal_from_host => gdbarch_gdb_signal_from_inferior
gdbarch_target_signal_to_host => gdbarch_gdb_signal_to_inferior

  Maciej


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 13:16         ` Maciej W. Rozycki
@ 2012-05-22 13:32           ` Pedro Alves
  0 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 13:32 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Michael Eager, gdb-patches

On 05/22/2012 02:16 PM, Maciej W. Rozycki wrote:

> On Tue, 22 May 2012, Pedro Alves wrote:
> 
>> > So I think that to sort all of this out, we should:
>> > 
>> > enum target_signal => enum gdb_signal
>> > 
>> > target_signal_from_host => gdb_signal_from_host (or gdb_signal_from_host_signal)
>> > target_signal_to_host => gdb_signal_to_host (or gdb_signal_to_host_signal)
>> > 
>> > gdbarch_target_signal_from_host => gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
>> > gdbarch_target_signal_to_host => gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)
>  How about we call the context of the signal "inferior" here and therefore 
> avoid the confusion which target is the host and which host is the target 
> altogether?  I.e.:
> 
> enum target_signal => enum gdb_signal
> 
> target_signal_from_host => gdb_signal_from_inferior
> target_signal_to_host => gdb_signal_to_inferior
> 
> gdbarch_target_signal_from_host => gdbarch_gdb_signal_from_inferior
> gdbarch_target_signal_to_host => gdbarch_gdb_signal_to_inferior


This blurs the point that gdb_signal_from_host/gdb_signal_from_inferior
should never be called if not from native/host code, IOW, only when you want
a mapping of the signal of the system the code is running on (host) and will
thus add to confusion.  I prefer my variant for making the distinction clear
with the naming.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Move store_waitstatus to inf-child.c (was: Re: MIPS Linux signals)
  2012-05-22 10:17       ` Pedro Alves
  2012-05-22 13:16         ` Maciej W. Rozycki
@ 2012-05-22 15:10         ` Pedro Alves
  2012-05-22 15:40         ` MIPS Linux signals Michael Eager
  2012-05-22 16:26         ` Pedro Alves
  3 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 15:10 UTC (permalink / raw)
  To: gdb-patches

On 05/22/2012 11:17 AM, Pedro Alves wrote:

> =====================
> target.c
> 
> Actually native code.  I think this could move to inf-child.c.
> 
> /* Helper function for child_wait and the derivatives of child_wait.
>    HOSTSTATUS is the waitstatus from wait() or the equivalent; store our
>    translation of that in OURSTATUS.  */
> void
> store_waitstatus (struct target_waitstatus *ourstatus, int hoststatus)


This patch moves it.

This is a function only used by (UNIX-ish) native targets, so inf-child.c
is a better place.  Interestingly, inf-child.o is always linked in through
COMMON_OBS, instead of being picked up by .mh fragments...

Build tested on X86_64 Fedora 16 as well as cross built for mingw.  The amount
of grepping I did makes me reasonably confident I haven't broken any
target, but you never know...

2012-05-22  Pedro Alves  <palves@redhat.com>

	* target.h (store_waitstatus): Move declaration ...
	* inf-child.h (store_waitstatus): ... here.
	* target.c: Move inclusion of gdb_wait.h, and ...
	(store_waitstatus): ... this ...
	* inf-child.c: ... here.
	* linux-nat.c: Include inf-child.h.
	* rs6000-nat.c: Include inf-child.h.
	* spu-linux-nat.c: Include inf-child.h.
---
 gdb/inf-child.c     |   24 ++++++++++++++++++++++++
 gdb/inf-child.h     |    5 +++++
 gdb/linux-nat.c     |    1 +
 gdb/rs6000-nat.c    |    1 +
 gdb/spu-linux-nat.c |    1 +
 gdb/target.c        |   24 ------------------------
 gdb/target.h        |    4 ----
 7 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 5531102..08955ea 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -30,6 +30,7 @@
 #include "inf-child.h"
 #include "gdb/fileio.h"
 #include "agent.h"
+#include "gdb_wait.h"

 #ifdef HAVE_SYS_PARAM_H
 #include <sys/param.h>		/* for MAXPATHLEN */
@@ -38,6 +39,29 @@
 #include <fcntl.h>
 #include <unistd.h>

+/* Helper function for child_wait and the derivatives of child_wait.
+   HOSTSTATUS is the waitstatus from wait() or the equivalent; store our
+   translation of that in OURSTATUS.  */
+void
+store_waitstatus (struct target_waitstatus *ourstatus, int hoststatus)
+{
+  if (WIFEXITED (hoststatus))
+    {
+      ourstatus->kind = TARGET_WAITKIND_EXITED;
+      ourstatus->value.integer = WEXITSTATUS (hoststatus);
+    }
+  else if (!WIFSTOPPED (hoststatus))
+    {
+      ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
+      ourstatus->value.sig = target_signal_from_host (WTERMSIG (hoststatus));
+    }
+  else
+    {
+      ourstatus->kind = TARGET_WAITKIND_STOPPED;
+      ourstatus->value.sig = target_signal_from_host (WSTOPSIG (hoststatus));
+    }
+}
+
 /* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
    for all registers.  */

diff --git a/gdb/inf-child.h b/gdb/inf-child.h
index d32c8cb..c394d5a 100644
--- a/gdb/inf-child.h
+++ b/gdb/inf-child.h
@@ -25,4 +25,9 @@

 extern struct target_ops *inf_child_target (void);

+/* Functions for helping to write a native target.  */
+
+/* This is for native targets which use a unix/POSIX-style waitstatus.  */
+extern void store_waitstatus (struct target_waitstatus *, int);
+
 #endif
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index ac1a0ea..86b3c7d 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -36,6 +36,7 @@
 #include "gdbcmd.h"
 #include "regcache.h"
 #include "regset.h"
+#include "inf-child.h"
 #include "inf-ptrace.h"
 #include "auxv.h"
 #include <sys/param.h>		/* for MAXPATHLEN */
diff --git a/gdb/rs6000-nat.c b/gdb/rs6000-nat.c
index 117ca01..b2b3e7e 100644
--- a/gdb/rs6000-nat.c
+++ b/gdb/rs6000-nat.c
@@ -31,6 +31,7 @@
 #include "gdb-stabs.h"
 #include "regcache.h"
 #include "arch-utils.h"
+#include "inf-child.h"
 #include "inf-ptrace.h"
 #include "ppc-tdep.h"
 #include "rs6000-tdep.h"
diff --git a/gdb/spu-linux-nat.c b/gdb/spu-linux-nat.c
index 7ddfc7a..235458e 100644
--- a/gdb/spu-linux-nat.c
+++ b/gdb/spu-linux-nat.c
@@ -23,6 +23,7 @@
 #include "gdb_string.h"
 #include "target.h"
 #include "inferior.h"
+#include "inf-child.h"
 #include "inf-ptrace.h"
 #include "regcache.h"
 #include "symfile.h"
diff --git a/gdb/target.c b/gdb/target.c
index 5361dbc..6dba936 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -29,7 +29,6 @@
 #include "bfd.h"
 #include "symfile.h"
 #include "objfiles.h"
-#include "gdb_wait.h"
 #include "dcache.h"
 #include <signal.h>
 #include "regcache.h"
@@ -3650,29 +3649,6 @@ generic_mourn_inferior (void)
     deprecated_detach_hook ();
 }
 \f
-/* Helper function for child_wait and the derivatives of child_wait.
-   HOSTSTATUS is the waitstatus from wait() or the equivalent; store our
-   translation of that in OURSTATUS.  */
-void
-store_waitstatus (struct target_waitstatus *ourstatus, int hoststatus)
-{
-  if (WIFEXITED (hoststatus))
-    {
-      ourstatus->kind = TARGET_WAITKIND_EXITED;
-      ourstatus->value.integer = WEXITSTATUS (hoststatus);
-    }
-  else if (!WIFSTOPPED (hoststatus))
-    {
-      ourstatus->kind = TARGET_WAITKIND_SIGNALLED;
-      ourstatus->value.sig = target_signal_from_host (WTERMSIG (hoststatus));
-    }
-  else
-    {
-      ourstatus->kind = TARGET_WAITKIND_STOPPED;
-      ourstatus->value.sig = target_signal_from_host (WSTOPSIG (hoststatus));
-    }
-}
-\f
 /* Convert a normal process ID to a string.  Returns the string in a
    static buffer.  */

diff --git a/gdb/target.h b/gdb/target.h
index 50a0ea6..84b462a 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1857,10 +1857,6 @@ extern int baud_rate;
 extern int remote_timeout;

 \f
-/* Functions for helping to write a native target.  */
-
-/* This is for native targets which use a unix/POSIX-style waitstatus.  */
-extern void store_waitstatus (struct target_waitstatus *, int);

 /* Set the show memory breakpoints mode to show, and installs a cleanup
    to restore it back to the current value.  */


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 10:17       ` Pedro Alves
  2012-05-22 13:16         ` Maciej W. Rozycki
  2012-05-22 15:10         ` Move store_waitstatus to inf-child.c (was: Re: MIPS Linux signals) Pedro Alves
@ 2012-05-22 15:40         ` Michael Eager
  2012-05-22 16:02           ` Pedro Alves
  2012-05-22 16:26         ` Pedro Alves
  3 siblings, 1 reply; 29+ messages in thread
From: Michael Eager @ 2012-05-22 15:40 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Maciej W. Rozycki, gdb-patches

On 05/22/2012 03:17 AM, Pedro Alves wrote:

> I agree that the current naming is confusing, but I'll point out why I think
> target_signal_from_host is actually correct, and then conclude with renaming
> suggestions to avoid the confusion, and fix the naming of the gdbarch hook, which
> I think is not correct.  We have
>
> enum target_signal
> {
> ...
>    TARGET_SIGNAL_FOO,
>    TARGET_SIGNAL_BAR,
> ...
> };
>
> which represents GDB's internal signal numbers.AFAIK, the "target_" prefix
> naming in this case is just a natural choice, given that that's how we name
> everything behind the target_ops abstraction -- target_read_memory,
> target_resume, etc., etc. (target.h) -- the mechanism that is mostly about
> abstracting the details of handling the target's debug interface API.

As you say, these are GDB's internal values.  They are not the values used on
(some) targets.   They are not the values used by (some) host systems either.

There's a difference between target_read_memory() which actually reads a
value from the memory on the target, and target_signal which is not the
value from the target, but a translated internal value.

> So this explains the "target_signal_from_" part of the function's signature.

Not for me.  :-)

> The "from_host" part is really correct: this really is a host function -- think in
> terms of autoconf's notion of build vs host vs target.  It only makes sense to call
> it in native code (native == host).  And in fact, that's what happens.

GDB tends to muddle host and target.  Or perhaps the confusion is between the
host environment and GDB's internal data structures.

There is no need to translate signal numbers from the host system environment
to the target system environment.  There is a need to translate from GDB's internal
numbering to the target numbering, and vice versa.  There is nothing significant
about the host's signal numbering that should affect how GDB works with the target,
whether the target is a native process, remote system, or core file.

When target=host, translating from the target's signal numbering to GDB's
internal numbering should be identical to the case when target!=host.

target_signal_from_host() makes no sense to me.  I don't have a host core
file; it's a target core file.

> gdbserver is always native, so always host.

That's the exception.  But that should still use the target to/from internal
translation function.

> enum target_signal =>  enum gdb_signal
>
> target_signal_from_host =>  gdb_signal_from_host (or gdb_signal_from_host_signal)
> target_signal_to_host =>  gdb_signal_to_host (or gdb_signal_to_host_signal)
>
> gdbarch_target_signal_from_host =>  gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
> gdbarch_target_signal_to_host =>  gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)

OK, but I'd recommend
   target_signal_from_host =>  gdb_signal_from_target
   target_signal_to_host =>  gdb_signal_to_target

This is symmetric with the gdbarch_ functions and clear that the function
translates to/from target system values, not the host system.

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 15:40         ` MIPS Linux signals Michael Eager
@ 2012-05-22 16:02           ` Pedro Alves
  2012-05-22 18:14             ` Michael Eager
  0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 16:02 UTC (permalink / raw)
  To: Michael Eager; +Cc: Maciej W. Rozycki, gdb-patches

On 05/22/2012 04:40 PM, Michael Eager wrote:

> On 05/22/2012 03:17 AM, Pedro Alves wrote:
> 
>> I agree that the current naming is confusing, but I'll point out why I think
>> target_signal_from_host is actually correct, and then conclude with renaming
>> suggestions to avoid the confusion, and fix the naming of the gdbarch hook, which
>> I think is not correct.  We have
>>
>> enum target_signal
>> {
>> ...
>>    TARGET_SIGNAL_FOO,
>>    TARGET_SIGNAL_BAR,
>> ...
>> };
>>
>> which represents GDB's internal signal numbers.AFAIK, the "target_" prefix
>> naming in this case is just a natural choice, given that that's how we name
>> everything behind the target_ops abstraction -- target_read_memory,
>> target_resume, etc., etc. (target.h) -- the mechanism that is mostly about
>> abstracting the details of handling the target's debug interface API.
> 
> As you say, these are GDB's internal values.  They are not the values used on
> (some) targets.   They are not the values used by (some) host systems either.
> 
> There's a difference between target_read_memory() which actually reads a
> value from the memory on the target, and target_signal which is not the
> value from the target, but a translated internal value.
> 
>> So this explains the "target_signal_from_" part of the function's signature.
> 
> Not for me.  :-)


Could it be you mean "I agree, but I think the naming
of the enum could be clearer/better"?  The enum is called "enum target_signal".
So the function that converts an enum target_signal from something else
is called "target_signal_from_...".  That much seems clear enough to me.

> 
>> The "from_host" part is really correct: this really is a host function -- think in
>> terms of autoconf's notion of build vs host vs target.  It only makes sense to call
>> it in native code (native == host).  And in fact, that's what happens.
> 
> GDB tends to muddle host and target.  Or perhaps the confusion is between the
> host environment and GDB's internal data structures.


Not sure what you're alluding to.

> There is no need to translate signal numbers from the host system environment
> to the target system environment.  There is a need to translate from GDB's internal
> numbering to the target numbering, and vice versa.  There is nothing significant
> about the host's signal numbering that should affect how GDB works with the target,
> whether the target is a native process, remote system, or core file.


Correct.  Is that in disagreement with anything I said?  It's just that GDB's
internal signal numbering is called "enum target_signal".

> 
> When target=host, translating from the target's signal numbering to GDB's
> internal numbering should be identical to the case when target!=host.
> 
> target_signal_from_host() makes no sense to me.


It makes sense in the native/host-only parts of GDB.  linux-nat.c and friends, etc.

> I don't have a host core file; it's a target core file.

Yes, that's why in corelow.c the conversion goes through the gdbarch method.
corelow.c is not a native/host-only file (ideally anyway).  We have the
target_signal_from_host fallback for old ports that haven't yet bothered
to convert to modern mechanisms, and can't cross-debug cores.

> 
>> gdbserver is always native, so always host.
> 
> That's the exception.  


So is all the native-debugger code within GDB; code which we want
to actually split out of gdb and share/merge with gdbserver.

> But that should still use the target to/from internal

> translation function.


Correct.

> 
>> enum target_signal =>  enum gdb_signal
>>
>> target_signal_from_host =>  gdb_signal_from_host (or gdb_signal_from_host_signal)
>> target_signal_to_host =>  gdb_signal_to_host (or gdb_signal_to_host_signal)
>>
>> gdbarch_target_signal_from_host =>  gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
>> gdbarch_target_signal_to_host =>  gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)
> 
> OK, but I'd recommend
>   target_signal_from_host =>  gdb_signal_from_target
>   target_signal_to_host =>  gdb_signal_to_target
> 
> This is symmetric with the gdbarch_ functions and clear that the function
> translates to/from target system values, not the host system.


But it's not what the functions do...  They really convert from the host
system signals, not the target's.  I think the symmetry will only lead to
people getting confused (which one to call in common/target-independent code?).

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 10:17       ` Pedro Alves
                           ` (2 preceding siblings ...)
  2012-05-22 15:40         ` MIPS Linux signals Michael Eager
@ 2012-05-22 16:26         ` Pedro Alves
  3 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 16:26 UTC (permalink / raw)
  To: gdb-patches; +Cc: Maciej W. Rozycki, Michael Eager

On 05/22/2012 11:17 AM, Pedro Alves wrote:

> 
> enum target_signal => enum gdb_signal


http://sourceware.org/ml/gdb-patches/2012-05/msg00831.html

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 16:02           ` Pedro Alves
@ 2012-05-22 18:14             ` Michael Eager
  2012-05-22 18:31               ` Pedro Alves
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Eager @ 2012-05-22 18:14 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Maciej W. Rozycki, gdb-patches

On 05/22/2012 09:01 AM, Pedro Alves wrote:

>>> target_signal_from_host =>   gdb_signal_from_host (or gdb_signal_from_host_signal)
>>> target_signal_to_host =>   gdb_signal_to_host (or gdb_signal_to_host_signal)
>>>
>>> gdbarch_target_signal_from_host =>   gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
>>> gdbarch_target_signal_to_host =>   gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)
>>
>> OK, but I'd recommend
>>    target_signal_from_host =>   gdb_signal_from_target
>>    target_signal_to_host =>   gdb_signal_to_target
>>
>> This is symmetric with the gdbarch_ functions and clear that the function
>> translates to/from target system values, not the host system.
>
>
> But it's not what the functions do...  They really convert from the host
> system signals, not the target's.  I think the symmetry will only lead to
> people getting confused (which one to call in common/target-independent code?).

If you are running GDB on a Windows host, for example, what host
system signals are you translating?

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 18:14             ` Michael Eager
@ 2012-05-22 18:31               ` Pedro Alves
  2012-05-22 19:32                 ` Michael Eager
  0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 18:31 UTC (permalink / raw)
  To: Michael Eager; +Cc: Maciej W. Rozycki, gdb-patches

On 05/22/2012 07:14 PM, Michael Eager wrote:

> On 05/22/2012 09:01 AM, Pedro Alves wrote:
> 
>>>> target_signal_from_host =>   gdb_signal_from_host (or gdb_signal_from_host_signal)
>>>> target_signal_to_host =>   gdb_signal_to_host (or gdb_signal_to_host_signal)
>>>>
>>>> gdbarch_target_signal_from_host =>   gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
>>>> gdbarch_target_signal_to_host =>   gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)
>>>
>>> OK, but I'd recommend
>>>    target_signal_from_host =>   gdb_signal_from_target
>>>    target_signal_to_host =>   gdb_signal_to_target
>>>
>>> This is symmetric with the gdbarch_ functions and clear that the function
>>> translates to/from target system values, not the host system.
>>
>>


>> But it's not what the functions do...  They really convert from the host
>> system signals, not the target's.  I think the symmetry will only lead to
>> people getting confused (which one to call in common/target-independent code?).
> 
> If you are running GDB on a Windows host, for example, what host
> system signals are you translating?


There's only be one such call -- the one from within corelow.c, if there's no
gdbarch_target_signal_from_host callback installed.  (The Windows ports don't call
target_signal_from_host anywhere (windows-nat.c and friends).  And in that case, if
you e.g., load a cygwin core, the target_signal_from_host fallback will try to
convert the signal number as if it was a host signal number.  If you're running g
b on a cygwin host, you'll happen to get the right values.  If you're debugging
a core (that same core or of some other non-native arch) with a cross debugger, with
a mingw hosted gdb, then target_signal_from_host will _still_ translate the signal
numbers found in mingw's signal.h header.  For reference, those are:

#define SIGINT          2       /* Interactive attention */
#define SIGILL          4       /* Illegal instruction */
#define SIGFPE          8       /* Floating point error */
#define SIGSEGV         11      /* Segmentation violation */
#define SIGTERM         15      /* Termination request */
#define SIGBREAK        21      /* Control-break */
#define SIGABRT         22      /* Abnormal termination (abort) */

All other signal numbers you pass to target_signal_from_host will end up
as TARGET_SIGNAL_UNKNOWN, due to the bunch of #ifdef SIGFOO bits in
common/signals.  Obviously, in most cases, this translation will
be wrong.  But the point to be taken is, target_signal_from_host _always_
translates the signal number passed as argument as if it was a host
signal number, no matter what the target really is.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 10:58       ` Pedro Alves
@ 2012-05-22 19:31         ` Aleksandar Ristovski
  2012-05-22 21:55           ` Pedro Alves
  0 siblings, 1 reply; 29+ messages in thread
From: Aleksandar Ristovski @ 2012-05-22 19:31 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Maciej W. Rozycki, Michael Eager, gdb-patches

On 12-05-22 06:57 AM, Pedro Alves wrote:
> [Adding Aleksandar]
>
> Answering this one.
>
> On 05/21/2012 11:48 PM, Maciej W. Rozycki wrote:
>
>>   Shall we drop the unused gdbarch API so as to avoid further confusion
>> then?
>
>
> If we don't find a use for it, yes.
>
> Aleksandar, we're discussing gdbarch_target_signal_from_host and
> gdbarch_target_signal_to_host.  It turns out that uses to either of those
> were never added to GDB.  gdbarch_target_signal_FROM_host's purpose is clear,
> and we're about to add a (new) use to fix the same situation you ran into at the
> time (cross core debugging).  I'm wondering if you ever found a use for
> gdbarch_target_signal_TO_host that we should consider, though.
>

The API was added to introduce consistency between gdb's view of 
target's numeric signal values and actual numerical signal values of the 
target. In general case, they should *not* be viewed as the same, but 
rather as distinct numeric sets which happen to have common names. When 
cross-examining a core this becomes very obvious, but it is also very 
obvious when debugging remote target which has different numerical 
values for signals.

I use both from_host and to_host.


That being said, I'm not sure why I never submitted actual uses for nto 
target... I have it in our repository.


Looking at the code now, I see why. I use it in our remote target (we 
have our own) and thus perform translation on-the-fly. Gdb receives 
correct GDB version as well as target (when gdb sends it).



Thanks,

Aleksandar


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 18:31               ` Pedro Alves
@ 2012-05-22 19:32                 ` Michael Eager
  2012-05-22 22:06                   ` Pedro Alves
  0 siblings, 1 reply; 29+ messages in thread
From: Michael Eager @ 2012-05-22 19:32 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Maciej W. Rozycki, gdb-patches

On 05/22/2012 11:30 AM, Pedro Alves wrote:
> On 05/22/2012 07:14 PM, Michael Eager wrote:
>
>> On 05/22/2012 09:01 AM, Pedro Alves wrote:
>>
>>>>> target_signal_from_host =>    gdb_signal_from_host (or gdb_signal_from_host_signal)
>>>>> target_signal_to_host =>    gdb_signal_to_host (or gdb_signal_to_host_signal)
>>>>>
>>>>> gdbarch_target_signal_from_host =>    gdbarch_gdb_signal_from_target (or gdbarch_gdb_signal_from_target_signal)
>>>>> gdbarch_target_signal_to_host =>    gdbarch_gdb_signal_to_target (or gdbarch_gdb_signal_to_target_signal)
>>>>
>>>> OK, but I'd recommend
>>>>     target_signal_from_host =>    gdb_signal_from_target
>>>>     target_signal_to_host =>    gdb_signal_to_target
>>>>
>>>> This is symmetric with the gdbarch_ functions and clear that the function
>>>> translates to/from target system values, not the host system.
>>>
>>>
>
>
>>> But it's not what the functions do...  They really convert from the host
>>> system signals, not the target's.  I think the symmetry will only lead to
>>> people getting confused (which one to call in common/target-independent code?).
>>
>> If you are running GDB on a Windows host, for example, what host
>> system signals are you translating?
>
>
> There's only be one such call -- the one from within corelow.c, if there's no
> gdbarch_target_signal_from_host callback installed.  (The Windows ports don't call
> target_signal_from_host anywhere (windows-nat.c and friends).  And in that case, if
> you e.g., load a cygwin core, the target_signal_from_host fallback will try to
> convert the signal number as if it was a host signal number.  If you're running g
> b on a cygwin host, you'll happen to get the right values.  If you're debugging
> a core (that same core or of some other non-native arch) with a cross debugger, with
> a mingw hosted gdb, then target_signal_from_host will _still_ translate the signal
> numbers found in mingw's signal.h header.  For reference, those are:
>
> #define SIGINT          2       /* Interactive attention */
> #define SIGILL          4       /* Illegal instruction */
> #define SIGFPE          8       /* Floating point error */
> #define SIGSEGV         11      /* Segmentation violation */
> #define SIGTERM         15      /* Termination request */
> #define SIGBREAK        21      /* Control-break */
> #define SIGABRT         22      /* Abnormal termination (abort) */
>
> All other signal numbers you pass to target_signal_from_host will end up
> as TARGET_SIGNAL_UNKNOWN, due to the bunch of #ifdef SIGFOO bits in
> common/signals.  Obviously, in most cases, this translation will
> be wrong.  But the point to be taken is, target_signal_from_host _always_
> translates the signal number passed as argument as if it was a host
> signal number, no matter what the target really is.

I think the point is that a target signal number should
always be treated as if it were from a target, never
that it was from the host.

All the #ifdefs checking if the host has this signal or that
is confusing.  The only thing that seems relevant to me is
whether the target has these signals.  If it happens that
target==host, then these happen to be equivalent.  But maintaining
a clear target abstraction is a benefit.


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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 19:31         ` Aleksandar Ristovski
@ 2012-05-22 21:55           ` Pedro Alves
  2012-05-22 23:29             ` Aleksandar Ristovski
  0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 21:55 UTC (permalink / raw)
  To: Aleksandar Ristovski; +Cc: Maciej W. Rozycki, Michael Eager, gdb-patches

On 05/22/2012 08:31 PM, Aleksandar Ristovski wrote:

> On 12-05-22 06:57 AM, Pedro Alves wrote:


>> Aleksandar, we're discussing gdbarch_target_signal_from_host and
>> gdbarch_target_signal_to_host.  It turns out that uses to either of those
>> were never added to GDB.  gdbarch_target_signal_FROM_host's purpose is clear,
>> and we're about to add a (new) use to fix the same situation you ran into at the
>> time (cross core debugging).  I'm wondering if you ever found a use for
>> gdbarch_target_signal_TO_host that we should consider, though.
>>
> 
> The API was added to introduce consistency between gdb's view of target's numeric signal values and actual

> numerical signal values of the target. In general case, they should *not* be viewed as the same, but rather
>  as distinct numeric sets which happen to have common names. When cross-examining a core this becomes very
> obvious, but it is also very obvious when debugging remote target which has different numerical values for signals.

> 
> I use both from_host and to_host.


I'm confused on the "when debugging remote target which has different numerical values for signals"
part, because the target is not supposed to send anything but the generic "enum target_signal" back to
GDB core.  The core should never need to do such translation with any target other than the
core target.

> That being said, I'm not sure why I never submitted actual uses for nto target... I have it in our repository.
> 
> 
> Looking at the code now, I see why. I use it in our remote target (we have our own) and thus perform translation on-the-fly. Gdb receives correct GDB version as well as target (when gdb sends it).


So it sounds like there's no real use for the gdbarch method in _common_ code then, right?  If
that's the case, we should zap it from the FSF tree until we find such a use.

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 19:32                 ` Michael Eager
@ 2012-05-22 22:06                   ` Pedro Alves
  0 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-22 22:06 UTC (permalink / raw)
  To: Michael Eager; +Cc: Maciej W. Rozycki, gdb-patches

On 05/22/2012 08:31 PM, Michael Eager wrote:

>>
>> All other signal numbers you pass to target_signal_from_host will end up
>> as TARGET_SIGNAL_UNKNOWN, due to the bunch of #ifdef SIGFOO bits in
>> common/signals.  Obviously, in most cases, this translation will
>> be wrong.  But the point to be taken is, target_signal_from_host _always_
>> translates the signal number passed as argument as if it was a host
>> signal number, no matter what the target really is.
> 
> I think the point is that a target signal number should
> always be treated as if it were from a target, never
> that it was from the host.


The discussion on the table is not about what should or not should
be design wise.  The discussion is about the confusing
target_signal_from_host method name.  I've given ample explanation of
why that should have "host" in its name.  If we should eliminate that
function and instead provide gdbarch mappings for all targets or not is
a different discussion.  The fact remains that target_signal_from_host
exists, and is widely used.  I'd very much like to retain "host" in its
name to remind the reader that while it exists, its implementation really
is just a mapping from the host signals.  I'm now pretty much done with
repeating the arguments.

> target signal number should always be treated as if it were from a target,

That requires GDB hardcoding signal mappings, and that's what the gdbarch
hook is for.  And that's why I proposed renaming it:

http://sourceware.org/ml/gdb-patches/2012-05/msg00839.html

-- 
Pedro Alves


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 21:55           ` Pedro Alves
@ 2012-05-22 23:29             ` Aleksandar Ristovski
  2012-05-23 11:39               ` Pedro Alves
  0 siblings, 1 reply; 29+ messages in thread
From: Aleksandar Ristovski @ 2012-05-22 23:29 UTC (permalink / raw)
  To: Pedro Alves; +Cc: Maciej W. Rozycki, Michael Eager, gdb-patches

On 12-05-22 05:55 PM, Pedro Alves wrote:
> On 05/22/2012 08:31 PM, Aleksandar Ristovski wrote:
>
>> On 12-05-22 06:57 AM, Pedro Alves wrote:
>
>
>>> Aleksandar, we're discussing gdbarch_target_signal_from_host and
>>> gdbarch_target_signal_to_host.  It turns out that uses to either of those
>>> were never added to GDB.  gdbarch_target_signal_FROM_host's purpose is clear,
>>> and we're about to add a (new) use to fix the same situation you ran into at the
>>> time (cross core debugging).  I'm wondering if you ever found a use for
>>> gdbarch_target_signal_TO_host that we should consider, though.
>>>
>>
>> The API was added to introduce consistency between gdb's view of target's numeric signal values and actual
>
>> numerical signal values of the target. In general case, they should *not* be viewed as the same, but rather
>>   as distinct numeric sets which happen to have common names. When cross-examining a core this becomes very
>> obvious, but it is also very obvious when debugging remote target which has different numerical values for signals.
>
>>
>> I use both from_host and to_host.
>
>
> I'm confused on the "when debugging remote target which has different numerical values for signals"
> part, because the target is not supposed to send anything but the generic "enum target_signal" back to
> GDB core.  The core should never need to do such translation with any target other than the
> core target.

In my case, translation happens in remote-nto.c which is our remote 
target. (our functional equivalent of gdbserver does not really know 
about gdb's enum target_signal).


>
>> That being said, I'm not sure why I never submitted actual uses for nto target... I have it in our repository.
>>
>>
>> Looking at the code now, I see why. I use it in our remote target (we have our own) and thus perform translation on-the-fly. Gdb receives correct GDB version as well as target (when gdb sends it).
>
>
> So it sounds like there's no real use for the gdbarch method in _common_ code then, right?  If
> that's the case, we should zap it from the FSF tree until we find such a use.
>

Fine by me.

(FWIW, I like renaming enum target_signal to gdb_signal; it will clear 
up some confusion).


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: MIPS Linux signals
  2012-05-22 23:29             ` Aleksandar Ristovski
@ 2012-05-23 11:39               ` Pedro Alves
  0 siblings, 0 replies; 29+ messages in thread
From: Pedro Alves @ 2012-05-23 11:39 UTC (permalink / raw)
  To: Aleksandar Ristovski; +Cc: Maciej W. Rozycki, Michael Eager, gdb-patches

On 05/23/2012 12:28 AM, Aleksandar Ristovski wrote:

>>
>> So it sounds like there's no real use for the gdbarch method in _common_ code then, right?  If
>> that's the case, we should zap it from the FSF tree until we find such a use.
>>
> Fine by me.


Thanks.  Applied.

2012-05-23  Pedro Alves  <palves@redhat.com>

	* arch-utils.h (default_target_signal_to_host): Delete.
	* arch-utils.c (default_target_signal_to_host): Delete.
	* gdbarch.sh (target_signal_to_host): Remove.
	* gdbarch.h, gdbarch.c: Regenerate.
---
 gdb/arch-utils.c |    6 ------
 gdb/arch-utils.h |    2 --
 gdb/gdbarch.c    |   24 ------------------------
 gdb/gdbarch.h    |    7 -------
 gdb/gdbarch.sh   |    3 ---
 5 files changed, 0 insertions(+), 42 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index fabb515..c6866a9 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -793,12 +793,6 @@ default_gen_return_address (struct gdbarch *gdbarch,
   error (_("This architecture has no method to collect a return address."));
 }

-int
-default_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal ts)
-{
-  return target_signal_to_host (ts);
-}
-
 enum target_signal
 default_target_signal_from_host (struct gdbarch *gdbarch, int signo)
 {
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index c2c3398..8f47635 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -174,7 +174,5 @@ extern const char *default_auto_wide_charset (void);

 extern enum target_signal default_target_signal_from_host (struct gdbarch *,
 							   int);
-extern int default_target_signal_to_host (struct gdbarch *,
-					  enum target_signal);

 #endif
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index af2033b..8d009f3 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -261,7 +261,6 @@ struct gdbarch
   gdbarch_process_record_ftype *process_record;
   gdbarch_process_record_signal_ftype *process_record_signal;
   gdbarch_target_signal_from_host_ftype *target_signal_from_host;
-  gdbarch_target_signal_to_host_ftype *target_signal_to_host;
   gdbarch_get_siginfo_type_ftype *get_siginfo_type;
   gdbarch_record_special_symbol_ftype *record_special_symbol;
   gdbarch_get_syscall_number_ftype *get_syscall_number;
@@ -429,7 +428,6 @@ struct gdbarch startup_gdbarch =
   0,  /* process_record */
   0,  /* process_record_signal */
   default_target_signal_from_host,  /* target_signal_from_host */
-  default_target_signal_to_host,  /* target_signal_to_host */
   0,  /* get_siginfo_type */
   0,  /* record_special_symbol */
   0,  /* get_syscall_number */
@@ -539,7 +537,6 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->displaced_step_location = NULL;
   gdbarch->relocate_instruction = NULL;
   gdbarch->target_signal_from_host = default_target_signal_from_host;
-  gdbarch->target_signal_to_host = default_target_signal_to_host;
   gdbarch->has_shared_address_space = default_has_shared_address_space;
   gdbarch->fast_tracepoint_valid_at = default_fast_tracepoint_valid_at;
   gdbarch->auto_charset = default_auto_charset;
@@ -731,7 +728,6 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of process_record, has predicate.  */
   /* Skip verify of process_record_signal, has predicate.  */
   /* Skip verify of target_signal_from_host, invalid_p == 0 */
-  /* Skip verify of target_signal_to_host, invalid_p == 0 */
   /* Skip verify of get_siginfo_type, has predicate.  */
   /* Skip verify of record_special_symbol, has predicate.  */
   /* Skip verify of get_syscall_number, has predicate.  */
@@ -1345,9 +1341,6 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
                       "gdbarch_dump: target_signal_from_host = <%s>\n",
                       host_address_to_string (gdbarch->target_signal_from_host));
   fprintf_unfiltered (file,
-                      "gdbarch_dump: target_signal_to_host = <%s>\n",
-                      host_address_to_string (gdbarch->target_signal_to_host));
-  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_unwind_pc_p() = %d\n",
                       gdbarch_unwind_pc_p (gdbarch));
   fprintf_unfiltered (file,
@@ -3812,23 +3805,6 @@ set_gdbarch_target_signal_from_host (struct gdbarch *gdbarch,
 }

 int
-gdbarch_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal ts)
-{
-  gdb_assert (gdbarch != NULL);
-  gdb_assert (gdbarch->target_signal_to_host != NULL);
-  if (gdbarch_debug >= 2)
-    fprintf_unfiltered (gdb_stdlog, "gdbarch_target_signal_to_host called\n");
-  return gdbarch->target_signal_to_host (gdbarch, ts);
-}
-
-void
-set_gdbarch_target_signal_to_host (struct gdbarch *gdbarch,
-                                   gdbarch_target_signal_to_host_ftype target_signal_to_host)
-{
-  gdbarch->target_signal_to_host = target_signal_to_host;
-}
-
-int
 gdbarch_get_siginfo_type_p (struct gdbarch *gdbarch)
 {
   gdb_assert (gdbarch != NULL);
diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index 5bc4f4d..4388a7d 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -946,13 +946,6 @@ typedef enum target_signal (gdbarch_target_signal_from_host_ftype) (struct gdbar
 extern enum target_signal gdbarch_target_signal_from_host (struct gdbarch *gdbarch, int signo);
 extern void set_gdbarch_target_signal_from_host (struct gdbarch *gdbarch, gdbarch_target_signal_from_host_ftype *target_signal_from_host);

-/* Signal translation: translate GDB's signal number into inferior's host
-   signal number. */
-
-typedef int (gdbarch_target_signal_to_host_ftype) (struct gdbarch *gdbarch, enum target_signal ts);
-extern int gdbarch_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal ts);
-extern void set_gdbarch_target_signal_to_host (struct gdbarch *gdbarch, gdbarch_target_signal_to_host_ftype *target_signal_to_host);
-
 /* Extra signal info inspection.

    Return a type suitable to inspect extra signal information. */
diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
index 3e3b126..9394677 100755
--- a/gdb/gdbarch.sh
+++ b/gdb/gdbarch.sh
@@ -775,9 +775,6 @@ M:int:process_record_signal:struct regcache *regcache, enum target_signal signal
 # Signal translation: translate inferior's signal (host's) number into
 # GDB's representation.
 m:enum target_signal:target_signal_from_host:int signo:signo::default_target_signal_from_host::0
-# Signal translation: translate GDB's signal number into inferior's host
-# signal number.
-m:int:target_signal_to_host:enum target_signal ts:ts::default_target_signal_to_host::0

 # Extra signal info inspection.
 #


^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2012-05-23 11:39 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-20  2:03 MIPS Linux signals Michael Eager
2012-05-21 11:04 ` Pedro Alves
2012-05-21 14:51   ` Michael Eager
2012-05-21 17:37     ` Pedro Alves
2012-05-21 18:06       ` Michael Eager
2012-05-21 18:19 ` Maciej W. Rozycki
     [not found] ` <alpine.DEB.1.10.1205211232260.11227@tp.orcam.me.uk>
2012-05-21 18:21   ` Michael Eager
2012-05-21 22:34     ` Maciej W. Rozycki
2012-05-22  9:38       ` Pedro Alves
2012-05-21 21:35 ` Pedro Alves
2012-05-21 21:53   ` Michael Eager
2012-05-21 22:48     ` Maciej W. Rozycki
2012-05-22  0:16       ` Michael Eager
2012-05-22 10:17       ` Pedro Alves
2012-05-22 13:16         ` Maciej W. Rozycki
2012-05-22 13:32           ` Pedro Alves
2012-05-22 15:10         ` Move store_waitstatus to inf-child.c (was: Re: MIPS Linux signals) Pedro Alves
2012-05-22 15:40         ` MIPS Linux signals Michael Eager
2012-05-22 16:02           ` Pedro Alves
2012-05-22 18:14             ` Michael Eager
2012-05-22 18:31               ` Pedro Alves
2012-05-22 19:32                 ` Michael Eager
2012-05-22 22:06                   ` Pedro Alves
2012-05-22 16:26         ` Pedro Alves
2012-05-22 10:58       ` Pedro Alves
2012-05-22 19:31         ` Aleksandar Ristovski
2012-05-22 21:55           ` Pedro Alves
2012-05-22 23:29             ` Aleksandar Ristovski
2012-05-23 11:39               ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox