Re: MIPS Linux signals

Pedro Alves
2012-05-22T10:17:43Z

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