* [PATCH] Fix hardware watchpoints on PowerPC servers
@ 2013-05-13 6:29 Edjunior Barbosa Machado
2013-05-13 7:08 ` Luis Machado
2013-05-13 13:19 ` Ulrich Weigand
0 siblings, 2 replies; 6+ messages in thread
From: Edjunior Barbosa Machado @ 2013-05-13 6:29 UTC (permalink / raw)
To: gdb-patches; +Cc: Ulrich Weigand
Hi,
initially developed only for bookE (embedded processors), the new ptrace
interface is now available for Power server processors too [1].
This patch fixes the insertion of hardware watchpoints on Power servers
using this new interface available on kernel 3.7 and newer (currently, the
region should be aligned to 8 bytes, which is also the max length). It reduces
the number of unexpected failures in gdb testsuite in 40 failures (ppc64
running kernel 3.8).
Thanks and regards,
--
Edjunior.
[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/arch/powerpc/kernel/ptrace.c?id=6c7a2856ade6a58c20038024247d16a12a8d2323
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. */
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)
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))
return 0;
}
/* addr+len must fall in the 8 byte watchable region for DABR-based
--
1.7.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix hardware watchpoints on PowerPC servers
2013-05-13 6:29 [PATCH] Fix hardware watchpoints on PowerPC servers Edjunior Barbosa Machado
@ 2013-05-13 7:08 ` Luis Machado
2013-05-13 14:16 ` Edjunior Barbosa Machado
2013-05-13 13:19 ` Ulrich Weigand
1 sibling, 1 reply; 6+ messages in thread
From: Luis Machado @ 2013-05-13 7:08 UTC (permalink / raw)
To: Edjunior Barbosa Machado; +Cc: gdb-patches, Ulrich Weigand
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fix hardware watchpoints on PowerPC servers
2013-05-13 7:08 ` Luis Machado
@ 2013-05-13 14:16 ` Edjunior Barbosa Machado
0 siblings, 0 replies; 6+ messages in thread
From: Edjunior Barbosa Machado @ 2013-05-13 14:16 UTC (permalink / raw)
To: lgustavo; +Cc: gdb-patches, Ulrich Weigand
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix hardware watchpoints on PowerPC servers
2013-05-13 6:29 [PATCH] Fix hardware watchpoints on PowerPC servers Edjunior Barbosa Machado
2013-05-13 7:08 ` Luis Machado
@ 2013-05-13 13:19 ` Ulrich Weigand
2013-05-13 14:17 ` Edjunior Barbosa Machado
1 sibling, 1 reply; 6+ messages in thread
From: Ulrich Weigand @ 2013-05-13 13:19 UTC (permalink / raw)
To: Edjunior Barbosa Machado; +Cc: gdb-patches
Edjunior Machado wrote:
> initially developed only for bookE (embedded processors), the new ptrace
> interface is now available for Power server processors too [1].
>
> This patch fixes the insertion of hardware watchpoints on Power servers
> using this new interface available on kernel 3.7 and newer (currently, the
> region should be aligned to 8 bytes, which is also the max length). It reduces
> the number of unexpected failures in gdb testsuite in 40 failures (ppc64
> running kernel 3.8).
So the kernel now reports it supports PPC_DEBUG_FEATURE_DATA_BP_RANGE,
but then rejects any attempt to set a range BP of more than 8 bytes?
And there's no way to check for this ahead of time except for checking
the BookE hwcaps bits? That seems unfortunate ...
But I guess if that's the way the kernel interface is now, we'll have to
add that check.
> * 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.
This is OK.
As noted by Luis, it would be good to rename the "booke" terminology,
maybe using something like "have_ptrace_hwdebug" instead of
have_ptrace_booke_interface. But that should be a separate patch.
Thanks,
Ulrich
--
Dr. Ulrich Weigand
GNU Toolchain for Linux on System z and Cell BE
Ulrich.Weigand@de.ibm.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix hardware watchpoints on PowerPC servers
2013-05-13 13:19 ` Ulrich Weigand
@ 2013-05-13 14:17 ` Edjunior Barbosa Machado
2013-05-17 23:09 ` Edjunior Barbosa Machado
0 siblings, 1 reply; 6+ messages in thread
From: Edjunior Barbosa Machado @ 2013-05-13 14:17 UTC (permalink / raw)
To: Ulrich Weigand; +Cc: gdb-patches
On 05/13/2013 10:19 AM, Ulrich Weigand wrote:
> So the kernel now reports it supports PPC_DEBUG_FEATURE_DATA_BP_RANGE,
> but then rejects any attempt to set a range BP of more than 8 bytes?
> And there's no way to check for this ahead of time except for checking
> the BookE hwcaps bits? That seems unfortunate ...
>
> But I guess if that's the way the kernel interface is now, we'll have to
> add that check.
Yes, unfortunately this is the info we have from the kernel interface.
booke_debug_info.data_bp_alignment informs the alignment, which, in the
case of current bookS processors, is also the max length for the
watchpoint (8 bytes).
>
>> * 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.
>
> This is OK.
>
> As noted by Luis, it would be good to rename the "booke" terminology,
> maybe using something like "have_ptrace_hwdebug" instead of
> have_ptrace_booke_interface. But that should be a separate patch.
I'll work on a patch following these suggestions you and Luis mentioned.
Thanks for the feedback, Ulrich.
--
Edjunior
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix hardware watchpoints on PowerPC servers
2013-05-13 14:17 ` Edjunior Barbosa Machado
@ 2013-05-17 23:09 ` Edjunior Barbosa Machado
0 siblings, 0 replies; 6+ messages in thread
From: Edjunior Barbosa Machado @ 2013-05-17 23:09 UTC (permalink / raw)
To: Ulrich Weigand; +Cc: gdb-patches
On 05/13/2013 11:16 AM, Edjunior Barbosa Machado wrote:
> On 05/13/2013 10:19 AM, Ulrich Weigand wrote:
>> So the kernel now reports it supports PPC_DEBUG_FEATURE_DATA_BP_RANGE,
>> but then rejects any attempt to set a range BP of more than 8 bytes?
>> And there's no way to check for this ahead of time except for checking
>> the BookE hwcaps bits? That seems unfortunate ...
>>
>> But I guess if that's the way the kernel interface is now, we'll have to
>> add that check.
>
> Yes, unfortunately this is the info we have from the kernel interface.
> booke_debug_info.data_bp_alignment informs the alignment, which, in the
> case of current bookS processors, is also the max length for the
> watchpoint (8 bytes).
>
>>
>>> * 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.
>>
>> This is OK.
>>
>> As noted by Luis, it would be good to rename the "booke" terminology,
>> maybe using something like "have_ptrace_hwdebug" instead of
>> have_ptrace_booke_interface. But that should be a separate patch.
>
> I'll work on a patch following these suggestions you and Luis mentioned.
>
> Thanks for the feedback, Ulrich.
>
Checked in: http://sourceware.org/ml/gdb-cvs/2013-05/msg00146.html
Thanks.
--
Edjunior
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-17 23:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-13 6:29 [PATCH] Fix hardware watchpoints on PowerPC servers Edjunior Barbosa Machado
2013-05-13 7:08 ` Luis Machado
2013-05-13 14:16 ` Edjunior Barbosa Machado
2013-05-13 13:19 ` Ulrich Weigand
2013-05-13 14:17 ` Edjunior Barbosa Machado
2013-05-17 23:09 ` Edjunior Barbosa Machado
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox