Fix 308341 vgdb should report process exit (or fatal signal)

patch from Mark Wielaard.
(with small modifications).
Also clarified some comments related to the resume reply.




git-svn-id: svn://svn.valgrind.org/valgrind/trunk@13052

NEWS

@@ -33,6 +33,7 @@ m = merged into 3_8_BRANCH
 306054    [390] s390x: Condition code computation for convert-to-int/logical
 307155    [390] filter_gdb should filter out syscall-template.S T_PSEUDO
 308321    [390] testsuite memcheck filter interferes with gdb_filter 
+308341    [390] vgdb should report process exit (or fatal signal)
 n-i-bz    [390] report error for vgdb snapshot requested before execution
 n-i-bz    [390] Some wrong command line options could be ignored
 

coregrind/m_gdbserver/m_gdbserver.c

@@ -33,6 +33,8 @@
 #include "pub_core_libcproc.h"
 #include "pub_core_libcprint.h"
 #include "pub_core_mallocfree.h"
+#include "pub_tool_libcsetjmp.h"
+#include "pub_core_threadstate.h"
 #include "pub_core_gdbserver.h"
 #include "pub_core_options.h"
 #include "pub_core_libcsetjmp.h"
@@ -68,7 +70,8 @@ typedef
       core_reason,    // gdbserver invocation by core (e.g. error encountered)
       break_reason,   // break encountered
       watch_reason,   // watchpoint detected by tool
-      signal_reason}  // signal encountered
+      signal_reason,  // signal encountered
+      exit_reason}    // process terminated
     CallReason;
 
 static char* ppCallReason(CallReason reason)
@@ -80,6 +83,7 @@ static char* ppCallReason(CallReason reason)
    case break_reason:   return "break_reason";
    case watch_reason:   return "watch_reason";
    case signal_reason:  return "signal_reason";
+   case exit_reason:    return "exit_reason";
    default: vg_assert (0);
    }
 }
@@ -641,6 +645,14 @@ static void call_gdbserver ( ThreadId tid , CallReason reason)
         VG_(getpid) (), tid, VG_(name_of_ThreadStatus)(tst->status),
         tst->sched_jmpbuf_valid);
 
+   /* If we are about to die, then just run server_main() once to get
+      the resume reply out and return immediately because most of the state
+      of this tid and process is about to be torn down. */
+   if (reason == exit_reason) {
+      server_main();
+      return;
+   }
+
    vg_assert(VG_(is_valid_tid)(tid));
    saved_pc = VG_(get_IP) (tid);
 
@@ -933,6 +945,34 @@ Bool VG_(gdbserver_report_signal) (Int vki_sigNo, ThreadId tid)
    }
 }
 
+void VG_(gdbserver_exit) (ThreadId tid, VgSchedReturnCode tids_schedretcode)
+{
+   dlog(1, "VG core calling VG_(gdbserver_exit) tid %d will exit\n", tid);
+   if (remote_connected()) {
+      /* Make sure vgdb knows we are about to die and why. */
+      switch(tids_schedretcode) {
+      case VgSrc_None:
+         vg_assert (0);
+      case VgSrc_ExitThread:
+      case VgSrc_ExitProcess:
+         gdbserver_process_exit_encountered ('W', VG_(threads)[tid].os_state.exitcode);
+         call_gdbserver (tid, exit_reason);
+         break;
+      case VgSrc_FatalSig:
+         gdbserver_process_exit_encountered ('X', VG_(threads)[tid].os_state.fatalsig);
+         call_gdbserver (tid, exit_reason);
+         break;
+      default:
+         vg_assert(0);
+      }
+   } else {
+      dlog(1, "not connected\n");
+   }
+
+   /* Tear down the connection if it still exists. */
+   VG_(gdbserver) (0);
+}
+
 // Check if single_stepping or if there is a break requested at iaddr. 
 // If yes, call debugger
 VG_REGPARM(1)

