Re: [PATCH, ppc] Fix hw *points for embedded ppc in a threaded environment

[Luis Machado <lgustavo at codesourcery dot com>]

On 04/17/2013 07:04 PM, Sergio Durigan Junior wrote:
> Didn't you forget to delete the previous line?  :-)
>
> > +	  {
> > +	    /* The ppc Linux kernel causes a thread to inherit its parent
> > +	       thread's debug state, and that includes any hardware
> > +	       watchpoints or breakpoints that the parent thread may have set.
> > +
> > +	       For this reason, the debug state of the new thread is cleared
> > +	       before trying to replicate any hardware watchpoints or
> > +	       breakpoints contained in other threads.  */
> > +
> > +	    /* The ppc debug resource accounting is done through "slots".
> > +	       Ask the kernel the deallocate this specific *point's slot.  */
> > +	    ptrace (PPC_PTRACE_DELHWDEBUG, tid, 0, hw_breaks[i].slot);
> > +
> > +	    booke_insert_point (hw_breaks[i].hw_break, tid);
> > +	  }

Heh, obviously!

Updated patch attached.

Thanks,
Luis

Index: gdb-head/gdb/ChangeLog
===================================================================
--- gdb-head.orig/gdb/ChangeLog	2013-04-17 11:13:12.551447594 +0200
+++ gdb-head/gdb/ChangeLog	2013-04-17 17:21:26.959055004 +0200
@@ -1,3 +1,12 @@
+2013-04-17  Luis Machado  <lgustavo@codesourcery.com>
+
+	* ppc-linux-nat.c (ppc_linux_new_thread): Clear the new thread's
+	debug state prior to replicating existing hardware watchpoints or
+	breakpoints.
+
+	* gdb/testsuite/gdb.threads/wp-replication.c: New file.
+	* gdb/testsuite/gdb.threads/wp-replication.exp: New file.
+
 2013-04-17  Yao Qi  <yao@codesourcery.com>
 
 	* top.c (print_gdb_configuration): Print configure-time
Index: gdb-head/gdb/ppc-linux-nat.c
===================================================================
--- gdb-head.orig/gdb/ppc-linux-nat.c	2013-04-17 10:24:52.919499117 +0200
+++ gdb-head/gdb/ppc-linux-nat.c	2013-04-17 19:06:28.070943039 +0200
@@ -2178,7 +2178,21 @@ ppc_linux_new_thread (struct lwp_info *l
       /* Copy that thread's breakpoints and watchpoints to the new thread.  */
       for (i = 0; i < max_slots_number; i++)
 	if (hw_breaks[i].hw_break)
-	  booke_insert_point (hw_breaks[i].hw_break, tid);
+	  {
+	    /* The ppc Linux kernel causes a thread to inherit its parent
+	       thread's debug state, and that includes any hardware
+	       watchpoints or breakpoints that the parent thread may have set.
+
+	       For this reason, the debug state of the new thread is cleared
+	       before trying to replicate any hardware watchpoints or
+	       breakpoints contained in other threads.  */
+
+	    /* The ppc debug resource accounting is done through "slots".
+	       Ask the kernel the deallocate this specific *point's slot.  */
+	    ptrace (PPC_PTRACE_DELHWDEBUG, tid, 0, hw_breaks[i].slot);
+
+	    booke_insert_point (hw_breaks[i].hw_break, tid);
+	  }
     }
   else
     ptrace (PTRACE_SET_DEBUGREG, tid, 0, saved_dabr_value);
