diff mbox

[RFC] Fix SW breakpoint handling for Cell multi-arch

Message ID 20150827162324.33ACF39FA@oc7340732750.ibm.com
State New
Headers show

Commit Message

Ulrich Weigand Aug. 27, 2015, 4:23 p.m. UTC
Pedro Alves wrote:
> On 08/27/2015 12:54 PM, Ulrich Weigand wrote:
> > Hi Pedro,
> > 
> > a second major issue with Cell multi-arch debugging right now is related
> > to the new target-side SW breakpoint handling.  Cell uses linux-nat as
> > primary target for the PowerPC side, which now returns true from the
> > to_supports_stopped_by_sw_breakpoint hook.
> > 
> > This works fine for the PowerPC side.  However, when a breakpoint on the
> > SPU side is hit, the kernel does *not* provide a siginfo with TRAP_BRKPT,
> > but instead simply delivers a SIGTRAP without siginfo.
> 
> Does si_code indicate that it was a kernel-generated SIGTRAP (that is,
> SI_KERNEL)?  Wondering whether that would still be distinguishable
> from trace/single-step traps and user sent SIGTRAPs.  See comment and
> table about x86's si_code in nat/linux-nat.h.  I don't know whether
> the SPU has to care about all the cases there, but I suspect
> not (e.g., I'd assume SPU code can't exec?).

That's an interesting idea.  Indeed the kernel uses SI_KERNEL for
SIGTRAPs indicating SW breakpoints on SPU, but nowhere else in all
of PowerPC code.  This means simply accepting either TRAP_BRKPT or
SI_KERNEL should work.  And indeed the patch appended below works
just as well as the original patch for me.

> If not, then we'll have to cope... :-/ .  Any chance the kernel gets
> fixed, in order for some future gdb stop worrying about this?  I was
> hoping to get rid of the moribund locations heuristic at some point.

There's probably no chance of changing the kernel at this point; Cell
is really just in maintenance mode at this point (the only supported
OS is RHEL 5).

Bye,
Ulrich

Comments

Pedro Alves Aug. 27, 2015, 4:44 p.m. UTC | #1
On 08/27/2015 05:23 PM, Ulrich Weigand wrote:
> Pedro Alves wrote:
>> On 08/27/2015 12:54 PM, Ulrich Weigand wrote:
>>> Hi Pedro,
>>>
>>> a second major issue with Cell multi-arch debugging right now is related
>>> to the new target-side SW breakpoint handling.  Cell uses linux-nat as
>>> primary target for the PowerPC side, which now returns true from the
>>> to_supports_stopped_by_sw_breakpoint hook.
>>>
>>> This works fine for the PowerPC side.  However, when a breakpoint on the
>>> SPU side is hit, the kernel does *not* provide a siginfo with TRAP_BRKPT,
>>> but instead simply delivers a SIGTRAP without siginfo.
>>
>> Does si_code indicate that it was a kernel-generated SIGTRAP (that is,
>> SI_KERNEL)?  Wondering whether that would still be distinguishable
>> from trace/single-step traps and user sent SIGTRAPs.  See comment and
>> table about x86's si_code in nat/linux-nat.h.  I don't know whether
>> the SPU has to care about all the cases there, but I suspect
>> not (e.g., I'd assume SPU code can't exec?).
> 
> That's an interesting idea.  Indeed the kernel uses SI_KERNEL for
> SIGTRAPs indicating SW breakpoints on SPU, but nowhere else in all
> of PowerPC code.  This means simply accepting either TRAP_BRKPT or
> SI_KERNEL should work.  And indeed the patch appended below works
> just as well as the original patch for me.

Excellent!

