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

[Luis Gustavo]

Hi Jan,

On 08/06/2012 02:04 PM, Jan Kratochvil wrote:
> On Mon, 06 Aug 2012 16:33:58 +0200, Luis Gustavo wrote:
>> With the somewhat recent booke kernel interface, more hw
>> watchpoints/breakpoints are available to GDB.
>
> Which hardware can be BookE tested on?  Red Hat has available for example
> PPC970FX, POWER5, POWER7 etc. but those AFAIK are not BookE.  Or does it
> depend just on kernel?

The ones you listed are server processors, and they use the old kernel 
interface. Examples of BookE's are the 4xx family (405, 440, 460, 476 
etc...)
>
>
>> 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.
>
> I have been testing it recently for fork() (processes, no threads) and my
> results were:
>
> kernel-3.3.4-5.fc17.ppc64 clears debug info in both parent (!) and child
> kernel-2.6.18-308.el5.ppc64 copies debug info to child and keeps it in parent

Interesting. The kernel i was trying things on was 2.6.3x.

The behavior you mention in 3.3.4 sounds odd to me. It would be nice to 
hear from the kernel folks if this is expected.

Also, the ppc server kernel may differ from the booke one.

>
>
>> 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.
>
> Yes, I agree GDB should cope with all these kinds of behavior.
>
>
>> --- 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);
>
> I agree with the patch.
>
> I was considering to call PPC_PTRACE_DELHWDEBUG unconditionally but that would
> be probably an unneeded overhead.
>
> Did you test that kernel really does not reorder the [i] slots during
> inheritance into the new thread?

The tests i did indicate that the child thread inherits whatever slot is 
in use by the parent thread, and no reordering appears to happen. 
Otherwise we would need to do a complete cleanup of the debug state in GDB.

What i saw with hardware that has 2 watchpoint slots (440):

* Thread 1 creates a hw watch using slot 5 (first DAC register).
* Thread 1 creates Thread 2.
* GDB proceeds to replicate the hw watch from Thread 1 in Thread 2.
* The kernel replies with slot 6, which indicates that a new DAC has 
been used (and also indicates slot 5 is currently in use)
* Thread 2 creates Thread 3
* GDB proceeds to replicate both hw watches from Threads 1 and 2  in 
Thread 3.
* Kernel replies -1, indicating there are no more slots left to create a 
new hw watchpoint. This shows slots 5 and 6 are in use in Thread 3.

>
>
>> +	  }
>>       }
>>     else
>>       ptrace (PTRACE_SET_DEBUGREG, tid, 0, saved_dabr_value);
>
>
>> This would be nice to include before 7.5, as it's an annoying problem.
>
> Does this fix affect existing testcases on BookE?
> Otherwise a new testcase would be appropriate.

I don't think it does.  Hardware watchpoints haven't been reliably 
tested for embedded ppc's due to the variaty of debugging features 
between cpu's. Additionaly, this only shows up when using hw watchpoints 
with threaded inferiors.

An appropriate test would be to run many threads and create the maximum 
amount of hw watchpoints and see if that works fine, but GDB's debug 
resource accounting is still a little awkward

Luis