coregrind/m_gdbserver/regcache.h

@@ -1,5 +1,5 @@
 /* Register support routines for the remote server for GDB.
-   Copyright (C) 2001, 2002 Free Software Foundation, Inc.
+   Copyright (C) 2001, 2002, 2012 Free Software Foundation, Inc.
 
    This file is part of GDB.
    It has been modified to integrate it in valgrind

coregrind/m_gdbserver/regdef.h

@@ -1,5 +1,5 @@
 /* Register protocol definition structures for the GNU Debugger
-   Copyright 2001, 2002 Free Software Foundation, Inc.
+   Copyright 2001, 2002, 2012 Free Software Foundation, Inc.
 
    This file is part of GDB.
    It has been modified to integrate it in valgrind

coregrind/m_gdbserver/server.c

@@ -765,6 +765,13 @@ void server_main (void)
          putpkt (own_buf);
       }
 
+      /* If we our status is terminal (exit or fatal signal) get out
+         as quickly as we can. We won't be able to handle any request
+         anymore.  */
+      if (status == 'W' || status == 'X') {
+         return;
+      }
+
       packet_len = getpkt (own_buf);
       if (packet_len <= 0)
          break;

coregrind/m_gdbserver/server.h

@@ -1,6 +1,6 @@
 /* Common definitions for remote server for GDB.
    Copyright (C) 1993, 1995, 1997, 1998, 1999, 2000, 2002, 2003, 2004, 2005,
-   2006
+   2006, 2012
    Free Software Foundation, Inc.
 
    This file is part of GDB.
@@ -40,9 +40,9 @@
 #include "pub_tool_libcassert.h"
 #include "pub_tool_libcbase.h"
 #include "pub_tool_options.h"
-#include "pub_core_gdbserver.h"
 #include "pub_tool_libcsetjmp.h"
 #include "pub_core_threadstate.h"
+#include "pub_core_gdbserver.h"
 #include "pub_core_aspacemgr.h"
 #include "pub_tool_vki.h"
 #include "valgrind.h"
@@ -198,16 +198,24 @@ struct thread_info;
 #include "gdb/signals.h"
 
 /* signal handling with gdbserver: before delivering a signal,
-   call gdbserver_signal_encountered then give control to
-   gdbserver by calling call_gdbserver.
-   On return, call gdbserver_deliver_signal to effectively
-   deliver the signal or not. */
+   call gdbserver_signal_encountered. This will set
+   the signal to report in the next resume reply sent to GDB.
+   A call to call_gdbserver is needed to send the resume reply to GDB.
+   After this call, gdbserver_deliver_signal indicates if the signal
+   is effectively to be delivered to the guest process. */
 extern void gdbserver_signal_encountered (Int vki_sigNo);
 /* between these two calls, call call_gdbserver */
 /* If gdbserver_deliver_signal True, then gdb did not ask
    to ignore the signal, so signal can be delivered to the guest. */
 extern Bool gdbserver_deliver_signal (Int vki_sigNo);
 
