[PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Luis Machado]

This is v3 of the patch, which adds a comment to how MIPS' kernel handles/will
handle si_code values.

Pedro, Maciej, does this look reasonable? Should i expand the comment with
any other bits of information?

--

While doing some MIPS/Linux tests, i've found a number of threading tests
failing due to spurious SIGTRAP's. Turns out those spurious SIGTRAP's were
actually software breakpoint hits that were supposed to be handled silently by
GDB/GDBserver, returning a swbreak event.

gdb.threads/continue-pending-status.exp is one of the testcases that show this
behavior.

--

Breakpoint 1, main () at gdb.threads/continue-pending-status.c:44^M
44        pthread_barrier_init (&barrier, NULL, NUM_THREADS);^M
(gdb) b continue-pending-status.c:36^M
Breakpoint 2 at 0x400a04: file gdb.threads/continue-pending-status.c, line 36.^M
(gdb) PASS: gdb.threads/continue-pending-status.exp: attempt 0: set break in tight loop
continue^M
Continuing.^M
[New Thread 5850]^M
[New Thread 5851]^M
[Switching to Thread 5850]^M
^M
Breakpoint 2, thread_function (arg=0x0) at gdb.threads/continue-pending-status.c:36^M
36        while (1); /* break here */^M
(gdb) PASS: gdb.threads/continue-pending-status.exp: attempt 0: continue to tight loop
print /x $_thread^M
$1 = 0x2^M
(gdb) PASS: gdb.threads/continue-pending-status.exp: attempt 0: get thread number
thread 3^M
[Switching to thread 3 (Thread 5851)]^M
36        while (1); /* break here */^M
(gdb) PASS: gdb.threads/continue-pending-status.exp: attempt 0: switch to non-event thread
delete breakpoints^M
Delete all breakpoints? (y or n) y^M
(gdb) info breakpoints^M
No breakpoints or watchpoints.^M
(gdb) continue^M
Continuing.^M
^M
Program received signal SIGTRAP, Trace/breakpoint trap.^M

<<<< This SIGTRAP was a pending breakpoint event that wasn't supposed to cause
<<<< a stop, but gdbserver did not figure out this was a breakpoint hit.

PASS: gdb.threads/continue-pending-status.exp: attempt 0: continue for ctrl-c
thread_function (arg=0x0) at gdb.threads/continue-pending-status.c:36^M
36        while (1); /* break here */^M
(gdb) FAIL: gdb.threads/continue-pending-status.exp: attempt 0: caught interrupt

--

I tracked this down to the lack of a proper definition of what MIPS' kernel
returns in the si_code for a software breakpoint trap.

Further discussion with MIPS maintainers showed that, historically, MIPS
kernels have never set a proper si_code and thus they use the default value of
SI_KERNEL.

There are plans to update the MIPS kernel to provide more meaningful si_code
values though, so we should expect both SI_KERNEL and TRAP_BRKPT from now
on, as GDB will handle both correctly, like powerpc.

With the following patch i have cleaner results for thread tests on
MIPS/Linux.

Regression-tested on a few MIPS boards.

OK?

gdb/ChangeLog:

2016-02-20  Luis Machado  <lgustavo@codesourcery.com>

  * nat/linux-ptrace.h: Check for both SI_KERNEL and TRAP_BRKPT si_code for
    MIPS.
---
 gdb/nat/linux-ptrace.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
index ba58717..72b32b1 100644
--- a/gdb/nat/linux-ptrace.h
+++ b/gdb/nat/linux-ptrace.h
@@ -140,11 +140,16 @@ struct buffer;
    in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
    on a SIGTRAP for any other reason.
 
+   The MIPS kernel uses the default si_code of SI_KERNEL for software
+   breakpoints, hardware watchpoints and SIGTRAP's in general.  There are
+   plans to start using a si_code value of TRAP_BRKPT in the kernel, so
+   we check for both values.
+
    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_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL)
-#elif defined __powerpc__
+#elif defined __powerpc__ || defined __mips__
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == SI_KERNEL || (X) == TRAP_BRKPT)
 #else
 # define GDB_ARCH_IS_TRAP_BRKPT(X) ((X) == TRAP_BRKPT)
-- 
1.9.1

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Pedro Alves]

On 02/22/2016 10:20 PM, Luis Machado wrote:

> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
> index ba58717..72b32b1 100644
> --- a/gdb/nat/linux-ptrace.h
> +++ b/gdb/nat/linux-ptrace.h
> @@ -140,11 +140,16 @@ struct buffer;
>     in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>     on a SIGTRAP for any other reason.
>  
> +   The MIPS kernel uses the default si_code of SI_KERNEL for software
> +   breakpoints, hardware watchpoints and SIGTRAP's in general.  

