[rfc] Problems with software single-step and SPU breakpoints during fork

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Hello,

we've seen problems in combined Cell/B.E. debugging if the PPU side fork()s
while debugging on an SPU context is currently in progress.


The first problem occurs if we're currently single-stepping on the SPU
at this point.  In this case, software single-step breakpoints will be
inserted at the time handle_inferior_event receives a TARGET_WAITKIND_FORKED
event.  In this code path (actually, in all the fork/exec related paths),
single-step breakpoints are currently unexpected and will not be removed.
This causes a GDB internal error at the next resume.

The patch fixes this by pulling out singlestep breakpoints at fork time
(and just resetting the flag at exec time as the address space is
already gone).  The next resume call will re-insert them anyway if
required.


The second problem is related to detach_breakpoints.  This routine is called
at fork time and pulls all breakpoints out of the child process, assuming
that they were copied by fork.

While this is certainly true for PPU side breakpoints, it is not true for SPU
side breakpoints.  fork will clone the SPU context file descriptors, so that
all the existing SPU contexts are in accessible in the new process.  However,
the contents of the SPU contexts themselves are *not* cloned.  Therefore the
effect of detach_breakpoints is to remove SPU breakpoints from the *original*
SPU context's local store ...

As a workaround, I'm now installing an SPU gdbarch_memory_remove_breakpoint
routine that simply does nothing if called from detach_breakpoints.  This
case is detected if the PID we are asked to remove this breakpoint from
(i.e. ptid_get_pid (inferior_ptid)) is different from the PID of the current
inferior (i.e. current_inferior()->pid).

Any comments?  I'd appreciate any suggestions how to fix this in a
cleaner way ..


Tested on spu-elf powerpc64-linux (Cell/B.E.).   I'm planning to commit
this by the end of the week, unless we find something better ...

Bye,
Ulrich


ChangeLog:

	* infrun.c (handle_inferior_event): Handle presence of single-step
	breakpoints for TARGET_WAITKIND_FORKED, TARGET_WAITKIND_VFORKED,
	and TARGET_WAITKIND_EXECD.

	* spu-tdep.c (spu_memory_remove_breakpoint): New function.
	(spu_gdbarch_init): Install it.

testsuite/ChangeLog:

	* gdb.cell/fork.exp: New file.
	* gdb.cell/fork.c: Likewise.
	* gdb.cell/fork-spu.c: Likewise.


Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.442
diff -u -p -r1.442 infrun.c
--- gdb/infrun.c	12 Jun 2010 00:05:21 -0000	1.442
+++ gdb/infrun.c	21 Jun 2010 18:19:01 -0000
@@ -3204,6 +3204,13 @@ handle_inferior_event (struct execution_
 	  reinit_frame_cache ();
 	}
 
+      if (singlestep_breakpoints_inserted_p)
+	{
+	  /* Pull the single step breakpoints out of the target. */
+	  remove_single_step_breakpoints ();
+	  singlestep_breakpoints_inserted_p = 0;
+	}
+
       /* Immediately detach breakpoints from the child before there's
 	 any chance of letting the user delete breakpoints from the
 	 breakpoint lists.  If we don't do this early, it's easy to
@@ -3314,6 +3321,8 @@ handle_inferior_event (struct execution_
 	  reinit_frame_cache ();
 	}
 
+      singlestep_breakpoints_inserted_p = 0;
+
       stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
 
       /* Do whatever is necessary to the parent branch of the vfork.  */
Index: gdb/spu-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/spu-tdep.c,v
retrieving revision 1.60
diff -u -p -r1.60 spu-tdep.c
--- gdb/spu-tdep.c	19 Jun 2010 17:59:06 -0000	1.60
+++ gdb/spu-tdep.c	21 Jun 2010 18:19:01 -0000
@@ -1494,6 +1494,39 @@ spu_breakpoint_from_pc (struct gdbarch *
   return breakpoint;
 }
 
