[RFC/PATCH] Add new internal variable $_signo

[RFC/PATCH] Add new internal variable $_signo

[Sergio Durigan Junior]

Hi,

This patch comes from a request at:

<https://bugzilla.redhat.com/show_bug.cgi?id=971849>

Basically, the ABRT project (<https://fedorahosted.org/abrt/wiki>) wants
to be able to perform some analysis on corefiles (to be implemented as a
Python GDB module) and for that it needs to be able to inspect the
signal which killed the program being investigated.

This can be done with recent Linux kernels by inspecting the $_siginfo
convenience variable (on corefiles, it works by parsing the contents of
the NT_SIGINFO section, whose support was added by Tom on
1b05b77b857f26c59ad5dc6443fc8baa21696440).  The NT_SIGINFO section was
added on the kernel by:

  author   Denys Vlasenko <vda.linux@googlemail.com>      2012-10-05 00:15:35 (GMT)
  commit         49ae4d4b113be03dc4a2ec5f2a1f573ff0fcddb3 (patch)

(This is Linux 3.7-rc1).

Well, the problem is that for older kernels (or not so old, as can be
noted by the date), when one tries to access $_siginfo.si_signo from a
corefile he/she gets:

  (gdb) core ./coredump
  [New LWP 2703]
  Core was generated by `/usr/bin/gnote <<skip>>'.
  Program terminated with signal 11, Segmentation fault.
  #0  0x09fa5348 in ?? ()
  (gdb) print $_siginfo.si_signo
  Unable to read siginfo

The signal can obviously be recovered because GDB itself mentioned it
when saying that it was "signal 11, Segmentation fault", and this is why
this patch came to life.  It basically sets/creates a new internal
variable called "$_signo" to be used as an alternative to
$_siginfo.si_signo when this one is unavailable.  It's not a complex
patch per se, but I would certainly like some review because there may
be other places where we should set the variable as well.

The patch also contains a testcase and an update to the documentation in
order to mention the new convenience variable.

Comments?  OK to apply?

-- 
Sergio

gdb/ChangeLog:
2013-06-13  Denys Vlasenko  <dvlasenk@redhat.com>

	* corelow.c (core_open): Set internal variable "$_signo".
	* infrun.c (handle_inferior_event): Likewise, for
	TARGET_WAITKIND_SIGNALLED and TARGET_WAITKIND_STOPPED.

gdb/testsuite/ChangeLog:
2013-06-13  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/signo.c: New file.
	* gdb.base/signo.exp: Likewise.

gdb/doc/ChangeLog:
2013-06-13  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.texinfo (Convenience Variables): Document "$_signo".

diff --git a/gdb/corelow.c b/gdb/corelow.c
index 0bfa743..6044b35 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -446,6 +446,10 @@ core_open (char *filename, int from_tty)
 
       printf_filtered (_("Program terminated with signal %d, %s.\n"),
 		       siggy, gdb_signal_to_string (sig));
+      /* Set the internal variable $_signo to hold the signal number that
+	 triggered the creation of the corefile.  */
+      set_internalvar_integer (lookup_internalvar ("_signo"),
+			       (LONGEST) siggy);
     }
 
   /* Fetch all registers from core file.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e6ec4ff..0eda8bc 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -9759,6 +9759,16 @@ The variable @code{$_siginfo} contains extra signal information
 could be empty, if the application has not yet received any signals.
 For example, it will be empty before you execute the @code{run} command.
 
+@item $_signo
+@vindex $_signo@r{, convenience variable}
+The variable @code{$_signo} contains the signal number received by the
+inferior.  Its purpose is to serve as an auxiliary way of retrieving
+such information for when the kernel does not provide the necessary
+support for @code{$_siginfo} to be used.  Note that @code{$_signo}
+could be @code{void} (empty), if the application has not received any
+signals.  For example, it will be @code{void} before you execute the
+@code{run} command.
+
 @item $_tlb
 @vindex $_tlb@r{, convenience variable}
 The variable @code{$_tlb} is automatically set when debugging
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 51a032b..ac1ca9d 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -3462,7 +3462,13 @@ handle_inferior_event (struct execution_control_state *ecs)
 	  print_exited_reason (ecs->ws.value.integer);
 	}
       else
-	print_signal_exited_reason (ecs->ws.value.sig);
+	{
+	  print_signal_exited_reason (ecs->ws.value.sig);
+	  /* Set the value of the internal variable $_signo to hold the
+	     signal number received by the inferior.  */
+	  set_internalvar_integer (lookup_internalvar ("_signo"),
+				   (LONGEST) ecs->ws.value.sig);
+	}
 
       gdb_flush (gdb_stdout);
       target_mourn_inferior ();
@@ -3719,6 +3725,10 @@ handle_inferior_event (struct execution_control_state *ecs)
       if (debug_infrun)
         fprintf_unfiltered (gdb_stdlog, "infrun: TARGET_WAITKIND_STOPPED\n");
       ecs->event_thread->suspend.stop_signal = ecs->ws.value.sig;
+      /* Set the value of the internal variable $_signo to hold the signal
+	 number that stopped the inferior.  */
+      set_internalvar_integer (lookup_internalvar ("_signo"),
+			       (LONGEST) ecs->ws.value.sig);
       break;
 
     case TARGET_WAITKIND_NO_HISTORY:
diff --git a/gdb/testsuite/gdb.base/signo.c b/gdb/testsuite/gdb.base/signo.c
new file mode 100644
index 0000000..9e2db13
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signo.c
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+*/
+
+int
+main (int argc, char *argv[])
+{
+  char *p = 0;
+
+  /* Generating a SIGSEGV.  */
+  *p = 1;
+}
diff --git a/gdb/testsuite/gdb.base/signo.exp b/gdb/testsuite/gdb.base/signo.exp
new file mode 100644
index 0000000..e75bcf1
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signo.exp
@@ -0,0 +1,45 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    return -1
+}
+
+# Run to main
+if { ![runto_main] } {
+    return -1
+}
+
+# Run to the SIGSEGV.
+gdb_test "continue" ".*Program received signal SIGSEGV.*" "continue to SIGSEGV"
+
+# Try to generate a core file, for a later test.
+set gcorefile [standard_output_file $testfile.gcore]
+set gcore_created [gdb_gcore_cmd $gcorefile "save a core file"]
+
+if { !${gcore_created} } {
+    untested "unable to create corefile."
+    return
+}
+
+# Restart all over, but this time with the corefile.
+clean_restart $binfile
+
+gdb_test "core $gcorefile" "Core was generated by.*" \
+    "core [file tail $gcorefile]"
+
+gdb_test "p \$_signo" " = 11"

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

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu, 13 Jun 2013 21:37:59 -0300
> 
> Comments?  OK to apply?

Thanks.  The documentation part is OK, but I think we want a NEWS
entry for this as well.

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

[Sergio Durigan Junior]

On Friday, June 14 2013, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Thu, 13 Jun 2013 21:37:59 -0300
>> 
>> Comments?  OK to apply?
>
> Thanks.  The documentation part is OK, but I think we want a NEWS
> entry for this as well.

Thanks for the review, Eli.  I will update the documentation with
Pedro's comments, and will also add a NEWS entry in the next iteration.

-- 
Sergio

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

[Mark Kettenis]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu, 13 Jun 2013 21:37:59 -0300
> 
> Hi,
> 
> This patch comes from a request at:
> 
> <https://bugzilla.redhat.com/show_bug.cgi?id=971849>
> 
> Basically, the ABRT project (<https://fedorahosted.org/abrt/wiki>) wants
> to be able to perform some analysis on corefiles (to be implemented as a
> Python GDB module) and for that it needs to be able to inspect the
> signal which killed the program being investigated.
> 
> This can be done with recent Linux kernels by inspecting the $_siginfo
> convenience variable (on corefiles, it works by parsing the contents of
> the NT_SIGINFO section, whose support was added by Tom on
> 1b05b77b857f26c59ad5dc6443fc8baa21696440).  The NT_SIGINFO section was
> added on the kernel by:
> 
>   author   Denys Vlasenko <vda.linux@googlemail.com>      2012-10-05 00:15:35 (GMT)
>   commit         49ae4d4b113be03dc4a2ec5f2a1f573ff0fcddb3 (patch)
> 
> (This is Linux 3.7-rc1).
> 
> Well, the problem is that for older kernels (or not so old, as can be
> noted by the date), when one tries to access $_siginfo.si_signo from a
> corefile he/she gets:
> 
>   (gdb) core ./coredump
>   [New LWP 2703]
>   Core was generated by `/usr/bin/gnote <<skip>>'.
>   Program terminated with signal 11, Segmentation fault.
>   #0  0x09fa5348 in ?? ()
>   (gdb) print $_siginfo.si_signo
>   Unable to read siginfo
> 
> The signal can obviously be recovered because GDB itself mentioned it
> when saying that it was "signal 11, Segmentation fault", and this is why
> this patch came to life.  It basically sets/creates a new internal
> variable called "$_signo" to be used as an alternative to
> $_siginfo.si_signo when this one is unavailable.  It's not a complex
> patch per se, but I would certainly like some review because there may
> be other places where we should set the variable as well.
> 
> The patch also contains a testcase and an update to the documentation in
> order to mention the new convenience variable.
> 
> Comments?  OK to apply?

Sounds reasonable to me.  Implementation seems sound.

> gdb/ChangeLog:
> 2013-06-13  Denys Vlasenko  <dvlasenk@redhat.com>
> 
> 	* corelow.c (core_open): Set internal variable "$_signo".
> 	* infrun.c (handle_inferior_event): Likewise, for
> 	TARGET_WAITKIND_SIGNALLED and TARGET_WAITKIND_STOPPED.
> 
> gdb/testsuite/ChangeLog:
> 2013-06-13  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gdb.base/signo.c: New file.
> 	* gdb.base/signo.exp: Likewise.
> 
> gdb/doc/ChangeLog:
> 2013-06-13  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gdb.texinfo (Convenience Variables): Document "$_signo".

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

[Pierre Muller]

  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?

  
  Aren't those two values sometimes different?
Wouldn't it be more consistent to only use the GDB internal number?

  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?

Pierre

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?


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"),
-                      siggy, gdb_signal_to_string (sig));
+                      gdb_signal_to_name (sig), gdb_signal_to_string
(sig));
     }

   /* Fetch all registers from core file.  */

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

