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

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

[Ulrich Weigand]

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

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

[Pedro Alves]

On Monday 21 June 2010 20:30:38, Ulrich Weigand wrote:
> +      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

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 ...

> @@ -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));

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.)

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

On Monday 21 June 2010 20:30:38, Ulrich Weigand wrote:
> 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 ..

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

detach_breakpoints itself is a hack, so I don't have a problem
with going with this approach meanwhile.

-- 
Pedro Alves

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

[Ulrich Weigand]

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

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

[Pedro Alves]

On Tuesday 22 June 2010 16:22:37, Ulrich Weigand wrote:

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

Looks good to me.  Thanks!

-- 
Pedro Alves

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

[Ulrich Weigand]

Pedro Alves wrote:
> On Tuesday 22 June 2010 16:22:37, Ulrich Weigand wrote:
> 
> > Below is an updated patch with the changes described above.
> > Tested on powerpc64-linux (on Cell/B.E.).
> 
> Looks good to me.  Thanks!

Thanks once again for the review!

I've checked the patch in now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com