+/* Called when a process is about to go with reason ('W' or 'X') and code.
+   This sets global variables that will be used to return the process
+   exit status to GDB in the next resume_reply.
+   Similarly to gdbserver_signal_encountered, a call to call_gdbserver
+   is needed to send the resume reply. */
+extern void gdbserver_process_exit_encountered (unsigned char status, Int code);
+
 /* To optimise signal handling, gdb can instruct gdbserver to
    not stop on some signals. In the below, a 1 indicates the gdb_nr signal
    has to be passed directly to the guest, without asking gdb.

coregrind/m_gdbserver/target.c

@@ -165,6 +165,15 @@ Bool gdbserver_deliver_signal (Int vki_sigNo)
    return vki_sigNo == vki_signal_to_deliver;
 }
 
+static unsigned char exit_status_to_report;
+static int exit_code_to_report;
+void gdbserver_process_exit_encountered (unsigned char status, Int code)
+{
+   vg_assert (status == 'W' || status == 'X');
+   exit_status_to_report = status;
+   exit_code_to_report = code;
+}
+
 static
 char* sym (Addr addr)
 {
@@ -248,6 +257,7 @@ unsigned char valgrind_wait (char *ourstatus)
    unsigned long wptid;
    ThreadState *tst;
    enum target_signal sig;
+   int code;
 
    pid = VG_(getpid) ();
    dlog(1, "enter valgrind_wait pid %d\n", pid);
@@ -255,6 +265,26 @@ unsigned char valgrind_wait (char *ourstatus)
    regcache_invalidate();
    valgrind_update_threads(pid);
 
+   /* First see if we are done with this process. */
+   if (exit_status_to_report != 0) {
+      *ourstatus = exit_status_to_report;
+      exit_status_to_report = 0;
+
+      if (*ourstatus == 'W') {
+         code = exit_code_to_report;
+         exit_code_to_report = 0;
+         dlog(1, "exit valgrind_wait status W exit code %d\n", code);
+         return code;
+      }
+
+      if (*ourstatus == 'X') {
+         sig = target_signal_from_host(exit_code_to_report);
+         exit_code_to_report = 0;
+         dlog(1, "exit valgrind_wait status X signal %d\n", sig);
+         return sig;
+      }
+   }
+
    /* in valgrind, we consider that a wait always succeeds with STOPPED 'T' 
       and with a signal TRAP (i.e. a breakpoint), unless there is
       a signal to report. */
@@ -279,7 +309,7 @@ unsigned char valgrind_wait (char *ourstatus)
    stop_pc = (*the_low_target.get_pc) ();
    
    dlog(1,
-        "exit valgrind_wait returns ptid %s stop_pc %s signal %d\n", 
+        "exit valgrind_wait status T ptid %s stop_pc %s signal %d\n", 
         image_ptid (wptid), sym (stop_pc), sig);
    return sig;
 }

coregrind/m_gdbserver/valgrind_low.h

@@ -2,7 +2,7 @@
    needed for interfacing the Valgrind gdbserver with the Valgrind
    guest.
 
-   Copyright (C) 2011
+   Copyright (C) 2011, 2012
    Free Software Foundation, Inc.
 
    This file has been inspired from a file that is part of GDB.

coregrind/m_libcprint.c

@@ -31,7 +31,7 @@
 #include "pub_core_basics.h"
 #include "pub_core_vki.h"
 #include "pub_core_debuglog.h"
-#include "pub_core_gdbserver.h"
+#include "pub_tool_gdbserver.h"  // VG_(gdb_printf)
 #include "pub_core_libcbase.h"
 #include "pub_core_libcassert.h"
 #include "pub_core_libcfile.h"   // VG_(write)(), VG_(write_socket)()

coregrind/m_main.c