If we get this for both software breakpoints and hardware watchpoints,
then it seems to me that this change still leaves watchpoints broken,
as I can't see how check_stopped_by_watchpoint is reached, in either
gdb/linux-nat.c or gdbserver/linux-low.c.

Also, "and SIGTRAP's in general." seems wrong.  I hope that that's
not the case for user-sent SIGTRAPs?

Thanks,
Pedro Alves

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Luis Machado]

On 02/22/2016 07:41 PM, Pedro Alves wrote:
> On 02/22/2016 10:20 PM, Luis Machado wrote:
>
>> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
>> index ba58717..72b32b1 100644
>> --- a/gdb/nat/linux-ptrace.h
>> +++ b/gdb/nat/linux-ptrace.h
>> @@ -140,11 +140,16 @@ struct buffer;
>>      in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>>      on a SIGTRAP for any other reason.
>>
>> +   The MIPS kernel uses the default si_code of SI_KERNEL for software
>> +   breakpoints, hardware watchpoints and SIGTRAP's in general.
>
> If we get this for both software breakpoints and hardware watchpoints,
> then it seems to me that this change still leaves watchpoints broken,
> as I can't see how check_stopped_by_watchpoint is reached, in either
> gdb/linux-nat.c or gdbserver/linux-low.c.
>
> Also, "and SIGTRAP's in general." seems wrong.  I hope that that's
> not the case for user-sent SIGTRAPs?

I'll defer that to Maciej, as he knows what the kernel emits here. From 
his descriptions, it seems MIPS' kernel pretty much just uses the 
default si_code of SI_KERNEL.

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Maciej W. Rozycki]

On Mon, 22 Feb 2016, Luis Machado wrote:

> > > +   The MIPS kernel uses the default si_code of SI_KERNEL for software
> > > +   breakpoints, hardware watchpoints and SIGTRAP's in general.
> > 
> > If we get this for both software breakpoints and hardware watchpoints,
> > then it seems to me that this change still leaves watchpoints broken,
> > as I can't see how check_stopped_by_watchpoint is reached, in either
> > gdb/linux-nat.c or gdbserver/linux-low.c.
> > 
> > Also, "and SIGTRAP's in general." seems wrong.  I hope that that's
> > not the case for user-sent SIGTRAPs?
> 
> I'll defer that to Maciej, as he knows what the kernel emits here. From his
> descriptions, it seems MIPS' kernel pretty much just uses the default si_code
> of SI_KERNEL.

 Yes, for signals produced by the kernel itself, i.e. with `__send_signal' 
from kernel/signal.c with the `info' argument set to SEND_SIG_PRIV, which 
the various calls to `force_sig' from arch/mips/kernel/traps.c boil down 
to.  To have the code set to something else `force_sig_info' is used, in 
the interpretation of `BREAK 6' and `BREAK 7' instructions among other 
cases.  This is also how kill(2) works, setting the code to SI_USER for 
user-issued signals, which is generic in Linux so any architecture will do 
the same here.

  Maciej

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Luis Machado]

On 02/22/2016 07:41 PM, Pedro Alves wrote:
> On 02/22/2016 10:20 PM, Luis Machado wrote:
>
>> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
>> index ba58717..72b32b1 100644
>> --- a/gdb/nat/linux-ptrace.h
>> +++ b/gdb/nat/linux-ptrace.h
>> @@ -140,11 +140,16 @@ struct buffer;
>>      in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>>      on a SIGTRAP for any other reason.
>>
>> +   The MIPS kernel uses the default si_code of SI_KERNEL for software
>> +   breakpoints, hardware watchpoints and SIGTRAP's in general.
>
> If we get this for both software breakpoints and hardware watchpoints,
> then it seems to me that this change still leaves watchpoints broken,
> as I can't see how check_stopped_by_watchpoint is reached, in either
> gdb/linux-nat.c or gdbserver/linux-low.c.

With or without this specific breakpoint fix, hardware watchpoints are 
already broken for MIPS AFAICS.

MIPS' kernel has never set si_code to anything other than SI_KERNEL. 
When the change to expect TRAP_HWBKPT was committed, then hardware 
watchpoints stopped working properly for MIPS.

Now all hardware watchpoints produce are breakpoint-like traps that get 
silently ignored by GDB as "delayed software breakpoint trap". Neither 
GDB nor GDBserver can tell when a hardware watchpoint really happened by 
using si_code information.

But this seems to be a different issue and i think it should be handle 
separately.

>
> Also, "and SIGTRAP's in general." seems wrong.  I hope that that's
> not the case for user-sent SIGTRAPs?
>