[Sergio Durigan Junior]

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

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

[Pedro Alves]

On 06/14/2013 06:58 PM, Sergio Durigan Junior wrote:
> 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).

gdb_signal_to_host is the fallback (and having a fallback is sort of a
hack).  The right signal number is the target's not the host's.  We
have gdbarch_gdb_signal_from_target for the opposite direction, but not
gdbarch_gdb_signal_to_target...  Having to bake the target OS's signal
numbers into GDB is a bit unfortunate, though we could get around it
at some point if we wanted by extending the RSP, and/or adding a python
hook.

-- 
Pedro Alves

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

[Sergio Durigan Junior]

On Friday, June 14 2013, Pedro Alves wrote:

> gdb_signal_to_host is the fallback (and having a fallback is sort of a
> hack).  The right signal number is the target's not the host's.  We
> have gdbarch_gdb_signal_from_target for the opposite direction, but not
> gdbarch_gdb_signal_to_target...  Having to bake the target OS's signal
> numbers into GDB is a bit unfortunate, though we could get around it
> at some point if we wanted by extending the RSP, and/or adding a python
> hook.

I was looking for a target variant but found none.  I'm not sure what
you mean with your comment.  Are you suggesting that I take care of this
as well?

In the meantime, I will leave the code as-is, i.e., without doing any
kind of conversion on infrun.c