Index: gdb-head/gdb/testsuite/gdb.threads/wp-replication.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb-head/gdb/testsuite/gdb.threads/wp-replication.c	2013-04-17 17:52:04.195022357 +0200
@@ -0,0 +1,144 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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/>.
+
+   Check that hardware watchpoints get propagated to all existing
+   threads when the hardware watchpoint is created.
+*/
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <pthread.h>
+
+#ifndef NR_THREADS
+#define NR_THREADS 4
+#endif
+
+#ifndef X_INCR_COUNT
+#define X_INCR_COUNT 10
+#endif
+
+void *thread_function (void *arg); /* Function executed by each thread.  */
+
+/* Used to hold threads back until wp-replication.exp is ready.  */
+int test_ready = 0;
+
+/* Used to push the program out of the waiting loop after the
+   testcase is done counting the number of hardware watchpoints
+   available for our target.  */
+int watch_count_done = 0;
+
+/* Array with elements we can create watchpoints for.  */
+static int watched_data[NR_BYTES];
+pthread_mutex_t data_mutex;
+
+/* Wait function to keep threads busy while the testcase does
+   what it needs to do.  */
+void empty_cycle (void)
+{
+  usleep (1);
+}
+
+int
+main ()
+{
+  int res;
+  pthread_t threads[NR_THREADS];
+  int i;
+
+  while (watch_count_done == 0)
+    {
+      /* GDB will modify the value of "i" at runtime and we will
+  get past this point.  */
+      empty_cycle ();
+    }
+
+  pthread_mutex_init (&data_mutex, NULL);
+
+  for (i = 0; i < NR_THREADS; ++i)
+    {
+      res = pthread_create (&threads[i],
+	 NULL,
+	 thread_function,
+	 (void *) (intptr_t) i);
+      if (res != 0)
+ {
+   fprintf (stderr, "error in thread %d create\n", i);
+   abort ();
+ }
+    }
+
+  for (i = 0; i < NR_THREADS; ++i)
+    {
+      res = pthread_join (threads[i], NULL);
+      if (res != 0)
+ {
+   fprintf (stderr, "error in thread %d join\n", i);
+   abort ();
+ }
+    }
+
+  exit (EXIT_SUCCESS);
+}
+
+/* Easy place for a breakpoint.
+   wp-replication.exp uses this to track when all threads are running
+   instead of, for example, the program keeping track
+   because we don't need the program to know when all threads are running,
+   instead we need gdb to know when all threads are running.
+   There is a delay between when a thread has started and when the thread
+   has been registered with gdb.  */
+
+void
+thread_started ()
+{
+}
+
+void *
+thread_function (void *arg)
+{
+  int i, j;
+  long thread_number = (long) arg;
+
+  thread_started ();
+
+  /* Don't start incrementing X until wp-replication.exp is ready.  */
+  while (!test_ready)
+    usleep (1);
+
+  for (i = 0; i < NR_TRIGGERS_PER_THREAD; ++i)
+    {
+      for (j = 0; j < NR_THREADS; j++)
+ {
+   pthread_mutex_lock (&data_mutex);
+   /* For debugging.  */
+   printf ("Thread %ld changing watch_thread[%d] data"
+     " from %d -> %d\n", thread_number, j,
+     watched_data[j], watched_data[j] + 1);
+   /* The call to usleep is so that when the watchpoint triggers,
+      the pc is still on the same line.  */
+   /* Increment the watched data field.  */
+   ++watched_data[j];
+   usleep (1);
+
+   pthread_mutex_unlock (&data_mutex);
+ }
+    }
+
+  pthread_exit (NULL);
+}
Index: gdb-head/gdb/testsuite/gdb.threads/wp-replication.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gdb-head/gdb/testsuite/gdb.threads/wp-replication.exp	2013-04-17 16:12:01.807129012 +0200
@@ -0,0 +1,141 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2013 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/>.
+
+# Check that hardware watchpoints get replicated to all existing
+# threads when the hardware watchpoint is created.
+
+
+set NR_THREADS 4
+set NR_TRIGGERS_PER_THREAD 10
+set NR_BYTES 100
+
+# This test verifies that a hardware watchpoint gets replicated to
+# every existing thread and is detected properly.  This test is
+# only meaningful on a target with hardware watchpoints.
+if {[skip_hw_watchpoint_tests]} {
+    return 0
+}
+
+standard_testfile
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable [list debug "additional_flags=-DNR_THREADS=$NR_THREADS -DNR_TRIGGERS_PER_THREAD=$NR_TRIGGERS_PER_THREAD -DNR_BYTES=$NR_BYTES"]] != "" } {
+    return -1
+}
+
+clean_restart ${binfile}
+
+# Force hardware watchpoints to be used.
+gdb_test_no_output "set can-use-hw-watchpoints 1" ""
+
+# Run to `main' where we begin our tests.
+if ![runto_main] then {
+    gdb_suppress_tests
+}
+
+# First, break at empty_cycle.
+gdb_test "break empty_cycle" \
+  "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+  "breakpoint on empty_cycle"
+
+# Set some default values.
+set hwatch_count 0
+set done 0
+
+# Count the number of hardware watchpoints available on
+# this target.
+while { $done == 0 } {
+
+  gdb_test "continue" \
+    ".*Breakpoint 2, empty_cycle \\(\\) at .*${srcfile}.*" \
+    "continue to empty_cycle"
+
+  # Some targets do resource counting as we insert watchpoints.
+  # Such targets won't cause a watchpoint insertion failure, but
+  # will switch to software watchpoints silently.  We check for
+  # both cases here.
+  gdb_test_multiple "watch watched_data\[$hwatch_count\]" \
+       "watch watched_data\[$hwatch_count\]" {
+    -re "Hardware watchpoint .*$gdb_prompt $" {
+    }
+    -re "Watchpoint .*$gdb_prompt $" {
+      set done 1
+      break
+    }
+  }
+
+  gdb_test_multiple "continue" "watchpoint created successfully" {
+    -re ".*Breakpoint 2, empty_cycle \\(\\).*$gdb_prompt $" {
+      incr hwatch_count
+    }
+    -re ".*Could not insert hardware watchpoint.*$gdb_prompt $" {
+      set done 1
+      break
+    }
+  }
+}
+
+if { $hwatch_count == 0} {
+  gdb_exit;
+}
+
+# At this point, we know how many hardware watchpoints
+# the target supports.  Use that to do further testing.
+delete_breakpoints
+
+# Break out of the empty_cycle loop by changing the
+# controlling variable.
+gdb_test_no_output "set var watch_count_done=1" \
+      "set var watch_count_done=1"
+
+# Prepare to create all the threads.
+gdb_test "break thread_started" \
+	 "Breakpoint \[0-9\]+ at .*: file .*${srcfile}, line .*" \
+	 "Breakpoint on thread_started"
+
+# Move all threads to where they're supposed to be for testing.
+for { set i 0 } { $i < $NR_THREADS } { incr i} {
+
+    # We want to set the maximum number of hardware watchpoints
+    # and make sure the target can handle that without an error.
+    # That will show us the watchpoints got replicated to all the
+    # threads correctly, and that no new watchpoints got created
+    # in the background for a specific thread.
+    if {$i < $hwatch_count} {
+      gdb_test "watch watched_data\[$i\]" \
+	"Hardware watchpoint .*" \
+	"watch watched_data\[$i\]"
+    }
+
+    gdb_test continue "Continuing\\..*Breakpoint \[0-9\]+, thread_started \\(\\) at .*$srcfile.*" \
+    "Break at thread_started"
+}
+
+# Let the threads run and change the watched data, leading
+# to watchpoint triggers.
+gdb_test_no_output "set var test_ready=1" \
+      "set var test_ready=1"
+
+# Set the number of expected watchpoint triggers.
+set TRIGGERS [expr "$NR_THREADS * $NR_THREADS * $NR_TRIGGERS_PER_THREAD"]
+
+# Move the threads and hit the watchpoints
+# TRIGGERS times.
+for { set i 0 } { $i < $TRIGGERS } { incr i} {
+    gdb_test continue "Continuing\\..*Hardware watchpoint \[0-9\]+: watched_data\[\[0-9\]+\].*Old value = \[0-9\]+.*New value = \[0-9\]+.*thread_function \\(arg=$hex\\) at .*$srcfile.*" \
+    "Continue to watchpoint trigger on watched_data"
+}
+
+gdb_exit