I'll get this updated in the next version.

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Maciej W. Rozycki]

On Tue, 23 Feb 2016, Luis Machado wrote:

> > If we get this for both software breakpoints and hardware watchpoints,
> > then it seems to me that this change still leaves watchpoints broken,
> > as I can't see how check_stopped_by_watchpoint is reached, in either
> > gdb/linux-nat.c or gdbserver/linux-low.c.
> 
> With or without this specific breakpoint fix, hardware watchpoints are already
> broken for MIPS AFAICS.
> 
> MIPS' kernel has never set si_code to anything other than SI_KERNEL. When the
> change to expect TRAP_HWBKPT was committed, then hardware watchpoints stopped
> working properly for MIPS.
> 
> Now all hardware watchpoints produce are breakpoint-like traps that get
> silently ignored by GDB as "delayed software breakpoint trap". Neither GDB nor
> GDBserver can tell when a hardware watchpoint really happened by using si_code
> information.

 Thanks for checking, I wasn't aware there could be something wrong here.

> But this seems to be a different issue and i think it should be handle
> separately.

 Agreed.  I'll see if I can get a kernel patch cooked up in the next few 
days, however this still leaves a question open as to what to do about 
legacy systems which don't have these signal codes implemented.  Plain 
leaving them broken forever does not sound like a good plan to me.

> > Also, "and SIGTRAP's in general." seems wrong.  I hope that that's
> > not the case for user-sent SIGTRAPs?
> > 
> 
> I'll get this updated in the next version.

 FWIW it LGTM, but I'll leave it up to Pedro to approve this change as it 
was him who requested a comment update.

  Maciej

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Pedro Alves]

On 02/23/2016 02:00 AM, Luis Machado wrote:
> On 02/22/2016 07:41 PM, Pedro Alves wrote:
>> On 02/22/2016 10:20 PM, Luis Machado wrote:
>>
>>> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
>>> index ba58717..72b32b1 100644
>>> --- a/gdb/nat/linux-ptrace.h
>>> +++ b/gdb/nat/linux-ptrace.h
>>> @@ -140,11 +140,16 @@ struct buffer;
>>>      in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>>>      on a SIGTRAP for any other reason.
>>>
>>> +   The MIPS kernel uses the default si_code of SI_KERNEL for software
>>> +   breakpoints, hardware watchpoints and SIGTRAP's in general.
>>
>> If we get this for both software breakpoints and hardware watchpoints,
>> then it seems to me that this change still leaves watchpoints broken,
>> as I can't see how check_stopped_by_watchpoint is reached, in either
>> gdb/linux-nat.c or gdbserver/linux-low.c.
> 
> With or without this specific breakpoint fix, hardware watchpoints are 
> already broken for MIPS AFAICS.

Yep, hence my "still".  ;-)

> 
> MIPS' kernel has never set si_code to anything other than SI_KERNEL. 
> When the change to expect TRAP_HWBKPT was committed, then hardware 
> watchpoints stopped working properly for MIPS.
> 
> Now all hardware watchpoints produce are breakpoint-like traps that get 
> silently ignored by GDB as "delayed software breakpoint trap". Neither 
> GDB nor GDBserver can tell when a hardware watchpoint really happened by 
> using si_code information.
> 
> But this seems to be a different issue and i think it should be handle 
> separately.

I think it's all the same, and that we'll need to change
linux-nat.c:save_sigtrap and the equivalent bits in gdbserver/linux-low.c.

So if we pick the table I added for x86, and fill it in
for MIPS, this is what we get:

   | what                                     | si_code        |
   |------------------------------------------+----------------|
   | software breakpoints                     | SI_KERNEL      |
   | single-steps                             | N/A            |
   | single-stepping a syscall                | N/A            |
   | user sent SIGTRAP                        | 0              |
   | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | doesn't matter |
   | hardware breakpoints                     | N/A            |
   | hardware watchpoints                     | SI_KERNEL      |

So the only distinction we need to make is between software
breakpoints SI_KERNEL, and hardware watchpoints SI_KERNEL.

And we can accurately do that with
mips_linux_stopped_by_watchpoint / mips_stopped_by_watchpoint.

Is my understanding correct?

Thanks,
Pedro Alves

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Luis Machado]