@@ -2543,7 +2543,7 @@ void shutdown_actions_NORETURN( ThreadId tid,
 
    /* terminate gdbserver if ever it was started. We terminate it here so that it get
       the output above if output was redirected to gdb */
-   VG_(gdbserver) (0);
+   VG_(gdbserver_exit) (tid, tids_schedretcode);
 
    /* Ok, finally exit in the os-specific way, according to the scheduler's
       return code.  In short, if the (last) thread exited by calling

coregrind/pub_core_gdbserver.h

@@ -48,6 +48,11 @@ void VG_(gdbserver_prerun_action) (ThreadId tid);
 // to handle this incoming vgdb request.                                
 extern Bool VG_(gdbserver_activity) (ThreadId tid);
 
+// If connected to GDB, VG_(gdbserver_exit) reports to GDB that the process
+// is about to exit.
+// gdbserver is then stopped (using VG_(gdbserver) (0))
+void VG_(gdbserver_exit) (ThreadId tid, VgSchedReturnCode tids_schedretcode);
+
 
 /* Called by low level to insert or remove a break or watch point.
    Break or watch point implementation is done using help from the tool.
@@ -76,8 +81,8 @@ Bool VG_(gdbserver_point) (PointKind kind, Bool insert,
    that the call has been properly pushed by vgdb. */
 extern void VG_(invoke_gdbserver) ( int check );
 
-// To be called before delivering a signal.
-// Returns True if gdb user asks to pass the signal to the client.
+// To be called by core (m_signals.c) before delivering a signal.
+// Returns True unless gdb user asks to not pass the signal to the client.
 // Note that if the below returns True, the signal might
 // still be ignored if this is the action desired by the
 // guest program.

gdbserver_tests/Makefile.am

@@ -92,6 +92,21 @@ EXTRA_DIST = \
 	nlfork_chain.stderr.exp \
 	nlfork_chain.stdout.exp \
 	nlfork_chain.vgtest \
+	nlgone_abrt.stderr.exp \
+	nlgone_abrt.stderrB.exp \
+	nlgone_abrt.stdinB.gdb \
+	nlgone_abrt.stdoutB.exp \
+	nlgone_abrt.vgtest \
+	nlgone_exit.stderr.exp \
+	nlgone_exit.stderrB.exp \
+	nlgone_exit.stdinB.gdb \
+	nlgone_exit.stdoutB.exp \
+	nlgone_exit.vgtest \
+	nlgone_return.stderr.exp \
+	nlgone_return.stderrB.exp \
+	nlgone_return.stdinB.gdb \
+	nlgone_return.stdoutB.exp \
+	nlgone_return.vgtest \
 	nlpasssigalrm.vgtest \
 	nlpasssigalrm.stderrB.exp \
 	nlpasssigalrm.stderr.exp \
@@ -105,9 +120,10 @@ EXTRA_DIST = \
 check_PROGRAMS = \
 	clean_after_fork \
 	fork_chain \
+	gone \
+	main_pic \
 	passsigalrm \
 	sleepers \
-	main_pic \
 	t \
 	watchpoints
 

gdbserver_tests/filter_gdb

@@ -51,6 +51,8 @@ sed -e '/^\ \ \ \ \.\.\.$/d'                                        |
 #         a.o. produced by gdb 7.2 on arm (same with standard gdbserver)
 #       delete empty lines (the last line (only made of prompts) sometimes
 #           finishes with a new line, sometimes not ???).
+#       'exited with code' and 'exited normally' are printed slightly
+#       differently between gdb versions, normalize to "Program exited...".
 sed -e '/Remote debugging using/,/vgdb launched process attached/d'                                   \
     -e 's/^\[?1034hReading symbols/Reading symbols/'                                                \
     -e '/^Missing separate debuginfo/d'                                                               \
@@ -66,6 +68,8 @@ sed -e '/Remote debugging using/,/vgdb launched process attached/d'
     -e '/^Loaded symbols for .*$/d'                                                                   \
     -e '/^Current language.*/d'                                                                       \
     -e '/^The current source language is.*/d'                                                         \
+    -e 's/^.*\( exited with code [0-9]\+\).$/Program\1\./g'                              \
+    -e 's/^.*\( exited normally\).$/Program\1\./g'                              \
     -e 's/(gdb) //g'                                                                                  \
     -e 's/^>[> ]*//'                                                                                  \
     -e '/^done\.$/d'                                                                                  \

gdbserver_tests/gone.c

@@ -0,0 +1,33 @@
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+int
+main (int argc, char **argv)
+{
+  fprintf(stderr, "starting ...\n");
+
+  // Three ways of going away...
+  if (argc > 1)
+    {
+      // Explicit exit() with exit code.
+      if (strcmp (argv[1], "exit") == 0)
+	{
+	  fprintf(stderr, "exiting ...\n");
+	  exit (1);
+	}
+
+      // Get killed by a signal.
+      if (strcmp (argv[1], "abort") == 0)
+	{
+	  fprintf(stderr, "aborting ...\n");
+	  kill(getpid(), SIGABRT);
+	}
+    }
+
+  // And finally, just return from main with success.
+  fprintf(stderr, "returning ...\n");
+  return 0;
+}

gdbserver_tests/mcleak.stderrB.exp

@@ -95,4 +95,3 @@ vgdb-error value changed from 0 to 999999
    by 0x........: f (leak-delta.c:28)
    by 0x........: main (leak-delta.c:60)
 
-Remote connection closed

gdbserver_tests/mcleak.stdoutB.exp

@@ -45,3 +45,4 @@ Breakpoint 1, breakme () at leak-delta.c:9
 #1  0x........ in f () at leak-delta.c:48
 48	   fprintf(stderr, "expecting details 32 (+32) bytes lost, 33 (-32) bytes reachable\n"); fflush(stderr); breakme();
 Continuing.
+Program exited normally.

gdbserver_tests/mcmain_pic.stderrB.exp

@@ -1,3 +1,2 @@
 relaying data between gdb and process ....
 vgdb-error value changed from 0 to 999999
-Remote connection closed

gdbserver_tests/mcmain_pic.stdoutB.exp

@@ -6,3 +6,4 @@ $1 = void
 $2 = (int (*)(int, char **)) 0x........ <main>
 $3 = (void (*)(char *)) 0x........ <another_func>
 Continuing.
+Program exited normally.

gdbserver_tests/mcwatchpoints.stdoutB.exp

@@ -32,3 +32,4 @@ main (argc=1, argv=0x........) at watchpoints.c:49
 49	   fprintf(stderr, "after writing 8\n");
 Delete all breakpoints? (y or n) [answered Y; input not from terminal]
 Continuing.
+Program exited normally.

gdbserver_tests/nlcontrolc.stdoutB.exp

@@ -21,3 +21,4 @@ $5 = 0
 $6 = 0
 $7 = 0
 Continuing.
+Program exited normally.

gdbserver_tests/nlgone_abrt.stderr.exp

@@ -0,0 +1,8 @@
+Nulgrind, the minimal Valgrind tool
+
+(action at startup) vgdb me ... 
+
+
+starting ...
+aborting ...
+

gdbserver_tests/nlgone_abrt.stderrB.exp

@@ -0,0 +1 @@
+relaying data between gdb and process ....

gdbserver_tests/nlgone_abrt.stdinB.gdb

@@ -0,0 +1,9 @@
+# connect gdb to Valgrind gdbserver:
+target remote | ./vgdb --wait=60 --vgdb-prefix=./vgdb-prefix-nlgone-abrt
+echo vgdb launched process attached\n
+
+continue
+# see process get a fatal signal
+continue
+# see program is gone
+quit

gdbserver_tests/nlgone_abrt.stdoutB.exp

@@ -0,0 +1,7 @@
+vgdb launched process attached
+Continuing.
+Program received signal SIGABRT, Aborted.
+0x........ in syscall ...
+Continuing.
+Program terminated with signal SIGABRT, Aborted.
+The program no longer exists.

gdbserver_tests/nlgone_abrt.vgtest

@@ -0,0 +1,12 @@
+# test that a fatal SIGABRT signal is properly passed on to gdb.
+
+prog: gone
+args: abort
+vgopts: --tool=none --vgdb=yes --vgdb-error=0 --vgdb-prefix=./vgdb-prefix-nlgone-abrt
+stderr_filter: filter_stderr
+prereq: test -e gdb
+progB: gdb
+argsB: --quiet -l 60 --nx ./gone
+stdinB: nlgone_abrt.stdinB.gdb
+stdoutB_filter: filter_gdb
+stderrB_filter: filter_gdb

gdbserver_tests/nlgone_exit.stderr.exp

@@ -0,0 +1,8 @@
+Nulgrind, the minimal Valgrind tool
+
+(action at startup) vgdb me ... 
+
+
+starting ...
+exiting ...
+

gdbserver_tests/nlgone_exit.stderrB.exp

@@ -0,0 +1 @@
+relaying data between gdb and process ....

gdbserver_tests/nlgone_exit.stdinB.gdb

@@ -0,0 +1,7 @@
+# connect gdb to Valgrind gdbserver:
+target remote | ./vgdb --wait=60 --vgdb-prefix=./vgdb-prefix-nlgone-exit
+echo vgdb launched process attached\n
+
+continue
+# see program is gone with exit code
+quit

gdbserver_tests/nlgone_exit.stdoutB.exp

@@ -0,0 +1,3 @@
+vgdb launched process attached
+Continuing.
+Program exited with code 01.

gdbserver_tests/nlgone_exit.vgtest

@@ -0,0 +1,12 @@
+# test that an exit (with return value) is properly passed on to gdb.
+
+prog: gone
+args: exit
+vgopts: --tool=none --vgdb=yes --vgdb-error=0 --vgdb-prefix=./vgdb-prefix-nlgone-exit
+stderr_filter: filter_stderr
+prereq: test -e gdb
+progB: gdb
+argsB: --quiet -l 60 --nx ./gone
+stdinB: nlgone_exit.stdinB.gdb
+stdoutB_filter: filter_gdb
+stderrB_filter: filter_gdb

gdbserver_tests/nlgone_return.stderr.exp

@@ -0,0 +1,8 @@
+Nulgrind, the minimal Valgrind tool
+
+(action at startup) vgdb me ... 
+
+
+starting ...
+returning ...
+

gdbserver_tests/nlgone_return.stderrB.exp

@@ -0,0 +1 @@
+relaying data between gdb and process ....

gdbserver_tests/nlgone_return.stdinB.gdb

@@ -0,0 +1,7 @@
+# connect gdb to Valgrind gdbserver:
+target remote | ./vgdb --wait=60 --vgdb-prefix=./vgdb-prefix-nlgone-return
+echo vgdb launched process attached\n
+
+continue
+# see program is gone
+quit

gdbserver_tests/nlgone_return.stdoutB.exp

@@ -0,0 +1,3 @@
+vgdb launched process attached
+Continuing.
+Program exited normally.

gdbserver_tests/nlgone_return.vgtest

@@ -0,0 +1,12 @@
+# test that a normal (successful) return is properly passed on to gdb.
+
+prog: gone
+args: return
+vgopts: --tool=none --vgdb=yes --vgdb-error=0 --vgdb-prefix=./vgdb-prefix-nlgone-return
+stderr_filter: filter_stderr
+prereq: test -e gdb
+progB: gdb
+argsB: --quiet -l 60 --nx ./gone
+stdinB: nlgone_return.stdinB.gdb
+stdoutB_filter: filter_gdb
+stderrB_filter: filter_gdb

gdbserver_tests/nlpasssigalrm.stderrB.exp

@@ -1,3 +1,2 @@
 relaying data between gdb and process ....
 vgdb-error value changed from 0 to 999999
-Remote connection closed

gdbserver_tests/nlpasssigalrm.stdoutB.exp

@@ -19,3 +19,4 @@ Continuing.
 Program received signal SIG34, Real-time event 34.
 0x........ in syscall ...
 Continuing.
+Program exited normally.

include/pub_tool_gdbserver.h

@@ -51,8 +51,8 @@
 // Calling VG_(gdbserver) with tid > 0 means to let a debugger attach
 // to the valgrind process. gdbserver will report to gdb that the
 // process stopped in thread tid.
-// tid == 0 indicates to stop gdbserver and report to gdb
-// that the valgrind-ified process has exited.
+// Calling VG_(gdbserver) with tid == 0 indicates to close
+// the connection with GDB (if still open) and stop gdbserver.
 //--------------------------------------------------------------------
 extern void VG_(gdbserver) ( ThreadId tid );