-- 
Sergio

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

[Pedro Alves]

On 06/14/2013 09:35 PM, Sergio Durigan Junior wrote:
> On Friday, June 14 2013, Pedro Alves wrote:
> 
>> gdb_signal_to_host is the fallback (and having a fallback is sort of a
>> hack).  The right signal number is the target's not the host's.  We
>> have gdbarch_gdb_signal_from_target for the opposite direction, but not
>> gdbarch_gdb_signal_to_target...  Having to bake the target OS's signal
>> numbers into GDB is a bit unfortunate, though we could get around it
>> at some point if we wanted by extending the RSP, and/or adding a python
>> hook.
> 
> I was looking for a target variant but found none.  I'm not sure what
> you mean with your comment.  

The right signal conversion is gdb signal <-> target signal.
gdb_signal_to_HOST only works for native debugging, because in that
case target == host.  See in corelow.c:

      /* If we don't have a CORE_GDBARCH to work with, assume a native
	 core (map gdb_signal from host signals).  If we do have
	 CORE_GDBARCH to work with, but no gdb_signal_from_target
	 implementation for that gdbarch, as a fallback measure,
	 assume the host signal mapping.  It'll be correct for native
	 cores, but most likely incorrect for cross-cores.  */
      enum gdb_signal sig = (core_gdbarch != NULL
			     && gdbarch_gdb_signal_from_target_p (core_gdbarch)
			     ? gdbarch_gdb_signal_from_target (core_gdbarch,
							       siggy)
			     : gdb_signal_from_host (siggy));


The same comment applies for reverse conversions (gdb -> target).  We
just don't do any in GDB at present.  These new convenience variables
would be the first such uses.

> Are you suggesting that I take care of this as well?

I'm suggesting that using gdb_signal_to_host only does the
right thing with native debugging, and that the proper conversion
involves adding a gdbarch_gdb_signal_to_target hook.  Given it doesn't
exist, it needs to be written...

(This actually negates my previous comment on $_signo existing
for all targets...)

Then I mumbled that this target signal -> gdb signal -> target signal
conversions are unfortunate, and that we could consider bypassing
them, by either (or both) extending the target interfaces to pass
along the original target signal (the signal numbers that pass around
in the remote protocol are GDB signal numbers, not the target signals),
and/or adding a python hook to aid in the conversion (so support
for random embedded RTOSs would be added without changing gdb).

> In the meantime, I will leave the code as-is, i.e., without doing any
> kind of conversion on infrun.c

-- 
Pedro Alves

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

[Pedro Alves]

On 06/14/2013 01:37 AM, Sergio Durigan Junior wrote:

> The signal can obviously be recovered because GDB itself mentioned it
> when saying that it was "signal 11, Segmentation fault", and this is why
> this patch came to life.  It basically sets/creates a new internal
> variable called "$_signo" to be used as an alternative to
> $_siginfo.si_signo when this one is unavailable.  It's not a complex
> patch per se, but I would certainly like some review because there may
> be other places where we should set the variable as well.

$_siginfo always prints the siginfo of the selected thread.  If you
change threads, you'll print the newly selected thread's siginfo.

From the way $_signo described in terms of $_siginfo, and is
implemented in the core target, it should always print the signal
of the last event of the selected thread.  However, the implementation
for live targets is doing something else:

On 06/14/2013 01:37 AM, Sergio Durigan Junior wrote:
> -	print_signal_exited_reason (ecs->ws.value.sig);
> +	{
> +	  print_signal_exited_reason (ecs->ws.value.sig);
> +	  /* Set the value of the internal variable $_signo to hold the
> +	     signal number received by the inferior.  */
> +	  set_internalvar_integer (lookup_internalvar ("_signo"),
> +				   (LONGEST) ecs->ws.value.sig);
> +	}

This bit is actually implementing a counterpart of the existing
$_exitcode.  At this point the inferior is gone already,
and $_siginfo couldn't ever work, even if the backend supports it.

This case is actually a different scenario from $_siginfo.  I think
it'd make more sense to call this one $_exitsignal, as sibling
of the existing $_exitcode.  $_exitcode records the inferior's exit
code for normal exits, $_exitsignal would record the signal number
for signal exits.  Like $_exitcode, $_exitsignal should print the
same signal number until the inferior exits again, no matter which
thread you're looking at.  Looked like that, $_exitsignal just
seems like an obvious omission.

Since when debugging a core file you're seeing a snapshot of the
inferior _before_ it exited, I think pedantically it makes more
sense for corelow to _not_ set $_exitsignal.  However, should
still be able to get the info from the $_signo of the thread that
exited (the one that gets selected when you load the core).

So in sum, here are my suggestions:

Add $_signo, but make it print the signal of the last event the
thread received.  I'd add a last_status field in struct thread_info
to record that info.  Don't implement $_signo as a regular
integer convenience variable that gets set whenever a thread
reports an event.  That won't work correctly in non-stop mode,
where you can be looking at thread #1, while other threads
could be hitting events (but the user remains with thread #1
selected).  See how the $_thread variable is implemented as lazy
convenience, very similar to what you need.

If you're willing and I think it'd be a nice addition too,
add $_exitsignal, and make it print the uncaught signal that made
the process die.  Document it with something like:

@item $_exitcode
@vindex $_exitcode@r{, convenience variable}
The variable @code{$_exitcode} is automatically set to the exit code when
-the program being debugged terminates.
+the program being debugged terminates normally.
+
+@item $_exitsignal
+@vindex $_exitsignal@r{, convenience variable}
+The variable @code{$_exitsignal} is automatically set to the
+signal number when the program being debugged dies due to an
+uncaught signal.

I suggest making $_signo and $_exitsignal separate patches,
even.

BTW, IMO, $_exitcode (and $_exitsignal) should print the exit
code/signal of the selected inferior.  It was never adjusted
for multi-inferior, so it always prints the exit code of the
inferior that happened to exit last.  Simon's patch for
displaying the inferior's exit-code in MI's -list-thread-groups records
the info in the inferior struct.  Once that is in, we'd just need to
make $_exitcode a lazy convenience var too.