On 02/24/2016 09:55 AM, Pedro Alves wrote:
> On 02/23/2016 02:00 AM, Luis Machado wrote:
>> On 02/22/2016 07:41 PM, Pedro Alves wrote:
>>> On 02/22/2016 10:20 PM, Luis Machado wrote:
>>>
>>>> diff --git a/gdb/nat/linux-ptrace.h b/gdb/nat/linux-ptrace.h
>>>> index ba58717..72b32b1 100644
>>>> --- a/gdb/nat/linux-ptrace.h
>>>> +++ b/gdb/nat/linux-ptrace.h
>>>> @@ -140,11 +140,16 @@ struct buffer;
>>>>       in SPU code on a Cell/B.E.  However, SI_KERNEL is never seen
>>>>       on a SIGTRAP for any other reason.
>>>>
>>>> +   The MIPS kernel uses the default si_code of SI_KERNEL for software
>>>> +   breakpoints, hardware watchpoints and SIGTRAP's in general.
>>>
>>> If we get this for both software breakpoints and hardware watchpoints,
>>> then it seems to me that this change still leaves watchpoints broken,
>>> as I can't see how check_stopped_by_watchpoint is reached, in either
>>> gdb/linux-nat.c or gdbserver/linux-low.c.
>>
>> With or without this specific breakpoint fix, hardware watchpoints are
>> already broken for MIPS AFAICS.
>
> Yep, hence my "still".  ;-)
>
>>
>> MIPS' kernel has never set si_code to anything other than SI_KERNEL.
>> When the change to expect TRAP_HWBKPT was committed, then hardware
>> watchpoints stopped working properly for MIPS.
>>
>> Now all hardware watchpoints produce are breakpoint-like traps that get
>> silently ignored by GDB as "delayed software breakpoint trap". Neither
>> GDB nor GDBserver can tell when a hardware watchpoint really happened by
>> using si_code information.
>>
>> But this seems to be a different issue and i think it should be handle
>> separately.
>
> I think it's all the same, and that we'll need to change
> linux-nat.c:save_sigtrap and the equivalent bits in gdbserver/linux-low.c.
>
> So if we pick the table I added for x86, and fill it in
> for MIPS, this is what we get:
>
>     | what                                     | si_code        |
>     |------------------------------------------+----------------|
>     | software breakpoints                     | SI_KERNEL      |
>     | single-steps                             | N/A            |
>     | single-stepping a syscall                | N/A            |
>     | user sent SIGTRAP                        | 0              |
>     | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | doesn't matter |
>     | hardware breakpoints                     | N/A            |
>     | hardware watchpoints                     | SI_KERNEL      |
>

Should i add that table explicitly or will v4 with the most meaningful 
bits of that table cut it? It seems we will eventually move from 
SI_KERNEL to something more appropriate for MIPS.

> So the only distinction we need to make is between software
> breakpoints SI_KERNEL, and hardware watchpoints SI_KERNEL.
>
> And we can accurately do that with
> mips_linux_stopped_by_watchpoint / mips_stopped_by_watchpoint.
>
> Is my understanding correct?
>

Yes. We need to check the MIPS debug registers unconditionally and not 
just when a meaningful si_code of TRAP_HWBKPT shows up.

Did you want that in the same patch or in a separate one? I was 
attempting to address the software breakpoint issue for now, but it 
seems i ended up with 2 problems now. :-)

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Pedro Alves]

On 02/24/2016 01:21 PM, Luis Machado wrote:

> Yes. We need to check the MIPS debug registers unconditionally and not 
> just when a meaningful si_code of TRAP_HWBKPT shows up.

Yeah, though we won't be able to do that unconditionally, due
to the s390 quirk.

> Did you want that in the same patch or in a separate one? I was 
> attempting to address the software breakpoint issue for now, but it 
> seems i ended up with 2 problems now. :-)

I see this as a single problem of a regression caused by the
rely-on-si_code change.  So I think we should fix it with a single patch.
Give me a bit while I try an approach here.

Thanks,
Pedro Alves

Re: [PATCH, v3] Expect SI_KERNEL or TRAP_BRKPT si_code values for MIPS breakpoint traps

[Pedro Alves]

On 02/24/2016 02:44 PM, Pedro Alves wrote:
> On 02/24/2016 01:21 PM, Luis Machado wrote:
> 
>> Yes. We need to check the MIPS debug registers unconditionally and not 
>> just when a meaningful si_code of TRAP_HWBKPT shows up.
> 
> Yeah, though we won't be able to do that unconditionally, due
> to the s390 quirk.
> 
>> Did you want that in the same patch or in a separate one? I was 
>> attempting to address the software breakpoint issue for now, but it 
>> seems i ended up with 2 problems now. :-)
> 
> I see this as a single problem of a regression caused by the
> rely-on-si_code change.  So I think we should fix it with a single patch.
> Give me a bit while I try an approach here.

Posted here:

 https://sourceware.org/ml/gdb-patches/2016-02/msg00734.html

Could you give it a try?

Thanks,
Pedro Alves