+static int
+spu_memory_remove_breakpoint (struct gdbarch *gdbarch,
+			      struct bp_target_info *bp_tgt)
+{
+  /* We work around a problem in combined Cell/B.E. debugging here.  Consider
+     that in a combined application, we have some breakpoints inserted in SPU
+     code, and now the application forks (on the PPU side).  GDB common code
+     will assume that the fork system call copied all breakpoints into the new
+     process' address space, and that all those copies now need to be removed
+     (see breakpoint.c:detach_breakpoints).
+
+     While this is certainly true for PPU side breakpoints, it is not true
+     for SPU side breakpoints.  fork will clone the SPU context file
+     descriptors, so that all the existing SPU contexts are in accessible
+     in the new process.  However, the contents of the SPU contexts themselves
+     are *not* cloned.  Therefore the effect of detach_breakpoints is to
+     remove SPU breakpoints from the *original* SPU context's local store
+     -- this is not the correct behaviour.
+
+     The workaround is to check whether the PID we are asked to remove this
+     breakpoint from (i.e. ptid_get_pid (inferior_ptid)) is different from the
+     PID of the current inferior (i.e. current_inferior()->pid).  This is only
+     true in the context of detach_breakpoints.  If so, we simply do nothing.
+     [ Note that for the fork child process, it does not matter if breakpoints
+     remain inserted, because those SPU contexts are not runnable anyway --
+     the Linux kernel allows only the original process to invoke spu_run.  */
+
+  if (ptid_get_pid (inferior_ptid) != current_inferior()->pid) 
+    return 0;
+
+  return default_memory_remove_breakpoint (gdbarch, bp_tgt);
+}
+
 
 /* Software single-stepping support.  */
 
