Re: [PATCH] Fix hardware watchpoints on PowerPC servers

[Luis Machado <lgustavo at codesourcery dot com>]

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.

>         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?

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

Thanks!
Luis