Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
* [PATCH, ppc] Fix hw *points for embedded ppc in a threaded environment.
@ 2012-08-06 14:33 Luis Gustavo
  2012-08-06 18:40 ` Jan Kratochvil
  2012-12-27 19:50 ` Luis Machado
  0 siblings, 2 replies; 24+ messages in thread
From: Luis Gustavo @ 2012-08-06 14:33 UTC (permalink / raw)
  To: gdb-patches; +Cc: prasad, benh

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

Hi,

GDB has always assumed that hardware watchpoints and breakpoints should 
be replicated for every new thread in ppc. This worked fine for the old 
DABR-based mechanism since both server and embedded ppc's supported only 
a single hw watchpoint or breakpoint.

With the somewhat recent booke kernel interface, more hw 
watchpoints/breakpoints are available to GDB.

The logic of replicating the existing process' debug state to the new 
thread is still there though, but the new booke interface in the kernel 
already replicates that state. More precisely, the kernel gives the new 
thread the debug state of its parent thread.

When GDB tries to replicate the debug state, it will actually cause the 
kernel to allocate a new hw *point entry, leading to inadequate 
consumption of hw debug resources.

It's still unclear if the kernel is supposed to do this and i'm chasing 
answers with the ppc linux kernel folks 
(https://lists.ozlabs.org/pipermail/linuxppc-dev/2012-August/100083.html). 
Nonetheless, the kernel is out and it has such behavior.

This patch tries to address this problem by clearing any debug state 
prior to replicating *points to the new thread. If the kernel is doing 
something it's not supposed to, then this is a workaround for the broken 
kernels.

This would be nice to include before 7.5, as it's an annoying problem.

OK?

Regards,
Luis

[-- Attachment #2: 0001-fix_ppc_hw_watch.diff --]
[-- Type: text/x-patch, Size: 1467 bytes --]

2012-08-06  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.

Index: gdb_head/gdb/ppc-linux-nat.c
===================================================================
--- gdb_head.orig/gdb/ppc-linux-nat.c	2012-08-06 11:02:12.538532628 -0300
+++ gdb_head/gdb/ppc-linux-nat.c	2012-08-06 11:04:38.486536320 -0300
@@ -2179,7 +2179,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);

^ permalink raw reply	[flat|nested] 24+ messages in thread
* [PATCH, ppc] Fix hw *points for embedded ppc in a threaded environment
@ 2013-04-17 20:33 Luis Machado
  2013-04-18  0:19 ` Sergio Durigan Junior
  2013-04-22 21:09 ` Luis Machado
  0 siblings, 2 replies; 24+ messages in thread
From: Luis Machado @ 2013-04-17 20:33 UTC (permalink / raw)
  To: 'gdb-patches@sourceware.org'

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

Hi,

This is an updated version of the patch i sent a while ago. It addresses 
Pedro's comments and hopefully is OK now.

I renamed the testcase to something more meaningful than watchthreads3.

If this doesn't delay the release, it would be nice to include it since 
it is an annoying issue for embedded ppc targets.

Regards,
Luis

[-- Attachment #2: hw_watch.diff --]
[-- Type: text/x-patch, Size: 10930 bytes --]

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 11:12:20.731448515 +0200
@@ -2179,6 +2179,21 @@ ppc_linux_new_thread (struct lwp_info *l
       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

^ permalink raw reply	[flat|nested] 24+ messages in thread
end of thread, other threads:[~2013-05-07  7:44 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-06 14:33 [PATCH, ppc] Fix hw *points for embedded ppc in a threaded environment Luis Gustavo
2012-08-06 18:40 ` Jan Kratochvil
2012-08-06 19:02   ` Luis Gustavo
2012-08-06 20:28     ` Jan Kratochvil
2012-08-07 14:55       ` Luis Gustavo
2012-08-07 15:00         ` Jan Kratochvil
2012-08-07 15:02           ` Luis Gustavo
2012-12-27 19:50 ` Luis Machado
2013-01-22 12:12   ` Pedro Alves
2013-04-17 20:33 Luis Machado
2013-04-18  0:19 ` Sergio Durigan Junior
2013-04-18  0:51   ` Luis Machado
2013-04-22 21:09 ` Luis Machado
2013-04-22 21:09   ` Pedro Alves
2013-04-23  0:05     ` Pedro Alves
2013-04-23  1:15       ` Pedro Alves
2013-04-23 20:25         ` Luis Machado
2013-04-23 23:18         ` Luis Machado
2013-04-24  3:19           ` Pedro Alves
2013-04-24  6:28             ` Luis Machado
2013-04-24 19:04               ` Pedro Alves
2013-04-25 14:13                 ` Luis Machado
2013-05-07  7:44                   ` Luis Machado
2013-04-24  4:36           ` Pedro Alves

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox