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 v3] Implement $_exitsignal


On Monday, September 16 2013, Pedro Alves wrote:

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

No problem.

> 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.

OK, will do that.

>> @@ -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 (*).

What do you mean by "always exporting the GDB internal number"?  Always
printing this internal number to the user, without converting it first?

> 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.

OK, makes sense to me.  Then my opinion is that we should somehow not
support filling $_exitsignal if there is no
gdbarch_gdb_signal_to_target, i.e., we should clear the convenience
variable and leave it alone.  I don't like the idea of exposing GDB's
internal number because there may/will be discrepancies already
discussed (unless that's not what you meant above).

> (*) - 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.  */

FWIW I don't agree with this comment.  It's like if we had an internal
syscall numbering system and required the user to know this "magical"
number.  It shouldn't be like that.  Unless we provided a way for the
user to get a list of "signal names -> internal signal numbers", but
even that would not be ideal because the user might only have the signal
number in hands.  Anyway...

And thanks for bringing this up!

> 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...

I'd say "file a PR?".

Thanks for your review, I will post a new version of the patch with your
modifications + Doug's requests.

-- 
Sergio


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