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

[Pedro Alves <palves at redhat dot com>]

On 04/23/2013 07:03 PM, Luis Machado wrote:
> 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.

One idea for less intrusive debugging would be to make it so
the kernel doesn't notified of new threads/clones at all, IOW, not
force parent/child clones to stop to report the child's existence to
a ptracer (unless it wants to).  Having the kernel auto-copy debug
resources in the new child is a requirement for that, and given
ptrace options and other things are copied as well, I can't
really call it broken outright.

I'd suggest just mixing something like

 Older kernels did not make new threads inherit their parent thread's
 debug state, so we always clear the slot and replicate the debug state
 ourselves, ensuring compatibility with all kernels.

into the existing comment.

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

It does not:

+# Run to `main' where we begin our tests.
+if ![runto_main] then {
+    fail "Failed to run to main"
+}
+



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

Let's call "unsupported" or even FAIL then, so it can draw attention.
We could also end up in that situation due to a bug somewhere in the
test or gdb.  Something like 'fail "no hardware watchpoints"'.

> 2013-04-23  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.

Recall that these go to gdb/testsuite/ChangeLog.

To indicate to the reviewer that was not a mistake, I suggest posting
the entry like:

2013-04-23  Luis Machado  <lgustavo@codesourcery.com>

	gdb/
	* 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.threads/wp-replication.exp: New file.

or:

gdb/
2013-04-23  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/
2013-04-23  Luis Machado  <lgustavo@codesourcery.com>

	* gdb.threads/wp-replication.c: New file.
	* gdb.threads/wp-replication.exp: New file.



> +void empty_cycle (void)

'\n' after first void.

> +for { set i 1 } { $i <= $TRIGGERS} { incr i} {
                            -------^    -----^
Missing spaces.

> +# Move the threads and hit the watchpoints
> +# TRIGGERS times.

Nit, that fits in a single line.

Otherwise OK.

Thanks,
-- 
Pedro Alves