[PATCH] disable ptrace BookE interface for PowerPC server processors

[PATCH] disable ptrace BookE interface for PowerPC server processors

[Edjunior Barbosa Machado]

Hi,

the ptrace BookE interface originally implemented only for embedded systems
should be available for Power servers processors as well shortly [1].
However, while this is not usable in upstream kernel, gdb should enforce
the usage of the old ptrace mechanism (using PTRACE_SET_DEBUGREG) on
servers, even if BookE debug registers are reported as available.

This patch fixes several tests related to watchpoints that were failing
when running upstream kernel (> 3.0), decreasing the number of
unexpected failures on gdb testsuite from 337 to 200 (Fedora 17 running
kernel 3.3.4-5.fc17.ppc64).

Ok to apply?

Thanks,
--
Edjunior Barbosa Machado
IBM Linux Technology Center

[1] http://lists.ozlabs.org/pipermail/linuxppc-dev/2012-May/097922.html

gdb/
2012-22-05  Edjunior Machado  <emachado@linux.vnet.ibm.com>

	* ppc-linux-nat.c (have_ptrace_booke_interface): disable ptrace booke
	interface for powerpc server processors while this is not available
	in kernel.

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 34c40b4..5c95926 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1418,8 +1418,11 @@ have_ptrace_booke_interface (void)
       if (tid == 0)
 	tid = PIDGET (inferior_ptid);
 
-      /* Check for kernel support for BOOKE debug registers.  */
-      if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
+      /* Check for kernel support for BOOKE debug registers.
+         This interface is currently available only for embedded
+         processors.  */
+      if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0
+	  && ppc_linux_get_hwcap () & PPC_FEATURE_BOOKE)
 	{
 	  have_ptrace_booke_interface = 1;
 	  max_slots_number = booke_debug_info.num_instruction_bps

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Mark Kettenis]

> From: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> Date: Tue, 22 May 2012 12:24:11 -0300
> 
> Hi,
> 
> the ptrace BookE interface originally implemented only for embedded systems
> should be available for Power servers processors as well shortly [1].
> However, while this is not usable in upstream kernel, gdb should enforce
> the usage of the old ptrace mechanism (using PTRACE_SET_DEBUGREG) on
> servers, even if BookE debug registers are reported as available.

But how are you going to handle this when that functionality does
become available?  

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Edjunior Barbosa Machado]

On 05/22/2012 02:34 PM, Mark Kettenis wrote:

> But how are you going to handle this when that functionality does
> become available?  


Thanks for the feedback and sorry for the late reply. I was discussing
with the kernel developer responsible for the next version of the ptrace
booke interface and he proposed that the 'features' field from the
struct ppc_debug_info returned by ptrace PPC_PTRACE_GETHWDBGINFO call
could be used to check if its availability. According to him, this field
is currently 0 for servers, but once the new ptrace interface becomes
functional, it will return the flag PPC_DEBUG_FEATURE_DATA_BP_RANGE
enabled. Moreover, this change will not affect the behavior on embedded
processors (which already has PPC_DEBUG_FEATURE_DATA_BP_RANGE and
PPC_DEBUG_FEATURE_DATA_BP_MASK enabled).

Please consider the new version of the patch below.

Thanks,
-- 
Edjunior Barbosa Machado
IBM Linux Technology Center

gdb/
2012-25-05  Edjunior Machado  <emachado@linux.vnet.ibm.com>

	* ppc-linux-nat.c (have_ptrace_booke_interface): disable ptrace booke
	interface for powerpc server processors if not available in kernel.

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 34c40b4..9ffcf88 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1421,17 +1421,18 @@ have_ptrace_booke_interface (void)
       /* Check for kernel support for BOOKE debug registers.  */
       if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
 	{
-	  have_ptrace_booke_interface = 1;
-	  max_slots_number = booke_debug_info.num_instruction_bps
-	    + booke_debug_info.num_data_bps
-	    + booke_debug_info.num_condition_regs;
-	}
-      else
-	{
-	  /* Old school interface and no BOOKE debug registers support.  */
-	  have_ptrace_booke_interface = 0;
-	  memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
+	  if (booke_debug_info.features)
+	    {
+	      have_ptrace_booke_interface = 1;
+	      max_slots_number = booke_debug_info.num_instruction_bps
+	        + booke_debug_info.num_data_bps
+	        + booke_debug_info.num_condition_regs;
+	      return have_ptrace_booke_interface;
+	    }
 	}