> The patch also contains a testcase and an update to the documentation in
> order to mention the new convenience variable.

The patch adds code to handle signalled live inferiors, but the
test didn't exercise that.

> +@item $_signo
> +@vindex $_signo@r{, convenience variable}
> +The variable @code{$_signo} contains the signal number received by the
> +inferior.

> +Its purpose is to serve as an auxiliary way of retrieving
> +such information for when the kernel does not provide the necessary
> +support for @code{$_siginfo} to be used.

IMO, this whole sentence would be better reduced to: "Unlike $_siginfo
below, $_signo is always available in all supported targets."

$_signo should work on Windows, for example, where there's no
concept of $_siginfo.

> Note that @code{$_signo}
> +could be @code{void} (empty), if the application has not received any
> +signals.  For example, it will be @code{void} before you execute the
> +@code{run} command.

-- 
Pedro Alves

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

[Sergio Durigan Junior]

On Friday, June 14 2013, Pedro Alves wrote:

> On 06/14/2013 01:37 AM, Sergio Durigan Junior wrote:
>> -	print_signal_exited_reason (ecs->ws.value.sig);
>> +	{
>> +	  print_signal_exited_reason (ecs->ws.value.sig);
>> +	  /* Set the value of the internal variable $_signo to hold the
>> +	     signal number received by the inferior.  */
>> +	  set_internalvar_integer (lookup_internalvar ("_signo"),
>> +				   (LONGEST) ecs->ws.value.sig);
>> +	}
>
> This bit is actually implementing a counterpart of the existing
> $_exitcode.  At this point the inferior is gone already,
> and $_siginfo couldn't ever work, even if the backend supports it.
>
> This case is actually a different scenario from $_siginfo.  I think
> it'd make more sense to call this one $_exitsignal, as sibling
> of the existing $_exitcode.  $_exitcode records the inferior's exit
> code for normal exits, $_exitsignal would record the signal number
> for signal exits.  Like $_exitcode, $_exitsignal should print the
> same signal number until the inferior exits again, no matter which
> thread you're looking at.  Looked like that, $_exitsignal just
> seems like an obvious omission.

OK, I have a patch for $_exitcode now, will send soon.  I've made some
assumptions, will explain them when I send the patch.

> Since when debugging a core file you're seeing a snapshot of the
> inferior _before_ it exited, I think pedantically it makes more
> sense for corelow to _not_ set $_exitsignal.  However, should
> still be able to get the info from the $_signo of the thread that
> exited (the one that gets selected when you load the core).

Yes, given the separation you proposed, I agree.

> So in sum, here are my suggestions:
>
> Add $_signo, but make it print the signal of the last event the
> thread received.  I'd add a last_status field in struct thread_info
> to record that info.  Don't implement $_signo as a regular
> integer convenience variable that gets set whenever a thread
> reports an event.  That won't work correctly in non-stop mode,
> where you can be looking at thread #1, while other threads
> could be hitting events (but the user remains with thread #1
> selected).  See how the $_thread variable is implemented as lazy
> convenience, very similar to what you need.

Thanks, I will work on it this Sunday, hope to have something work soon.

> If you're willing and I think it'd be a nice addition too,
> add $_exitsignal, and make it print the uncaught signal that made
> the process die.  Document it with something like:
>
> @item $_exitcode
> @vindex $_exitcode@r{, convenience variable}
> The variable @code{$_exitcode} is automatically set to the exit code when
> -the program being debugged terminates.
> +the program being debugged terminates normally.
> +
> +@item $_exitsignal
> +@vindex $_exitsignal@r{, convenience variable}
> +The variable @code{$_exitsignal} is automatically set to the
> +signal number when the program being debugged dies due to an
> +uncaught signal.
>
> I suggest making $_signo and $_exitsignal separate patches,
> even.

Yeah, a patch's coming.

> BTW, IMO, $_exitcode (and $_exitsignal) should print the exit
> code/signal of the selected inferior.  It was never adjusted
> for multi-inferior, so it always prints the exit code of the
> inferior that happened to exit last.  Simon's patch for
> displaying the inferior's exit-code in MI's -list-thread-groups records
> the info in the inferior struct.  Once that is in, we'd just need to
> make $_exitcode a lazy convenience var too.

