diff mbox

[2/2] Fix native follow-exec-mode "new"

Message ID 1440542496-14988-3-git-send-email-donb@codesourcery.com
State New
Headers show

Commit Message

Don Breazeal Aug. 25, 2015, 10:41 p.m. UTC
This patch fixes a segmentation fault in native GDB when 
handling an exec event with follow-exec-mode set to "new".

The stack trace from the segfault was this:

#0  0x0000000000669594 in gdbarch_data (gdbarch=0x0, data=0x20da7a0)
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/gdbarch.c:4847
#1  0x00000000004d430e in get_remote_arch_state ()
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/remote.c:603
#2  0x00000000004d431e in get_remote_state ()
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/remote.c:616
#3  0x00000000004dda8b in discard_pending_stop_replies (inf=0x217c710)
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/remote.c:5775
#4  0x00000000006a5928 in observer_inferior_exit_notification_stub (
    data=0x4dda7a <discard_pending_stop_replies>, args_data=0x7fff12c258f0)
    at ./observer.inc:1137
#5  0x00000000006a419a in generic_observer_notify (subject=0x21dfbe0, 
    args=0x7fff12c258f0)
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/observer.c:167
#6  0x00000000006a59ba in observer_notify_inferior_exit (inf=0x217c710)
    at ./observer.inc:1162
#7  0x00000000007981d5 in exit_inferior_1 (inftoex=0x217c710, silent=1)
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/inferior.c:244
#8  0x00000000007982f2 in exit_inferior_num_silent (num=1)
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/inferior.c:286
#9  0x000000000062f93d in follow_exec (ptid=..., 
    execd_pathname=0x7fff12c259a0 "/scratch/dbreazea/sandbox/exec-nat/build/gdb/testsuite/gdb.base/execd-prog")
    at /scratch/dbreazea/sandbox/exec-nat/binutils-gdb/gdb/infrun.c:1195

In follow_exec we were creating a new inferior for the execd program,
as required by the exec mode, but we were doing it before calling
exit_inferior_num_silent on the original inferior.  So on entry to
exit_inferior_num_silent we had two inferiors with the same ptid.

In the calls made by exit_inferior_num_silent, the current inferior
is temporarily saved and replaced in order to make use of functions
that only operate on the current inferior (for example, in
do_all_continuations, called while deleting the threads of the original
inferior).  When we restored the original inferior, we just took the
first inferior that matched the ptid of the original and got the new
(wrong) one.  It hadn't been initialized yet and had no gdbarch
pointer, and GDB segfaulted.

The fix for that is to call exit_inferior_num_silent before adding the new
inferior, so that we never have two inferiors with the same ptid.  Then
exit_inferior_num_silent uses the original inferior as the current inferior
throughout, and can find a valid gdbarch pointer.

Once we have finished with the exit of the old inferior and added the
new one, we need to create a new thread for the new inferior.  In the
function that called follow_exec, handle_inferior_event_1,
ecs->event_thread now points to the thread that was deleted with the
exit of the original inferior.  To remedy this we create the new thread,
and once we return from follow_exec we reset ecs->event_thread.

Note that we are guaranteed that we can reset ecs->event_thread
safely using inferior_thread because we have set the current
inferior in follow_exec, and inferior_ptid was set by the call
to context_switch at the beginning of exec event handling.

WDYT?
Thanks
--Don

gdb/
2015-08-25  Don Breazeal  <donb@codesourcery.com>

	* infrun.c (follow_exec): Re-order operations for
	handling follow-exec-mode "new".
	(handle_inferior_event_1): Assign ecs->event_thread
	to the current thread.
	* remote.c (get_remote_arch_state): Add an assertion.

---
 gdb/infrun.c | 15 ++++++++++++---
 gdb/remote.c |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Pedro Alves Aug. 26, 2015, 10:41 a.m. UTC | #1
On 08/25/2015 11:41 PM, Don Breazeal wrote:

> 
> gdb/
> 2015-08-25  Don Breazeal  <donb@codesourcery.com>
> 
> 	* infrun.c (follow_exec): Re-order operations for
> 	handling follow-exec-mode "new".
> 	(handle_inferior_event_1): Assign ecs->event_thread
> 	to the current thread.
> 	* remote.c (get_remote_arch_state): Add an assertion.


OK.

Thanks,
Pedro Alves
Don Breazeal Aug. 26, 2015, 9:28 p.m. UTC | #2
On 8/26/2015 3:41 AM, Pedro Alves wrote:
> On 08/25/2015 11:41 PM, Don Breazeal wrote:
> 
>>
>> gdb/
>> 2015-08-25  Don Breazeal  <donb@codesourcery.com>
>>
>> 	* infrun.c (follow_exec): Re-order operations for
>> 	handling follow-exec-mode "new".
>> 	(handle_inferior_event_1): Assign ecs->event_thread
>> 	to the current thread.
>> 	* remote.c (get_remote_arch_state): Add an assertion.
> 
> 
> OK.
> 
> Thanks,
> Pedro Alves
> 
Thanks Pedro.  This is now pushed.
--Don
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 25036a4..d043563 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1187,12 +1187,16 @@  follow_exec (ptid_t ptid, char *execd_pathname)
       /* The user wants to keep the old inferior and program spaces
 	 around.  Create a new fresh one, and switch to it.  */
 
-      inf = add_inferior (current_inferior ()->pid);
+      /* Do exit processing for the original inferior before adding
+	 the new inferior so we don't have two active inferiors with
+	 the same ptid, which can confuse find_inferior_ptid.  */
+      exit_inferior_num_silent (current_inferior ()->num);
+
+      inf = add_inferior (pid);
       pspace = add_program_space (maybe_new_address_space ());
       inf->pspace = pspace;
       inf->aspace = pspace->aspace;
-
-      exit_inferior_num_silent (current_inferior ()->num);
+      add_thread (ptid);
 
       set_current_inferior (inf);
       set_current_program_space (pspace);
@@ -4978,6 +4982,11 @@  Cannot fill $_exitsignal with the correct signal number.\n"));
          stop.  */
       follow_exec (inferior_ptid, ecs->ws.value.execd_pathname);
 
+      /* In follow_exec we may have deleted the original thread and
+	 created a new one.  Make sure that the event thread is the
+	 execd thread for that case (this is a nop otherwise).  */
+      ecs->event_thread = inferior_thread ();
+
       ecs->event_thread->control.stop_bpstat
 	= bpstat_stop_status (get_regcache_aspace (get_current_regcache ()),
 			      stop_pc, ecs->ptid, &ecs->ws);
diff --git a/gdb/remote.c b/gdb/remote.c
index f2968eb..9bb81ed 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -600,6 +600,7 @@  static struct gdbarch_data *remote_gdbarch_data_handle;
 static struct remote_arch_state *
 get_remote_arch_state (void)
 {
+  gdb_assert (target_gdbarch () != NULL);
   return gdbarch_data (target_gdbarch (), remote_gdbarch_data_handle);
 }