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

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

Pedro Alves wrote:

> I think you should unpatch the single-step breakpoints from
> both parent and child.  If you set detach-on-fork off, and then
> resume the child, won't it trip on the left over software single-step
> breakpoint?  Oh, I guess that happens to not be a problem on the SPU,
> as both parent and child access the same SPU contexts, so removing
> the breakpoints from the parent also removed it from the child, though it
> looks like a problem on regular software single-step archs, like linux
> ARM or MIPS.  Unfortunately, single step breakpoints are still using the
> deprecated_insert_raw_breakpoint mechanism and don't appear in the
> breakpoints/locations lists, otherwise, the "Immediately detach
> breakpoints" bit below would also take care of it ...

Good point.  I've updated detach_breakpoints to also take care of
single-step breakpoints for now.

> Hmm, what is actually clearing single_step_breakpoints[0|1]?
> Are things just appearing to work correctly because 
> the single_step_breakpoints[1] slot happens to be free on next
> single-step resume?  (actually, don't we have the same problem
> with process exits while single-step breakpoints are inserted?
> that's a rare event, but possible.)

I've just copied this from process exit, but it seems you're right.
I've now added a new cancel_single_step_breakpoints routine that
can be called from infrun at those events.

> (software single-step and these related globals obviously
> need TLC for multi-process/non-stop)

That's certainly true.

> I think doing something clean would entail teaching gdb core
> about SPU/PPU's multiple address spaces.

That's not yet possible as it would require support for more than
one program / address space per inferior ...  But long term this
seems the way to go.
 
> detach_breakpoints itself is a hack, so I don't have a problem
> with going with this approach meanwhile.

OK, thanks for the review and feedback!

Below is an updated patch with the changes described above.
Tested on powerpc64-linux (on Cell/B.E.).

Bye,
Ulrich


ChangeLog:

	* infrun.c (handle_inferior_event): Handle presence of single-step
	breakpoints for TARGET_WAITKIND_FORKED and TARGET_WAITKIND_VFORKED.
	Cancel single-step breakpoints for TARGET_WAITKIND_EXITED,
	TARGET_WAITKIND_SIGNALED, and TARGET_WAITKIND_EXECD.
	* breakpoint.c (detach_single_step_breakpoints): New function.
	(detach_breakpoints): Call it.
	(cancel_single_step_breakpoints): New function.
	* breakpoint.h (cancel_single_step_breakpoints): Add prototype.

	* 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/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.490
diff -u -p -r1.490 breakpoint.c
--- gdb/breakpoint.c	16 Jun 2010 18:30:30 -0000	1.490
+++ gdb/breakpoint.c	22 Jun 2010 15:04:45 -0000
@@ -196,6 +196,8 @@ static void tcatch_command (char *arg, i
 
 static void ep_skip_leading_whitespace (char **s);
 
+static void detach_single_step_breakpoints (void);
+
 static int single_step_breakpoint_inserted_here_p (struct address_space *,
 						   CORE_ADDR pc);
 
@@ -2383,6 +2385,10 @@ detach_breakpoints (int pid)
     if (b->inserted)
       val |= remove_breakpoint_1 (b, mark_inserted);
   }
+
+  /* Detach single-step breakpoints as well.  */
+  detach_single_step_breakpoints ();
+
   do_cleanups (old_chain);
   return val;
 }
@@ -10491,6 +10497,39 @@ remove_single_step_breakpoints (void)
     }
 }
 
+/* Delete software single step breakpoints without removing them from
+   the inferior.  This is intended to be used if the inferior's address
+   space where they were inserted is already gone, e.g. after exit or
+   exec.  */
+
+void
+cancel_single_step_breakpoints (void)
+{
+  int i;
+
+  for (i = 0; i < 2; i++)
+    if (single_step_breakpoints[i])
+      {
+	xfree (single_step_breakpoints[i]);
+	single_step_breakpoints[i] = NULL;
+	single_step_gdbarch[i] = NULL;
+      }
+}
+
+/* Detach software single-step breakpoints from INFERIOR_PTID without
+   removing them.  */
+
+static void
+detach_single_step_breakpoints (void)
+{
+  int i;
+
+  for (i = 0; i < 2; i++)
+    if (single_step_breakpoints[i])
+      target_remove_breakpoint (single_step_gdbarch[i],
+				single_step_breakpoints[i]);
+}
+
 /* Check whether a software single-step breakpoint is inserted at PC.  */
 
 static int
Index: gdb/breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.119
diff -u -p -r1.119 breakpoint.h
--- gdb/breakpoint.h	7 Jun 2010 13:39:10 -0000	1.119
+++ gdb/breakpoint.h	22 Jun 2010 15:04:45 -0000
@@ -985,6 +985,7 @@ extern int remove_hw_watchpoints (void);
 extern void insert_single_step_breakpoint (struct gdbarch *,
 					   struct address_space *, CORE_ADDR);
 extern void remove_single_step_breakpoints (void);
+extern void cancel_single_step_breakpoints (void);
 
 /* Manage manual breakpoints, separate from the normal chain of
    breakpoints.  These functions are used in murky target-specific
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	22 Jun 2010 15:04:47 -0000
@@ -3165,6 +3165,7 @@ handle_inferior_event (struct execution_
       gdb_flush (gdb_stdout);
       target_mourn_inferior ();
       singlestep_breakpoints_inserted_p = 0;
+      cancel_single_step_breakpoints ();
       stop_print_frame = 0;
       stop_stepping (ecs);
       return;
@@ -3188,6 +3189,7 @@ handle_inferior_event (struct execution_
 
       print_stop_reason (SIGNAL_EXITED, ecs->ws.value.sig);
       singlestep_breakpoints_inserted_p = 0;
+      cancel_single_step_breakpoints ();
       stop_stepping (ecs);
       return;
 
@@ -3225,6 +3227,13 @@ handle_inferior_event (struct execution_
 	  detach_breakpoints (child_pid);
 	}
 
+      if (singlestep_breakpoints_inserted_p)
+	{
+	  /* Pull the single step breakpoints out of the target. */
+	  remove_single_step_breakpoints ();
+	  singlestep_breakpoints_inserted_p = 0;
+	}
+
       /* In case the event is caught by a catchpoint, remember that
 	 the event is to be followed at the next resume of the thread,
 	 and not immediately.  */
@@ -3314,6 +3323,9 @@ handle_inferior_event (struct execution_
 	  reinit_frame_cache ();
 	}
 
+      singlestep_breakpoints_inserted_p = 0;
+      cancel_single_step_breakpoints ();
+
       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	22 Jun 2010 15:04:47 -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:20:00.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