[binutils-gdb] follow-exec: delete all non-execing threads

[Pedro Alves <palves at sourceware dot org>]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=95e50b2723eba05ca34e9ea69c1de63e65ce9578

commit 95e50b2723eba05ca34e9ea69c1de63e65ce9578
Author: Pedro Alves <palves@redhat.com>
Date:   Tue Mar 3 01:25:17 2015 +0000

    follow-exec: delete all non-execing threads
    
    This fixes invalid reads Valgrind first caught when debugging against
    a GDBserver patched with a series that adds exec events to the remote
    protocol.  Like these, using the gdb.threads/thread-execl.exp test:
    
    $ valgrind ./gdb -data-directory=data-directory ./testsuite/gdb.threads/thread-execl  -ex "tar extended-remote :9999" -ex "b thread_execler" -ex "c" -ex "set scheduler-locking on"
    ...
    Breakpoint 1, thread_execler (arg=0x0) at src/gdb/testsuite/gdb.threads/thread-execl.c:29
    29        if (execl (image, image, NULL) == -1)
    (gdb) n
    Thread 32509.32509 is executing new program: build/gdb/testsuite/gdb.threads/thread-execl
    [New Thread 32509.32532]
    ==32510== Invalid read of size 4
    ==32510==    at 0x5AA7D8: delete_breakpoint (breakpoint.c:13989)
    ==32510==    by 0x6285D3: delete_thread_breakpoint (thread.c:100)
    ==32510==    by 0x628603: delete_step_resume_breakpoint (thread.c:109)
    ==32510==    by 0x61622B: delete_thread_infrun_breakpoints (infrun.c:2928)
    ==32510==    by 0x6162EF: for_each_just_stopped_thread (infrun.c:2958)
    ==32510==    by 0x616311: delete_just_stopped_threads_infrun_breakpoints (infrun.c:2969)
    ==32510==    by 0x616C96: fetch_inferior_event (infrun.c:3267)
    ==32510==    by 0x63A2DE: inferior_event_handler (inf-loop.c:57)
    ==32510==    by 0x4E0E56: remote_async_serial_handler (remote.c:11877)
    ==32510==    by 0x4AF620: run_async_handler_and_reschedule (ser-base.c:137)
    ==32510==    by 0x4AF6F0: fd_event (ser-base.c:182)
    ==32510==    by 0x63806D: handle_file_event (event-loop.c:762)
    ==32510==  Address 0xcf333e0 is 16 bytes inside a block of size 200 free'd
    ==32510==    at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==32510==    by 0x77CB74: xfree (common-utils.c:98)
    ==32510==    by 0x5AA954: delete_breakpoint (breakpoint.c:14056)
    ==32510==    by 0x5988BD: update_breakpoints_after_exec (breakpoint.c:3765)
    ==32510==    by 0x61360F: follow_exec (infrun.c:1091)
    ==32510==    by 0x6186FA: handle_inferior_event (infrun.c:4061)
    ==32510==    by 0x616C55: fetch_inferior_event (infrun.c:3261)
    ==32510==    by 0x63A2DE: inferior_event_handler (inf-loop.c:57)
    ==32510==    by 0x4E0E56: remote_async_serial_handler (remote.c:11877)
    ==32510==    by 0x4AF620: run_async_handler_and_reschedule (ser-base.c:137)
    ==32510==    by 0x4AF6F0: fd_event (ser-base.c:182)
    ==32510==    by 0x63806D: handle_file_event (event-loop.c:762)
    ==32510==
    [Switching to Thread 32509.32532]
    
    Breakpoint 1, thread_execler (arg=0x0) at src/gdb/testsuite/gdb.threads/thread-execl.c:29
    29        if (execl (image, image, NULL) == -1)
    (gdb)
    
    The breakpoint in question is the step-resume breakpoint of the
    non-main thread, the one that was "next"ed.
    
    The exact same issue can be seen on mainline with native debugging, by
    running the thread-execl.exp test in non-stop mode, because the kernel
    doesn't report a thread exit event for the execing thread.
    
    Tested on x86_64 Fedora 20.
    
    gdb/ChangeLog:
    2015-03-02  Pedro Alves  <palves@redhat.com>
    
    	* infrun.c (follow_exec): Delete all threads of the process except
    	the event thread.  Extended comments.
    
    gdb/testsuite/ChangeLog:
    2015-03-02  Pedro Alves  <palves@redhat.com>
    
    	* gdb.threads/thread-execl.exp (do_test): Handle non-stop.
    	(top level): Call do_test with non-stop as well.

Diff:
---
 gdb/ChangeLog                              |  5 ++++
 gdb/infrun.c                               | 48 ++++++++++++++++++++++--------
 gdb/testsuite/ChangeLog                    |  5 ++++
 gdb/testsuite/gdb.threads/thread-execl.exp | 24 +++++++++++++--
 4 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 922b1d9..68c55c1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-03-02  Pedro Alves  <palves@redhat.com>