@@ -2638,6 +2671,7 @@ spu_gdbarch_init (struct gdbarch_info in
   /* Breakpoints.  */
   set_gdbarch_decr_pc_after_break (gdbarch, 4);
   set_gdbarch_breakpoint_from_pc (gdbarch, spu_breakpoint_from_pc);
+  set_gdbarch_memory_remove_breakpoint (gdbarch, spu_memory_remove_breakpoint);
   set_gdbarch_cannot_step_breakpoint (gdbarch, 1);
   set_gdbarch_software_single_step (gdbarch, spu_software_single_step);
   set_gdbarch_get_longjmp_target (gdbarch, spu_get_longjmp_target);
--- /dev/null	2010-06-09 19:31:28.423437333 +0200
+++ gdb/testsuite/gdb.cell/fork.exp	2010-06-21 20:16:23.000000000 +0200
@@ -0,0 +1,85 @@
+# Copyright 2010 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/>.
+#
+# Contributed by Ulrich Weigand <uweigand@de.ibm.com>.
+#
+# Testsuite for Cell Broadband Engine combined debugger
+# This testcases tests support for PPU-side fork during SPU debugging
+
+load_lib cell.exp
+
+set testfile "fork"
+set ppu_file "fork"
+set ppu_src ${srcdir}/${subdir}/${ppu_file}.c
+set ppu_bin ${objdir}/${subdir}/${ppu_file}
+set spu_file "fork-spu"
+set spu_src ${srcdir}/${subdir}/${spu_file}.c
+set spu_bin ${objdir}/${subdir}/${spu_file}
+
+if {[skip_cell_tests]} {
+    return 0
+}
+
+# Compile SPU binary.
+if { [gdb_compile_cell_spu $spu_src $spu_bin executable {debug}]  != "" } {
+  unsupported "Compiling spu binary failed."
+  return -1
+}
+# Compile PPU binary.
+if { [gdb_cell_embedspu $spu_bin $spu_bin-embed.o {debug}]  != "" } {
+  unsupported "Embedding spu binary failed."
+  return -1
+}
+if { [gdb_compile_cell_ppu [list $ppu_src $spu_bin-embed.o] $ppu_bin executable {debug}] != "" } {
+  unsupported "Compiling ppu binary failed."
+  return -1
+}
+
+if [get_compiler_info ${ppu_bin}] {
+  return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${ppu_bin}
+
+if ![runto_main] then {
+  fail "Can't run to main"
+  return 0
+}
+
+gdb_test_no_output "set spu stop-on-load" "set spu stop-on-load"
+
+gdb_test "continue" "Continuing\\..*Temporary breakpoint \[0-9\]+, main \\(speid=.*, argp=.*, envp=.*\\) at .*$spu_file\\.c:.*spu_write_out_intr_mbox.*" \
+	 "run until SPU main"
+
+gdb_test "break func" "Breakpoint \[0-9\]+ at.* file .*$spu_file.c, line \[0-9\]+\\." "break func"
+gdb_test "watch var" "Watchpoint \[0-9\]+: var" "watch var"
+
+gdb_test "continue" "Continuing\\..*Watchpoint.*Old value = 0.*New value = 1.*" \
+	 "run until watchpoint hit"
+
+gdb_test_no_output "delete \$bpnum" "delete watchpoint"
+
+gdb_test "continue" "Continuing\\..*Breakpoint \[0-9\]+, func \\(\\) at .*$spu_file.c:.*" \
+	 "run until breakpoint hit"
+
+gdb_test "continue" "Continuing\\..*Program exited normally.*" \
+	 "run until end"
+
+gdb_exit
+
+return 0
--- /dev/null	2010-06-09 19:31:28.423437333 +0200
+++ gdb/testsuite/gdb.cell/fork.c	2010-06-21 19:32:02.000000000 +0200
@@ -0,0 +1,77 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Contributed by Ulrich Weigand <uweigand@de.ibm.com>  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <libspe2.h>
+#include <pthread.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+extern spe_program_handle_t fork_spu;
+
+void *
+spe_thread (void * arg)
+{
+  int flags = 0;
+  unsigned int entry = SPE_DEFAULT_ENTRY;
+  spe_context_ptr_t *ctx = (spe_context_ptr_t *) arg;
+
+  spe_program_load (*ctx, &fork_spu);
+  spe_context_run (*ctx, &entry, flags, NULL, NULL, NULL);
+
+  pthread_exit (NULL);
+}
+
+int
+main (void)
+{
+  pthread_t pts;
+  spe_context_ptr_t ctx;
+  unsigned int value;
+  unsigned int pid;
+
+  ctx = spe_context_create (0, NULL);
+  pthread_create (&pts, NULL, &spe_thread, &ctx);
+
+  /* Wait until the SPU thread is running.  */
+  spe_out_intr_mbox_read (ctx, &value, 1, SPE_MBOX_ALL_BLOCKING);
+
+  pid = fork ();
+  if (pid == 0)
+    {
+      /* This is the child.  Just exit immediately.  */
+      exit (0);
+    }
+  else
+    {
+      /* This is the parent.  Wait for the child to exit.  */
+      waitpid (pid, NULL, 0);
+    }
+
+  /* Tell SPU to continue.  */
+  spe_in_mbox_write (ctx, &value, 1, SPE_MBOX_ALL_BLOCKING);
+  
+  pthread_join (pts, NULL);
+  spe_context_destroy (ctx);
+
+  return 0;
+}
+
--- /dev/null	2010-06-09 19:31:28.423437333 +0200
+++ gdb/testsuite/gdb.cell/fork-spu.c	2010-06-21 19:33:14.000000000 +0200
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 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/>.
+
+   Contributed by Ulrich Weigand <uweigand@de.ibm.com>  */
+
+#include <spu_mfcio.h>
+
+int var;
+
+void
+func (void)
+{
+}
+
+int
+main (unsigned long long speid, unsigned long long argp,
+      unsigned long long envp)
+{
+  /* Signal to PPU side that it should fork now.  */
+  spu_write_out_intr_mbox (0);
+
+  /* Wait until fork completed.  */
+  spu_read_in_mbox ();
+
+  /* Trigger watchpoint.  */
+  var = 1;
+
+  /* Now call some function to trigger breakpoint.  */
+  func ();
+
+  return 0;
+}
+
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com