Re: [RFC/PATCH] Add new internal variable $_signo

[Sergio Durigan Junior <sergiodj@redhat.com>]

Hi Pierre,

Thanks for the review.

On Friday, June 14 2013, Pierre Muller wrote:

>   Is it that I didn't understand the patch correctly or
> do you use the GDB signal number in infrun.c
> while you use the native signal integer value in the
> corelow.c case?

Yes, you are right.

>   Aren't those two values sometimes different?

They probably are in some cases.

> Wouldn't it be more consistent to only use the GDB internal number?

Hm, now that you raised the question, I am wondering.  I believe it is
more consistent to use the GDB internal number when we are printing
something, yeah.

However, in the $_signo case, we are actually displaying the number
itself, so your comment applies to my patch, but backwards: I should
actually be converting the GDB internal number to the actual signal
number on infrun.c.

>   In fact, this "inconsistency" is not specific to your patch,
> the siggy from corelow.c is printed out, while other signals are always 
> first converted to GDB enum values before being printed (and apparently not
> in
> integer form but using the gdb_signal_to_name function.
>
>   Shouldn't we use gdb_signal_to_name (sig) in core_open
> and set $_signo also to sig?

I don't think $_signo should be set to "sig", it should remain "siggy".
What should happen (IIUC everything) is that the infrun.c uses should be
converted to the actual signal number (by using gdb_signal_to_host).

> Proposed patch (untested...)
> Should I submit it independently or 
> is there a specific reason to print the numeric value of the signal 
> for core dumps while we seem to use signal names elsewhere?

I always think that the numeric value of a signal is a good thing to be
printed.

> 2013-06-14  Pierre Muller  <muller@sourceware.org>
>
>         * corelow.c (core_open): Use GDB signal name instead of raw
>         signal value.
>
> Index: corelow.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/corelow.c,v
> retrieving revision 1.132
> diff -u -p -r1.132 corelow.c
> --- corelow.c   15 May 2013 12:26:14 -0000      1.132
> +++ corelow.c   14 Jun 2013 08:56:08 -0000
> @@ -445,7 +445,7 @@ core_open (char *filename, int from_tty)
>                              : gdb_signal_from_host (siggy));
>
>        printf_filtered (_("Program terminated with signal %s, %s.\n"),

Could you print the number here?  BTW, the number is "siggy" :-).  You
could say:

  printf_filtered (_("Program terminated with signal %s (%d), %s.\n"),

BTW, this patch is wrong for me, the previous printf_filtered line was
"%d, %s".

> -                      siggy, gdb_signal_to_string (sig));
> +                      gdb_signal_to_name (sig), gdb_signal_to_string
> (sig));
>      }
>
>    /* Fetch all registers from core file.  */

I will send a new patch addressing the issues I mentioned above shortly.

Thanks,

-- 
Sergio