+
+	* infrun.c (follow_exec): Delete all threads of the process except
+	the event thread.  Extended comments.
+
 2015-03-02  Joel Brobecker  <brobecker@adacore.com>
 
 	* contrib/ari/gdb_ari.sh: Reinstate checks for "true" and "false".
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 15589b6..f87ed4c 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1060,10 +1060,11 @@ show_follow_exec_mode_string (struct ui_file *file, int from_tty,
 /* EXECD_PATHNAME is assumed to be non-NULL.  */
 
 static void
-follow_exec (ptid_t pid, char *execd_pathname)
+follow_exec (ptid_t ptid, char *execd_pathname)
 {
-  struct thread_info *th = inferior_thread ();
+  struct thread_info *th, *tmp;
   struct inferior *inf = current_inferior ();
+  int pid = ptid_get_pid (ptid);
 
   /* This is an exec event that we actually wish to pay attention to.
      Refresh our symbol table to the newly exec'd program, remove any
@@ -1088,24 +1089,47 @@ follow_exec (ptid_t pid, char *execd_pathname)
 
   mark_breakpoints_out ();
 
-  update_breakpoints_after_exec ();
-
-  /* If there was one, it's gone now.  We cannot truly step-to-next
-     statement through an exec().  */
+  /* The target reports the exec event to the main thread, even if
+     some other thread does the exec, and even if the main thread was
+     stopped or already gone.  We may still have non-leader threads of
+     the process on our list.  E.g., on targets that don't have thread
+     exit events (like remote); or on native Linux in non-stop mode if
+     there were only two threads in the inferior and the non-leader
+     one is the one that execs (and nothing forces an update of the
+     thread list up to here).  When debugging remotely, it's best to
+     avoid extra traffic, when possible, so avoid syncing the thread
+     list with the target, and instead go ahead and delete all threads
+     of the process but one that reported the event.  Note this must
+     be done before calling update_breakpoints_after_exec, as
+     otherwise clearing the threads' resources would reference stale
+     thread breakpoints -- it may have been one of these threads that
+     stepped across the exec.  We could just clear their stepping
+     states, but as long as we're iterating, might as well delete
+     them.  Deleting them now rather than at the next user-visible
+     stop provides a nicer sequence of events for user and MI
+     notifications.  */
+  ALL_NON_EXITED_THREADS_SAFE (th, tmp)
+    if (ptid_get_pid (th->ptid) == pid && !ptid_equal (th->ptid, ptid))
+      delete_thread (th->ptid);
+
+  /* We also need to clear any left over stale state for the
+     leader/event thread.  E.g., if there was any step-resume
+     breakpoint or similar, it's gone now.  We cannot truly
+     step-to-next statement through an exec().  */
+  th = inferior_thread ();
   th->control.step_resume_breakpoint = NULL;
   th->control.exception_resume_breakpoint = NULL;
   th->control.single_step_breakpoints = NULL;
   th->control.step_range_start = 0;
   th->control.step_range_end = 0;
 
-  /* The target reports the exec event to the main thread, even if
-     some other thread does the exec, and even if the main thread was
-     already stopped --- if debugging in non-stop mode, it's possible
-     the user had the main thread held stopped in the previous image
-     --- release it now.  This is the same behavior as step-over-exec
-     with scheduler-locking on in all-stop mode.  */
+  /* The user may have had the main thread held stopped in the
+     previous image (e.g., schedlock on, or non-stop).  Release
+     it now.  */
   th->stop_requested = 0;
 
+  update_breakpoints_after_exec ();
+
   /* What is this a.out's name?  */
   printf_unfiltered (_("%s is executing new program: %s\n"),
 		     target_pid_to_str (inferior_ptid),
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 76ed5e2..3b7bf7d 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2015-03-02  Pedro Alves  <palves@redhat.com>
 
+	* gdb.threads/thread-execl.exp (do_test): Handle non-stop.
+	(top level): Call do_test with non-stop as well.
+
+2015-03-02  Pedro Alves  <palves@redhat.com>
+
 	* lib/gdb.exp (gdb_test_multiple) <internal error>: Set result to
 	-1.
 
diff --git a/gdb/testsuite/gdb.threads/thread-execl.exp b/gdb/testsuite/gdb.threads/thread-execl.exp
index 4a8016c..a598ad0 100644
--- a/gdb/testsuite/gdb.threads/thread-execl.exp
+++ b/gdb/testsuite/gdb.threads/thread-execl.exp
@@ -34,9 +34,18 @@ if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
 proc do_test { schedlock } {
     global binfile
 
-    with_test_prefix "schedlock $schedlock" {
+    if {$schedlock == "non-stop"} {
+	set prefix $schedlock
+    } else {
+	set prefix "schedlock $schedlock"
+    }
+    with_test_prefix "$prefix" {
 	clean_restart ${binfile}
 
+	if {$schedlock == "non-stop"} {
+	    gdb_test_no_output "set non-stop 1"
+	}
+
 	if ![runto_main] {
 	    return 0
 	}
@@ -45,16 +54,25 @@ proc do_test { schedlock } {
 	gdb_breakpoint "thread_execler"
 	gdb_test "continue" ".*thread_execler.*" "continue to thread start"
 
+	if {$schedlock == "non-stop"} {
+	    gdb_test "thread 2" \
+		"Switching to .*thread_execler.*" \
+		"switch to event thread"
+	}
+
 	# Now set a breakpoint at `main', and step over the execl call.  The
 	# breakpoint at main should be reached.  GDB should not try to revert
 	# back to the old thread from the old image and resume stepping it
 	# (since it is gone).
 	gdb_breakpoint "main"
-	gdb_test_no_output "set scheduler-locking $schedlock"
+
+	if {$schedlock != "non-stop"} {
+	    gdb_test_no_output "set scheduler-locking $schedlock"
+	}
 	gdb_test "next" ".*main.*" "get to main in new image"
     }
 }
 
-foreach schedlock {"off" "step" "on"} {
+foreach schedlock {"off" "step" "on" "non-stop"} {
     do_test $schedlock
 }