OK, this paragraph mades sense but made me feel a bit confused.  Anyway,
I will send the $_exitsignal patch implemented as $_exitcode is
implemented now, and if you feel it should be different, you can comment
there.

>> The patch also contains a testcase and an update to the documentation in
>> order to mention the new convenience variable.
>
> The patch adds code to handle signalled live inferiors, but the
> test didn't exercise that.

I will add that in the next iteration.

>> +@item $_signo
>> +@vindex $_signo@r{, convenience variable}
>> +The variable @code{$_signo} contains the signal number received by the
>> +inferior.
>
>> +Its purpose is to serve as an auxiliary way of retrieving
>> +such information for when the kernel does not provide the necessary
>> +support for @code{$_siginfo} to be used.
>
> IMO, this whole sentence would be better reduced to: "Unlike $_siginfo
> below, $_signo is always available in all supported targets."
>
> $_signo should work on Windows, for example, where there's no
> concept of $_siginfo.

OK, fair enough.  I will update the docs as well.

Thanks,

-- 
Sergio

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

[Sergio Durigan Junior]

On Sunday, June 16 2013, I wrote:

> OK, I have a patch for $_exitcode now, will send soon.  I've made some
> assumptions, will explain them when I send the patch.

s/exitcode/exitsignal/

-- 
Sergio

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

[Pedro Alves]

On 06/16/2013 06:56 AM, Sergio Durigan Junior wrote:
> 
>> > BTW, IMO, $_exitcode (and $_exitsignal) should print the exit
>> > code/signal of the selected inferior.  It was never adjusted
>> > for multi-inferior, so it always prints the exit code of the
>> > inferior that happened to exit last.  Simon's patch for
>> > displaying the inferior's exit-code in MI's -list-thread-groups records
>> > the info in the inferior struct.  Once that is in, we'd just need to
>> > make $_exitcode a lazy convenience var too.
> OK, this paragraph mades sense but made me feel a bit confused.

Ok, that clarify.  Say:

- you're running 100 inferiors under GDB in non-stop mode.
- they're all running (c -a&) while you're inspecting, say
  thread #3 of inferior #1.
- inferiors #4, #65 and #87 exit, with exit codes 10, 20, 30,
  respectively.
- '$_exitcode' is left set to '30', the exit code of inferior #87:

 (gdb) p $_exitcode
 $1 = 30
 (gdb) inferior 4
 (gdb) p $_exitcode
 $2 = 30
 (gdb) inferior 65
 (gdb) p $_exitcode
 $3 = 30
 (gdb) inferior 87
 (gdb) p $_exitcode
 $4 = 30

Meaning, $_exitcode is useless when you consider async debugging,
as it may be overwritten at any point in time.

I was saying that IMO it'd make more sense that GDB behaved like
this instead:

 (gdb) p $_exitcode
 $1 = void
 (gdb) inferior 4
 (gdb) p $_exitcode
 $2 = 10
 (gdb) inferior 65
 (gdb) p $_exitcode
 $3 = 20
 (gdb) inferior 87
 (gdb) p $_exitcode
 $4 = 30
 (gdb) inferior 4
 (gdb) p $_exitcode
 $5 = 10

And that means making the $_exitcode convenience var a lazy
convenience var that prints 'struct inferior'::exit_code,
instead of a regular convenience var that gets set whenever
an inferior exits.

-- 
Pedro Alves

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

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Basically, the ABRT project (<https://fedorahosted.org/abrt/wiki>) wants
Sergio> to be able to perform some analysis on corefiles (to be implemented as a
Sergio> Python GDB module) and for that it needs to be able to inspect the
Sergio> signal which killed the program being investigated.

I read through this thread.  It was very informative.

If you are not planning to tackle all the items Pedro pointed out, would
you mind filing bugs for the ones you are not planning to do?

Sergio>   (gdb) core ./coredump
Sergio>   [New LWP 2703]
Sergio>   Core was generated by `/usr/bin/gnote <<skip>>'.
Sergio>   Program terminated with signal 11, Segmentation fault.
Sergio>   #0  0x09fa5348 in ?? ()
Sergio>   (gdb) print $_siginfo.si_signo
Sergio>   Unable to read siginfo

One other idea that occurs to me is that, in this situation, $_siginfo
could yield a value where only the si_signo bits are available.  We've
already got all the machinery needed to make this work.

TBH I'm not sure if this idea is clever and simple, or obscure and hard
to use.

The one major benefit, I think, would be that scripts could simply use
$_siginfo.si_signo unconditionally.

Tom