This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] target_signal obvious fixups
- From: Pedro Alves <pedro at codesourcery dot com>
- To: gdb-patches at sourceware dot org
- Cc: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Date: Mon, 6 Sep 2010 12:29:08 +0100
- Subject: Re: [patch] target_signal obvious fixups
- References: <20100904215812.GA9740@host1.dyn.jankratochvil.net>
On Saturday 04 September 2010 22:58:12, Jan Kratochvil wrote:
> Hi,
>
> as the siginfo series got complicated here is at least the obvious fixup part
> to be useful independently on the other patches.
>
> It is a part of:
> [patch 3/9]#2 Change target_signal_t to a struct
> http://sourceware.org/ml/gdb-patches/2010-08/msg00483.html
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.
>
> Not checked in but it seems mostly obvious.
Comments inline below.
> gdb/
> 2010-09-04 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * corelow.c (core_open): Do not print signal name without
> CORE_GDBARCH.
> * fork-child.c (startup_inferior) <resume_signal>: Use enum
> target_signal type.
> * linux-nat.c (linux_nat_resume): Replace strsignal by
> target_signal_to_name. Use enum target_signal type for saved_signo.
> (linux_handle_extended_wait) <signo>: Use enum target_signal type.
> (linux_nat_wait_1): Use enum target_signal type for signo. Replace
> strsignal by target_signal_to_name.
> * remote-m32r-sdi.c (m32r_wait, m32r_detach): Replace 0 by
> TARGET_SIGNAL_0.
>
> gdb/gdbserver/
> 2010-09-04 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * server.c (handle_v_cont) <sig>: Use enum target_signal type.
> * target.c (mywait) <TARGET_WAITKIND_EXITED>: Fix to use INTEGER.
>
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -437,7 +437,7 @@ core_open (char *filename, int from_tty)
> target_signal_to_string (
> (core_gdbarch != NULL) ?
> gdbarch_target_signal_from_host (core_gdbarch, siggy)
> - : siggy));
> + : TARGET_SIGNAL_0));
Something like this would be better:
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));
I had something similar in my patch, but I see I typod there and
wrote target_signal_from_number instead of target_signal_from_host.
> /* Fetch all registers from core file. */
> target_fetch_registers (get_current_regcache (), -1);
> --- a/gdb/fork-child.c
> +++ b/gdb/fork-child.c
> @@ -448,7 +448,7 @@ startup_inferior (int ntraps)
>
> while (1)
> {
> - int resume_signal = TARGET_SIGNAL_0;
> + enum target_signal resume_signal = TARGET_SIGNAL_0;
> ptid_t event_ptid;
>
Okay.
> struct target_waitstatus ws;
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1874,7 +1874,7 @@ linux_nat_resume (struct target_ops *ops,
> "LLR: Preparing to %s %s, %s, inferior_ptid %s\n",
> step ? "step" : "resume",
> target_pid_to_str (ptid),
> - signo ? strsignal (signo) : "0",
> + target_signal_to_name (signo),
> target_pid_to_str (inferior_ptid));
(...)
> block_child_signals (&prev_mask);
> @@ -1907,7 +1907,7 @@ linux_nat_resume (struct target_ops *ops,
>
> if (lp->status && WIFSTOPPED (lp->status))
> {
> - int saved_signo;
> + enum target_signal saved_signo;
> struct inferior *inf;
>
> inf = find_inferior_pid (ptid_get_pid (lp->ptid));
> @@ -1974,7 +1974,7 @@ linux_nat_resume (struct target_ops *ops,
> "LLR: %s %s, %s (resume event thread)\n",
> step ? "PTRACE_SINGLESTEP" : "PTRACE_CONT",
> target_pid_to_str (ptid),
> - signo ? strsignal (signo) : "0");
> + target_signal_to_name (signo));
>
I think it'd be better to keep using strsignal, so we may easily see
when the target_signal translates to something bizarre that we're about
to send to ptrace/tkill/kill. (This is at the native level, so it goes
along with the PTRACE_SINGLESTEP|CONT.) Agree? See my patch.
> restore_child_signals_mask (&prev_mask);
> if (target_can_async_p ())
> @@ -2266,7 +2266,7 @@ linux_handle_extended_wait (struct lwp_info *lp, int status,
> catchpoints. */
> if (!stopping)
> {
> - int signo;
> + enum target_signal signo;
>
Okay.
> new_lp->stopped = 0;
> new_lp->resumed = 1;
> @@ -3567,7 +3567,7 @@ retry:
>
> if (WIFSTOPPED (status))
> {
> - int signo = target_signal_from_host (WSTOPSIG (status));
> + enum target_signal signo = target_signal_from_host (WSTOPSIG (status));
> struct inferior *inf;
>
Okay.
> inf = find_inferior_pid (ptid_get_pid (lp->ptid));
> @@ -3597,7 +3597,7 @@ retry:
> lp->step ?
> "PTRACE_SINGLESTEP" : "PTRACE_CONT",
> target_pid_to_str (lp->ptid),
> - signo ? strsignal (signo) : "0");
> + target_signal_to_name (signo));
Same as above.
> lp->stopped = 0;
> goto retry;
> }
> --- a/gdb/remote-m32r-sdi.c
> +++ b/gdb/remote-m32r-sdi.c
> @@ -715,7 +715,7 @@ m32r_wait (struct target_ops *ops,
> fprintf_unfiltered (gdb_stdlog, "m32r_wait()\n");
>
> status->kind = TARGET_WAITKIND_EXITED;
> - status->value.sig = 0;
> + status->value.sig = TARGET_SIGNAL_0;
Okay.
>
> interrupted = 0;
> prev_sigint = signal (SIGINT, gdb_cntrl_c);
> @@ -886,7 +886,7 @@ m32r_detach (struct target_ops *ops, char *args, int from_tty)
> if (remote_debug)
> fprintf_unfiltered (gdb_stdlog, "m32r_detach(%d)\n", from_tty);
>
> - m32r_resume (ops, inferior_ptid, 0, 0);
> + m32r_resume (ops, inferior_ptid, 0, TARGET_SIGNAL_0);
Okay.
>
> /* calls m32r_close to do the real work */
> pop_target ();
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -1748,7 +1748,8 @@ handle_v_cont (char *own_buf)
>
> if (p[0] == 'S' || p[0] == 'C')
> {
> - int sig;
> + enum target_signal sig;
> +
> sig = strtol (p + 1, &q, 16);
This is okay with me, but I'd like to point out that
this makes one use of sig correct (a bit below this code,
not quoted), while making the "sig = strtol" assignment
assume you can convert a long to an enum / target_signal.
IMO, this is a case where target_signal_from_number would
make sense (I had one in my patch, not sure you had one
in yours). Let me repeat this is okay with me as is...
> if (p == q)
> goto err;
> --- a/gdb/gdbserver/target.c
> +++ b/gdb/gdbserver/target.c
> @@ -98,7 +98,7 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
>
> if (ourstatus->kind == TARGET_WAITKIND_EXITED)
> fprintf (stderr,
> - "\nChild exited with status %d\n", ourstatus->value.sig);
> + "\nChild exited with status %d\n", ourstatus->value.integer);
Okay.
> else if (ourstatus->kind == TARGET_WAITKIND_SIGNALLED)
> fprintf (stderr, "\nChild terminated with signal = 0x%x (%s)\n",
> target_signal_to_host (ourstatus->value.sig),
>
--
Pedro Alves