> Index: binutils-gdb/gdb/gdbserver/linux-low.c
> ===================================================================
> --- binutils-gdb.orig/gdb/gdbserver/linux-low.c
> +++ binutils-gdb/gdb/gdbserver/linux-low.c
> @@ -651,7 +651,7 @@ check_stopped_by_breakpoint (struct lwp_
>      {
>        if (siginfo.si_signo == SIGTRAP)
>  	{
> -	  if (siginfo.si_code == GDB_ARCH_TRAP_BRKPT)
> +	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
>  	    {
>  	      if (debug_threads)
>  		{
> Index: binutils-gdb/gdb/linux-nat.c
> ===================================================================
> --- binutils-gdb.orig/gdb/linux-nat.c
> +++ binutils-gdb/gdb/linux-nat.c
> @@ -2801,7 +2801,7 @@ check_stopped_by_breakpoint (struct lwp_
>      {
>        if (siginfo.si_signo == SIGTRAP)
>  	{
> -	  if (siginfo.si_code == GDB_ARCH_TRAP_BRKPT)
> +	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
>  	    {
>  	      if (debug_linux_nat)
>  		fprintf_unfiltered (gdb_stdlog,
> Index: binutils-gdb/gdb/nat/linux-ptrace.h
> ===================================================================
> --- binutils-gdb.orig/gdb/nat/linux-ptrace.h
> +++ binutils-gdb/gdb/nat/linux-ptrace.h
> @@ -135,12 +135,19 @@ struct buffer;
>     running to a breakpoint and checking what comes out of
>     siginfo->si_code.
>  
> -   The generic Linux target code should use GDB_ARCH_TRAP_BRKPT
> -   instead of TRAP_BRKPT to abstract out this x86 peculiarity.  */
> +   The ppc kernel does use TRAP_BRKPT for software breakpoints
> +   in PowerPC code, but it uses SI_KERNEL for software breakpoints
> +   in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
> +   on a SIGTRAP for any other reason.
> +
> +   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
> +   instead of TRAP_BRKPT to abstract out these peculiarities.  */
>  #if defined __i386__ || defined __x86_64__
> -# define GDB_ARCH_TRAP_BRKPT SI_KERNEL
> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
> +#elif defined __powerpc__
> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
>  #else
> -# define GDB_ARCH_TRAP_BRKPT TRAP_BRKPT
> +# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
>  #endif
>  
>  #ifndef TRAP_HWBKPT
> 

LGTM.

Thanks,
Pedro Alves
diff mbox

Patch

Index: binutils-gdb/gdb/gdbserver/linux-low.c
===================================================================
--- binutils-gdb.orig/gdb/gdbserver/linux-low.c
+++ binutils-gdb/gdb/gdbserver/linux-low.c
@@ -651,7 +651,7 @@  check_stopped_by_breakpoint (struct lwp_
     {
       if (siginfo.si_signo == SIGTRAP)
 	{
-	  if (siginfo.si_code == GDB_ARCH_TRAP_BRKPT)
+	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
 	    {
 	      if (debug_threads)
 		{
Index: binutils-gdb/gdb/linux-nat.c
===================================================================
--- binutils-gdb.orig/gdb/linux-nat.c
+++ binutils-gdb/gdb/linux-nat.c
@@ -2801,7 +2801,7 @@  check_stopped_by_breakpoint (struct lwp_
     {
       if (siginfo.si_signo == SIGTRAP)
 	{
-	  if (siginfo.si_code == GDB_ARCH_TRAP_BRKPT)
+	  if (GDB_ARCH_IS_TRAP_BRKPT (siginfo.si_code))
 	    {
 	      if (debug_linux_nat)
 		fprintf_unfiltered (gdb_stdlog,
Index: binutils-gdb/gdb/nat/linux-ptrace.h
===================================================================
--- binutils-gdb.orig/gdb/nat/linux-ptrace.h
+++ binutils-gdb/gdb/nat/linux-ptrace.h
@@ -135,12 +135,19 @@  struct buffer;
    running to a breakpoint and checking what comes out of
    siginfo->si_code.
 
-   The generic Linux target code should use GDB_ARCH_TRAP_BRKPT
-   instead of TRAP_BRKPT to abstract out this x86 peculiarity.  */
+   The ppc kernel does use TRAP_BRKPT for software breakpoints
+   in PowerPC code, but it uses SI_KERNEL for software breakpoints
+   in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
+   on a SIGTRAP for any other reason.
+
+   The generic Linux target code should use GDB_ARCH_IS_TRAP_BRKPT
+   instead of TRAP_BRKPT to abstract out these peculiarities.  */
 #if defined __i386__ || defined __x86_64__
-# define GDB_ARCH_TRAP_BRKPT SI_KERNEL
+# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
+#elif defined __powerpc__
+# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
 #else
-# define GDB_ARCH_TRAP_BRKPT TRAP_BRKPT
+# define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
 #endif
 
 #ifndef TRAP_HWBKPT