Re: [PATCH] Fix hardware watchpoints on PowerPC servers

Luis Machado
2013-05-13T07:08:40Z

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