+      /* Old school interface and no BOOKE debug registers support.  */
+      have_ptrace_booke_interface = 0;
+      memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
     }
 
   return have_ptrace_booke_interface;

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Mark Kettenis]

> Date: Fri, 25 May 2012 00:49:39 -0300
> From: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> 
> On 05/22/2012 02:34 PM, Mark Kettenis wrote:
> 
> > But how are you going to handle this when that functionality does
> > become available?  
> 
> 
> Thanks for the feedback and sorry for the late reply. I was discussing
> with the kernel developer responsible for the next version of the ptrace
> booke interface and he proposed that the 'features' field from the
> struct ppc_debug_info returned by ptrace PPC_PTRACE_GETHWDBGINFO call
> could be used to check if its availability. According to him, this field
> is currently 0 for servers, but once the new ptrace interface becomes
> functional, it will return the flag PPC_DEBUG_FEATURE_DATA_BP_RANGE
> enabled. Moreover, this change will not affect the behavior on embedded
> processors (which already has PPC_DEBUG_FEATURE_DATA_BP_RANGE and
> PPC_DEBUG_FEATURE_DATA_BP_MASK enabled).
> 
> Please consider the new version of the patch below.

Certainly makes more sense to me ;)

> gdb/
> 2012-25-05  Edjunior Machado  <emachado@linux.vnet.ibm.com>
> 
> 	* ppc-linux-nat.c (have_ptrace_booke_interface): disable ptrace booke
> 	interface for powerpc server processors if not available in kernel.
> 
> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index 34c40b4..9ffcf88 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -1421,17 +1421,18 @@ have_ptrace_booke_interface (void)
>        /* Check for kernel support for BOOKE debug registers.  */
>        if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
>  	{
> -	  have_ptrace_booke_interface = 1;
> -	  max_slots_number = booke_debug_info.num_instruction_bps
> -	    + booke_debug_info.num_data_bps
> -	    + booke_debug_info.num_condition_regs;
> -	}
> -      else
> -	{
> -	  /* Old school interface and no BOOKE debug registers support.  */
> -	  have_ptrace_booke_interface = 0;
> -	  memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
> +	  if (booke_debug_info.features)
> +	    {
> +	      have_ptrace_booke_interface = 1;
> +	      max_slots_number = booke_debug_info.num_instruction_bps
> +	        + booke_debug_info.num_data_bps
> +	        + booke_debug_info.num_condition_regs;
> +	      return have_ptrace_booke_interface;
> +	    }
>  	}
> +      /* Old school interface and no BOOKE debug registers support.  */
> +      have_ptrace_booke_interface = 0;
> +      memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
>      }
>  
>    return have_ptrace_booke_interface;
> 
> 

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Edjunior Barbosa Machado]

On 05/29/2012 03:54 PM, Mark Kettenis wrote:

>> Date: Fri, 25 May 2012 00:49:39 -0300
>> From: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
>>
>> On 05/22/2012 02:34 PM, Mark Kettenis wrote:
>>
>>> But how are you going to handle this when that functionality does
>>> become available?  
>>
>>
>> Thanks for the feedback and sorry for the late reply. I was discussing
>> with the kernel developer responsible for the next version of the ptrace
>> booke interface and he proposed that the 'features' field from the
>> struct ppc_debug_info returned by ptrace PPC_PTRACE_GETHWDBGINFO call
>> could be used to check if its availability. According to him, this field
>> is currently 0 for servers, but once the new ptrace interface becomes
>> functional, it will return the flag PPC_DEBUG_FEATURE_DATA_BP_RANGE
>> enabled. Moreover, this change will not affect the behavior on embedded
>> processors (which already has PPC_DEBUG_FEATURE_DATA_BP_RANGE and
>> PPC_DEBUG_FEATURE_DATA_BP_MASK enabled).
>>
>> Please consider the new version of the patch below.
> 
> Certainly makes more sense to me ;)


which means this is ok to apply? :)

Thanks for the review once again, Mark.

-- 
Edjunior

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Mark Kettenis]

> Date: Wed, 30 May 2012 01:16:36 -0300
> From: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> CC: gdb-patches@sourceware.org, prasad@linux.vnet.ibm.com
> X-Content-Scanned: Fidelis XPS MAILER
> x-cbid: 12053004-2362-0000-0000-00000750AC06
> X-XS4ALL-DNSBL-Checked: mxdrop221.xs4all.nl checked 32.104.18.26 against DNS blacklists
> X-CNFS-Analysis: v=2.0 cv=ZbOqwLpA c=1 sm=0 a=nuFXaNQDUkPZN5VKAd7j+w==:17
> 	a=kUSHYda-BbMA:10 a=sYXyRZcBhIwA:10 a=-oxz3-EH24EA:10
> 	a=8nJEP1OIZ-IA:10 a=9tSwX7uLV1UA:10 a=VnNF1IyMAAAA:8
> 	a=d0wQilHS6ga4fH6NGAsA:9 a=wPNLvfGTeEIA:10
> 	a=nuFXaNQDUkPZN5VKAd7j+w==:117
> X-Virus-Scanned: by XS4ALL Virus Scanner
> X-XS4ALL-Spam-Score: 0.0 () none
> X-XS4ALL-Spam: NO
> Envelope-To: mark.kettenis@xs4all.nl
> 
> On 05/29/2012 03:54 PM, Mark Kettenis wrote:
> 
> >> Date: Fri, 25 May 2012 00:49:39 -0300
> >> From: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com>
> >>
> >> On 05/22/2012 02:34 PM, Mark Kettenis wrote:
> >>
> >>> But how are you going to handle this when that functionality does
> >>> become available?  
> >>
> >>
> >> Thanks for the feedback and sorry for the late reply. I was discussing
> >> with the kernel developer responsible for the next version of the ptrace
> >> booke interface and he proposed that the 'features' field from the
> >> struct ppc_debug_info returned by ptrace PPC_PTRACE_GETHWDBGINFO call
> >> could be used to check if its availability. According to him, this field
> >> is currently 0 for servers, but once the new ptrace interface becomes
> >> functional, it will return the flag PPC_DEBUG_FEATURE_DATA_BP_RANGE
> >> enabled. Moreover, this change will not affect the behavior on embedded
> >> processors (which already has PPC_DEBUG_FEATURE_DATA_BP_RANGE and
> >> PPC_DEBUG_FEATURE_DATA_BP_MASK enabled).
> >>
> >> Please consider the new version of the patch below.
> > 
> > Certainly makes more sense to me ;)
> 
> 
> which means this is ok to apply? :)

Fine with me, but I've never (knowingly) used Linux on
POWER/PowerPC/PowerISA ;).

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Sergio Durigan Junior]

On Friday, May 25 2012, Edjunior Barbosa Machado wrote:

> On 05/22/2012 02:34 PM, Mark Kettenis wrote:
>
> Thanks for the feedback and sorry for the late reply. I was discussing
> with the kernel developer responsible for the next version of the ptrace
> booke interface and he proposed that the 'features' field from the
> struct ppc_debug_info returned by ptrace PPC_PTRACE_GETHWDBGINFO call
> could be used to check if its availability. According to him, this field
> is currently 0 for servers, but once the new ptrace interface becomes
> functional, it will return the flag PPC_DEBUG_FEATURE_DATA_BP_RANGE
> enabled. Moreover, this change will not affect the behavior on embedded
> processors (which already has PPC_DEBUG_FEATURE_DATA_BP_RANGE and
> PPC_DEBUG_FEATURE_DATA_BP_MASK enabled).

Thanks for the patch.

> Please consider the new version of the patch below.

> gdb/
> 2012-25-05  Edjunior Machado  <emachado@linux.vnet.ibm.com>
>
> 	* ppc-linux-nat.c (have_ptrace_booke_interface): disable ptrace booke
> 	interface for powerpc server processors if not available in
> 	kernel.

I guess you could rewrite this to:

        * ppc-linux-nat.c (have_ptrace_booke_interface): Disable ptrace
          BookE interface for PowerPC server processors if not available
          in the Linux kernel.

> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index 34c40b4..9ffcf88 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -1421,17 +1421,18 @@ have_ptrace_booke_interface (void)
>        /* Check for kernel support for BOOKE debug registers.  */
>        if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
>  	{
> -	  have_ptrace_booke_interface = 1;
> -	  max_slots_number = booke_debug_info.num_instruction_bps
> -	    + booke_debug_info.num_data_bps
> -	    + booke_debug_info.num_condition_regs;
> -	}
> -      else
> -	{
> -	  /* Old school interface and no BOOKE debug registers support.  */
> -	  have_ptrace_booke_interface = 0;
> -	  memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));


