Re: [PATCH v3] Implement $_exitsignal

[Pedro Alves <palves at redhat dot com>]

Sorry for the late reply, I was away last week.

On 08/25/2013 10:25 PM, Sergio Durigan Junior wrote:

> @@ -446,6 +446,16 @@ core_open (char *filename, int from_tty)
>  
>        printf_filtered (_("Program terminated with signal %s, %s.\n"),
>  		       gdb_signal_to_name (sig), gdb_signal_to_string (sig));
> +
> +      /* Set the value of the internal variable $_exitsignal,
> +	 which holds the signal uncaught by the inferior.  */
> +      set_internalvar_integer (lookup_internalvar ("_exitsignal"),
> +			       siggy);
> +
> +      /* Clear the $_exitcode internal variable, because if the
> +	 inferior died with a signal then its return code does not
> +	 exist.  */
> +      clear_internalvar (lookup_internalvar ("_exitcode"));
>      }

$_exitcode and $_exitsignal should probably be cleared even
if siggy == 0.



> @@ -3428,6 +3428,10 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	  set_internalvar_integer (lookup_internalvar ("_exitcode"),
>  				   (LONGEST) ecs->ws.value.integer);
>  
> +	  /* Clear the $_exitsignal internal variable, since if we are
> +	     here the inferior has not been terminated by a signal.  */
> +	  clear_internalvar (lookup_internalvar ("_exitsignal"));
> +
>  	  /* Also record this in the inferior itself.  */
>  	  current_inferior ()->has_exit_code = 1;
>  	  current_inferior ()->exit_code = (LONGEST) ecs->ws.value.integer;
> @@ -3435,7 +3439,43 @@ handle_inferior_event (struct execution_control_state *ecs)
>  	  print_exited_reason (ecs->ws.value.integer);
>  	}
>        else
> -	print_signal_exited_reason (ecs->ws.value.sig);
> +	{
> +	  LONGEST signo;
> +	  struct regcache *regcache = get_thread_regcache (ecs->ptid);
> +	  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +
> +	  if (gdbarch_gdb_signal_to_target_p (gdbarch))
> +	    signo = (LONGEST) gdbarch_gdb_signal_to_target (gdbarch,
> +							    ecs->ws.value.sig);
> +	  else if (gdb_signal_to_host_p (ecs->ws.value.sig))
> +	    {
> +	      /* This is a workaround.  If we don't have a way to a
> +		 way of converting a signal using the target's
> +		 notation (which is the best), then we at least offer
> +		 the conversion using the host's notation.  This will
> +		 be OK as long as the user is not doing remote
> +		 debugging with target != host.  */
> +	      signo = (LONGEST) gdb_signal_to_host (ecs->ws.value.sig);
> +	    }
> +	  else
> +	    {
> +	      /* This is *ugly*, but if we can't even rely on the host
> +		 converstion, then the least we can do is to print
> +		 GDB's internal signal number.  */
> +	      signo = (LONGEST) ecs->ws.value.sig;
> +	    }

Urgh.  I'm really not sure this fallbacks are a good idea.
The last one in particular looks very nasty.  The old code in corelow.c
that falls back to a host conversion is there because it's very
old code that existed way before gdb even gained proper support for
cross debugging.  I think for new functionality we should pick one of either
just not supporting the feature if we can't do a proper
gdbarch_gdb_signal_to_target conversion, or always exporting the GDB internal
number (*).  A mixed solution like that just doesn't look right to me.  The
new tests will happen to pass for most targets/hosts, because the values of
common signals like SIGABRT and SIGSEGV will happen to coincide with gdb's
own numbers.

(*) - I now happened to notice this in the "handle" command's
implementation:

  else if (digits > 0)
	{
	  /* It is numeric.  The numeric signal refers to our own
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	     internal signal numbering from target.h, not to host/target
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	     signal  number.  This is a feature; users really should be
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	     using symbolic names anyway, and the common ones like
	     SIGHUP, SIGINT, SIGALRM, etc. will work right anyway.  */


IOW, with the target conversion, "handle $_exitsignal" ("handle"
doesn't accept convenience vars today, but it reasonably could),
won't do what a user could reasonably expect.  Argh. :-/  Oh well...

-- 
Pedro Alves