From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8478 invoked by alias); 6 Sep 2010 11:29:49 -0000 Received: (qmail 8448 invoked by uid 22791); 6 Sep 2010 11:29:47 -0000 X-SWARE-Spam-Status: No, hits=-2.0 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 06 Sep 2010 11:29:12 +0000 Received: (qmail 4100 invoked from network); 6 Sep 2010 11:29:10 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 6 Sep 2010 11:29:10 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Re: [patch] target_signal obvious fixups Date: Mon, 06 Sep 2010 13:35:00 -0000 User-Agent: KMail/1.13.2 (Linux/2.6.33-29-realtime; KDE/4.4.2; x86_64; ; ) Cc: Jan Kratochvil References: <20100904215812.GA9740@host1.dyn.jankratochvil.net> In-Reply-To: <20100904215812.GA9740@host1.dyn.jankratochvil.net> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201009061229.09019.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-09/txt/msg00153.txt.bz2 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 > > * corelow.c (core_open): Do not print signal name without > CORE_GDBARCH. > * fork-child.c (startup_inferior) : 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) : 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 > > * server.c (handle_v_cont) : Use enum target_signal type. > * target.c (mywait) : 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