Re: [patch 1/9]#2 Rename `enum target_signal' to target_signal_t

Jan Kratochvil
2010-09-02T19:33:58Z

On Thu, 02 Sep 2010 19:27:52 +0200, Pedro Alves wrote:
> I don't think it is a good idea to associate the siginfo with a signal
> and think of it as a single entity. For many cases, you don't need the
> siginfo. Carrying it around would be extra burden, especially for the remote
> target.

For the remote part I was thinking about making it lazy, requesting it only
when retrieved for a different kind of processing than a target resume.

When we forget for a moment about GDB: From the OS point of view int SIG* and
siginfo_t are associated. Why should their GDB model differ?

> > and TARGET_SIGNAL_SIGTRAP should be called
> > TARGET_SIGNAL_BREAKPOINT_HIT etc.
>
> I'm not sure that would be a good idea either. That's not a
> signal in the "unix sense". A breakpoint hit is a higher
> level event than a SIGTRAP.

Yes, it is a higher level event. Then why infrun.c has conditions like:

/* Make sure we were stopped at a breakpoint. */
if (wait_status.kind != TARGET_WAITKIND_STOPPED
|| (wait_status.value.sig != TARGET_SIGNAL_TRAP
&& wait_status.value.sig != TARGET_SIGNAL_ILL
&& wait_status.value.sig != TARGET_SIGNAL_SEGV
&& wait_status.value.sig != TARGET_SIGNAL_EMT))

http://sourceware.org/ml/gdb-patches/2010-01/msg00619.html
+ if (ecs->ws.kind == TARGET_WAITKIND_STOPPED
+ && (ecs->ws.value.sig == TARGET_SIGNAL_ILL
+ || ecs->ws.value.sig == TARGET_SIGNAL_SEGV
+ || ecs->ws.value.sig == TARGET_SIGNAL_EMT)
+ && breakpoint_inserted_here_p (get_regcache_aspace (regcache),
+ regcache_read_pc (regcache)))
+ {
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: Treating signal as SIGTRAP\n");
+ ecs->ws.value.sig = TARGET_SIGNAL_TRAP;

here TARGET_SIGNAL_TRAP is considered to represent a breakpoint hit.

(I do not say it is a wrong patch, it IMO fits the current GDB architecture,
just the current GDB architecture is not right. Also I do not say it is
a blocker for any bug, just that it is internally not right.)

> In any case, for a new breakpoint hit waitkind
> (TARGET_WAITKIND_BREAKPOINT_HIT or something.

OK, thanks, I wasn't so TARGET_WAITKIND_* aware.

> At the remote protocol level, you'd probably have the remote stub transfer
> something like "T05 bkpt", rather than a stop reply with the whole siginfo.
> Consider the remote protocol and software single-step targets. You don't
> want to have to transfer the whole siginfo blob back and forth for each
> single-step.

I believe it is right to assume the signal should have the siginfo associated.
Then we can implement various accelerations for special cases when it does not
have to be transferred.

Currently if the GDB middle-end (infrun.c) would want to save a signal and
restore it later, it cannot. So here is a testcase:
FAIL: FSF GDB linux-nat
FAIL: FSF GDB gdbserver
PASS: FSF GDB linux-nat with my siginfo_t patchset

I understand you can save and restore TARGET_OBJECT_SIGNAL_INFO along
tp->stop_signal in save_inferior_thread_state and
restore_inferior_thread_state. But is it really the right way when we talk
about the proper design? While it is IMO clearly wrong design it still may be
pragmatically the most practical way to go, though.

> In any case, even if we got rid of the gdb-signal <-> host-signals mapping
> completely (which would be underirable as it would mean we'd need to teach
> gdb about any random OS we want to use remote debugging with),

OK, good point... Still the protocol should then transfer TARGET_WAITKIND_*
rather than TARGET_SIGNAL_*.

> embedding more info _within_ a simple
> signal id appears wrong to me. And that's what the whole
> series was about, and I think we're in agreement on that now.

I may possibly agree it is enough for the current GDB as-is.
I would not agree for it on an academic discussion how it should be properly
designed.

> If you get rid of the gdb generic
> signals from infrun.c completely, some things might become
> weird if you don't have such a mechanism, like "signal FOO" sending
> one signal when native debugging, and another when remote debugging.

linux-nat should have its own signal list plus linux-nat should not exist.

Thanks,
Jan

gdb/testsuite/
2010-09-02 Jan Kratochvil <jan.kratochvil@redhat.com>

* gdb.base/siginfo-infcall.exp: New file.
* gdb.base/siginfo-infcall.c: New file.

--- /dev/null
+++ b/gdb/testsuite/gdb.base/siginfo-infcall.c
@@ -0,0 +1,68 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2010 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include <signal.h>
+#include <assert.h>
+#include <string.h>
+
+#ifndef SA_SIGINFO
+# error "SA_SIGINFO is required for this test"
+#endif
+
+static int
+callme (void)
+{
+ return 42;
+}
+
+static volatile int pass, fail;
+
+static void
+handler (int sig, siginfo_t *siginfo, void *context)
+{
+ assert (sig == SIGUSR1);
+ assert (siginfo->si_signo == SIGUSR1);
+ if (siginfo->si_pid == getpid ())
+ pass = 1;
+ else
+ fail = 1;
+}
+
+int
+main (void)
+{
+ struct sigaction sa;
+ int i;
+
+ callme ();
+
+ memset (&sa, 0, sizeof (sa));
+ sa.sa_sigaction = handler;
+ sa.sa_flags = SA_SIGINFO;
+
+ i = sigemptyset (&sa.sa_mask);
+ assert (i == 0);
+
+ i = sigaction (SIGUSR1, &sa, NULL);
+ assert (i == 0);
+
+ i = raise (SIGUSR1);
+ assert (i == 0);
+
+ sleep (600);
+ return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/siginfo-infcall.exp
@@ -0,0 +1,47 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+if [target_info exists gdb,nosignals] {
+ verbose "Skipping siginfo-infcall.exp because of nosignals."
+ continue
+}
+
+set testfile siginfo-infcall
+set srcfile ${testfile}.c
+set executable ${testfile}
+if { [prepare_for_testing ${testfile}.exp $executable] } {
+ return -1
+}
+
+if ![runto_main] {
+ return -1
+}
+
+gdb_breakpoint [gdb_get_line_number "pass = 1;"]
+gdb_breakpoint [gdb_get_line_number "fail = 1;"]
+
+gdb_test "continue" "Program received signal SIGUSR1, .*" "continue to SIGUSR1"
+
+gdb_test "p callme ()" " = 42"
+
+set test "continue to the handler"
+gdb_test_multiple "continue" $test {
+ -re "pass = 1;\r\n$gdb_prompt $" {
+ pass $test
+ }
+ -re "fail = 1;\r\n$gdb_prompt $" {
+ fail $test
+ }
+}