Re: MIPS Linux signals

[Pedro Alves <palves at redhat dot com>]

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