[PATCH, ppc] Fix hw *points for embedded ppc in a threaded environment
Luis Machado
lgustavo@codesourcery.com
Wed Apr 24 06:28:00 GMT 2013
On 04/23/2013 06:16 PM, Pedro Alves wrote:
> On 04/23/2013 02:30 PM, Luis Machado wrote:
>> /* 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.
>
> ISTR there was a point when the kernel changed behavior, right?
> It'd be good to mention the kernel version here. I _think_ you wanted
> to retain compatibility with older kernels, but, alas, the comment
> doesn't mention that.
I can't state what version of the kernel had the behavior changed. I
cc-ed the kernel folks in the original mail, but we received no answers
back.
>
> (Also, I'm left wondering if we couldn't detect the need for this
> just once, and skip several ptrace calls if we find the kernel
> does this for us already.)
We probably could do that. The question is whether it brings us a great
benefit compared to what we do now. Ideally the kernel developers would
fix what is broken in my opinion.
>
>> +
>> + 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);
>> + }
>
>> 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-23 15:29:10.518508221 +0200
>> @@ -0,0 +1,160 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> + Copyright 2013 Free Software Foundation, Inc.
>
> I now notice this was largely based on watchthreads2.c/exp. Please
> retain the copyright years.
Done.
>
>> +
>> + 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.
>
> Hmm. watchthreads2.exp has the exact same comment. Can we have some more
> info here on what is supposed to be different between this and
> watchthreads2.exp?
Not a lot of difference here, except that we test for the specific
scenario of adding one watchpoint per thread and checking if that
succeeds until we reach the maximum count (something related to per-slot
hw *points for powerpc).
The testcase actually tests more than that. Maybe it is a more reliable
hardware watchpoint test for targets, as it properly finds out the
number of hardware watchpoints a target supports.
I could probably expand it to test other kinds of watchpoints and
variations. But maybe another day. :-)
>
>> +*/
>> +
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +#include <pthread.h>
>> +
>> +#ifndef NR_THREADS
>> +#define NR_THREADS 4 /* Set by the testcase. */
>> +#endif
>> +
>> +#ifndef X_INCR_COUNT
>> +#define X_INCR_COUNT 10 /* Set by the testcase. */
>> +#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 hold threads back until every thread has had a chance of causing
>> + a watchpoint trigger. This prevents a situation in GDB where it may miss
>> + watchpoint triggers when threads exit while other threads are causing
>> + watchpoint triggers. */
>> +int can_terminate = 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;
>> +
>> +/* Number of watchpoints GDB is capable of using (this is provided
>> + by GDB during the test run). */
>> +int hw_watch_count = 0;
>> +
>> +/* Array with elements we can create watchpoints for. */
>> +static int watched_data[NR_THREADS];
>> +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)
Done.
>
>> +{
>> +}
>> +
>> +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);
>> +
>> + pthread_mutex_lock (&data_mutex);
>> +
>> + for (i = 0; i < NR_TRIGGERS_PER_THREAD; i++)
>> + {
>> + for (j = 0; j < hw_watch_count; j++)
>> + {
>> + /* 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. */
>
> This comment made sense in watchthreads2.c, which reads:
>
> printf ("Thread %ld changing x %d -> %d\n", (long) arg, x, x + 1);
> /* The call to usleep is so that when the watchpoint triggers,
> the pc is still on the same line. */
> ++x; usleep (1); /* X increment. */
>
> but the usleep is no longer on the same line... Do we still need it,
> even?
No. Removed from the code.
>
>> + /* Increment the watched data field. */
>> + watched_data[j]++;
>> + usleep (1);
>> + }
>> + }
>> +
>> + pthread_mutex_unlock (&data_mutex);
>> +
>> + /* Hold the threads here to work around a problem GDB has evaluating
>> + watchpoints right when a DSO event shows up. Sleep a little longer
>
> s/shows up/shows up (PR breakpoints/10116)/
>
> I'd s/longer// ("longer than what?")
I've added more context on this.
>
>> + to avoid consuming lots of cycles. */
>> + while (!can_terminate)
>> + usleep (100);
>> +
>> + 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-23 15:13:16.230525175 +0200
>> @@ -0,0 +1,148 @@
>> +# 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.
>
> Ditto.
>
Done.
>> +
>> +set NR_THREADS 10
>> +set NR_TRIGGERS_PER_THREAD 2
>> +
>> +# 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 watchpoint support.
>> +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"]] != "" } {
>> + 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
>
> Avoid gdb_suppress_tests. Just fail/return as usual.
>
Done. It just returns 0 now.
>> +}
>> +
>> +# 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
>> + }
>> + }
>> +}
>> +
>> +# Target cannot insert hardware watchpoints. Maybe it should have reported
>> +# it does not supported them in the first place.
>
> "does not support them". What does does this "maybe" note mean though?
> A bug? Where?
>
It means that whoever has configured the testsuite for this target did
not clearly specify it does not support hardware watchpoints (in
gdbserver, for example).
In practice, it means we will succeed in executing the first part of the
test, but we will exit due to the number of available hardware
watchpoints being 0.
>> +if { $hwatch_count == 0} {
>> + gdb_exit;
>> +}
>
> I don't understand this. Exit gdb, but carry on?
>
Thinko, this should just return 0.
>> +
>> +# Set the testcase's internal variable indicating the number of
>> +# hardware watchpoints the target supports.
>> +gdb_test_no_output "set var hw_watch_count=${hwatch_count}" \
>> + "set var hw_watch_count=${hwatch_count}"
>> +
>> +# 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\]"
>> + } else {
>> + verbose -log "Not setting watchpoint for watched_data\[$i\]\n"
>> + }
>> +
>> + 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 * $hwatch_count * $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 ${i} out of ${TRIGGERS} on watched_data"
>> +}
>> +
>
>
>> +gdb_exit
>
> Not necessary.
Done.
>
>
> The test has repeated messages in gdb.sum:
>
> http://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique
>
>
> Otherwise looks fine, and I confirm I no longer see racy FAILs either.
>
> Thanks,
>
Seems to be fixed now with unique messages and more numbers to make them
unique.
Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hw_watch.diff
Type: text/x-patch
Size: 12314 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20130424/df4b61b1/attachment.bin>
More information about the Gdb-patches
mailing list