This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3] Implement $_exitsignal
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: GDB Patches <gdb-patches at sourceware dot org>, Tom Tromey <tromey at redhat dot com>
- Date: Mon, 16 Sep 2013 21:19:26 -0300
- Subject: Re: [PATCH v3] Implement $_exitsignal
- Authentication-results: sourceware.org; auth=none
- References: <m3d2p1xy2w dot fsf at redhat dot com> <52375342 dot 6080902 at redhat dot com>
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