Re: [PATCH] Fix hardware watchpoints on PowerPC servers

[Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>]

On 05/13/2013 04:08 AM, Luis Machado wrote:
> Hi,
> 
> As a general thought, the generic ptrace interface for powerpc hardware
> debugging resources was limited to the BOOK E processors. Since it is no
> longer the case, using the BOOK E naming throughout the code looks
> confusing now.
> 
> On 05/13/2013 08:28 AM, Edjunior Barbosa Machado wrote:
>> gdb/ChangeLog
>> 2013-05-12  Edjunior Machado  <emachado@linux.vnet.ibm.com>
>>
>>     * ppc-linux-nat.c (ppc_linux_region_ok_for_hw_watchpoint): Check
>> if the
>>     region is ok for a hardware watchpoint using the new ptrace interface
>>     on Power servers.
>>
>> ---
>>   gdb/ppc-linux-nat.c |   19 +++++++++++--------
>>   1 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
>> index 280dcbe..1ff00a6 100644
>> --- a/gdb/ppc-linux-nat.c
>> +++ b/gdb/ppc-linux-nat.c
>> @@ -1503,16 +1503,19 @@ ppc_linux_region_ok_for_hw_watchpoint
>> (CORE_ADDR addr, int len)
>>        to determine the hardcoded watchable region for watchpoints.  */
>>     if (have_ptrace_booke_interface ())
>>       {
>> -      /* DAC-based processors (i.e., embedded processors), like the
>> PowerPC 440
>> -     have ranged watchpoints and can watch any access within an
>> arbitrary
>> -     memory region.  This is useful to watch arrays and structs, for
>> -     instance.  It takes two hardware watchpoints though.  */
>> +      /* Embedded DAC-based processors, like the PowerPC 440 have ranged
>> +     watchpoints and can watch any access within an arbitrary memory
>> +     region. This is useful to watch arrays and structs, for
>> instance.  It
>> +         takes two hardware watchpoints though.  */
> 
> Any special reason this comment was tweaked? It does not seem to add
> more substantial information.

I tried to highlight that the if() below was focused on embedded
processors ("ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE").

>>         if (len > 1
>> -      && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE)
>> +      && booke_debug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE
>> +      && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
> 
> This bit, though correct, looks confusing now. We are dealing with a
> structure named booke_debug_info, but we are checking
> "ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE" to make sure we are really
> dealing with a BOOK-E processor now.
> 
> I think people will eventually scratch their heads when they get to this
> point.
> 
> We should probably rename the structure to something more generic now
> that this is no longer BOOK E-specific and make it clear that we are
> dealing with either BOOK E or BOOK S processors (maybe even explicitly
> mentioning IBM's POWER processors).
> 
> Are we also handling 64-bit DABR-based PowerPC processors like the 970?

Yes, this new ptrace interface is now a common interface that deals with
DAC and DABR-based processors.

>>       return 2;
>> -      else if (booke_debug_info.data_bp_alignment
>> -           && (addr + len > (addr &
>> ~(booke_debug_info.data_bp_alignment - 1))
>> -           + booke_debug_info.data_bp_alignment))
>> +      /* Server processors provide one hardware watchpoint and
>> addr+len should
>> +         fall in the watchable region provided by the ptrace
>> interface.  */
>> +      if (booke_debug_info.data_bp_alignment
>> +      && (addr + len > (addr & ~(booke_debug_info.data_bp_alignment -
>> 1))
>> +          + booke_debug_info.data_bp_alignment))
> 
> Similarly, we're dealing with a server processor in this chunk, but it
> is not clear due to the naming.
> 
> While going through this code, I wonder if we should extract these
> alignment checks and put them inside functions with more meaningful
> names. As is, they can get confusing.
> 
> It doesn't need to be in this patch though.

Agree with you. I believe all these functions and structures prefixed by
booke_* are quite confusing now, and they might be renamed to something
more significant, since this interface is no longer related only to
embedded processors now. I'll work on a separate patch for this.

Thank you for the review, Luis.
-- 
Edjunior