> +	  if (booke_debug_info.features)

I'd rather use an explicit check like this:

          if (booke_debug_info.features != 0)

Also, I guess you could put a comment above this check and insert a
brief explanation like you did in the top of this message, WDYT?

From what I remember when I hacked this interface, the patch is good
(and somewhat trivial to me) :-).

Thanks,

-- 
Sergio

Re: [PATCH] disable ptrace BookE interface for PowerPC server processors

[Edjunior Barbosa Machado]

On 05/31/2012 05:55 PM, Sergio Durigan Junior wrote:

> On Friday, May 25 2012, Edjunior Barbosa Machado wrote:
>> gdb/
>> 2012-25-05  Edjunior Machado  <emachado@linux.vnet.ibm.com>
>>
>> 	* ppc-linux-nat.c (have_ptrace_booke_interface): disable ptrace booke
>> 	interface for powerpc server processors if not available in
>> 	kernel.
> 
> I guess you could rewrite this to:
> 
>         * ppc-linux-nat.c (have_ptrace_booke_interface): Disable ptrace
>           BookE interface for PowerPC server processors if not available
>           in the Linux kernel.
> 
>> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
>> index 34c40b4..9ffcf88 100644
>> --- a/gdb/ppc-linux-nat.c
>> +++ b/gdb/ppc-linux-nat.c
>> @@ -1421,17 +1421,18 @@ have_ptrace_booke_interface (void)
>>        /* Check for kernel support for BOOKE debug registers.  */
>>        if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
>>  	{
>> -	  have_ptrace_booke_interface = 1;
>> -	  max_slots_number = booke_debug_info.num_instruction_bps
>> -	    + booke_debug_info.num_data_bps
>> -	    + booke_debug_info.num_condition_regs;
>> -	}
>> -      else
>> -	{
>> -	  /* Old school interface and no BOOKE debug registers support.  */
>> -	  have_ptrace_booke_interface = 0;
>> -	  memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
> 
> 
>> +	  if (booke_debug_info.features)
> 
> I'd rather use an explicit check like this:
> 
>           if (booke_debug_info.features != 0)
> 
> Also, I guess you could put a comment above this check and insert a
> brief explanation like you did in the top of this message, WDYT?
> 
> From what I remember when I hacked this interface, the patch is good
> (and somewhat trivial to me) :-).
> 
> Thanks,
> 


Thanks for the review, Sergio.

Based on your reply and the feedback from Mark, I've just committed the
following version of the patch.

Thanks,
-- 
Edjunior

gdb/
2012-05-31  Edjunior Machado  <emachado@linux.vnet.ibm.com>

	* ppc-linux-nat.c (have_ptrace_booke_interface): Disable ptrace
	BookE interface for PowerPC server processors if not available
	in the Linux Kernel.

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index b9e0c85..45cdd73 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1421,17 +1421,20 @@ have_ptrace_booke_interface (void)
       /* Check for kernel support for BOOKE debug registers.  */
       if (ptrace (PPC_PTRACE_GETHWDBGINFO, tid, 0, &booke_debug_info) >= 0)
 	{
-	  have_ptrace_booke_interface = 1;
-	  max_slots_number = booke_debug_info.num_instruction_bps
-	    + booke_debug_info.num_data_bps
-	    + booke_debug_info.num_condition_regs;
-	}
-      else
-	{
-	  /* Old school interface and no BOOKE debug registers support.  */
-	  have_ptrace_booke_interface = 0;
-	  memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
+	  /* Check whether ptrace BOOKE interface is functional and
+	     provides any supported feature.  */
+	  if (booke_debug_info.features != 0)
+	    {
+	      have_ptrace_booke_interface = 1;
+	      max_slots_number = booke_debug_info.num_instruction_bps
+	        + booke_debug_info.num_data_bps
+	        + booke_debug_info.num_condition_regs;
+	      return have_ptrace_booke_interface;
+	    }
 	}
+      /* Old school interface and no BOOKE debug registers support.  */
+      have_ptrace_booke_interface = 0;
+      memset (&booke_debug_info, 0, sizeof (struct ppc_debug_info));
     }

   return have_ptrace_booke_interface;