Re: [PATCH] Fix hardware watchpoints on PowerPC servers

Edjunior Barbosa Machado
2013-05-13T14:16:15Z

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