This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] target_signal obvious fixups


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]