Re: [PATCH 0/8] Get rid of the whole moribund breakpoints mechanism, use TRAP_BRKPT

[Pedro Alves <palves at redhat dot com>]

Hi,

I went ahead and pushed this in, after addressing Eli's comments.

The non-stop-fair-events.exp failures/timeouts are quite annoying
and contributing a lot to the current long buildbot delays.  Hopefully
they're fully fixed with this, along with a few other non-stop
tests that are occasionally failing.

Naturally I'll do my best to address any issues or further comments.

To recap, if this causes trouble for some architecture, we can just
define USE_SIGTRAP_SIGINFO to 0 in nat/linux-ptrace.h on that arch, to
revert back to old behavior.

/me crosses fingers.

Thanks,
Pedro Alves

On 02/26/2015 12:17 AM, Pedro Alves wrote:
> Well, not really get rid of it (at least not yet), but add a reliable,
> non-heuristic, but still efficient alternative.
> 
> Note: the "remote" patch includes manual and NEWS changes.
> 
> The non-stop-fair-events.exp test is frequently failing currently.
> E.g., see https://sourceware.org/ml/gdb-testers/2015-q1/msg03148.html.
> 
> The root cause is a fundamental problem with the whole moribund
> breakpoint locations mechanism.
> 
> This series teaches GDB about targets that can reliably tell whether a
> trap was caused by a software or hardware breakpoint, and makes use of
> that information instead as reliable mechanism to detect delayed
> events.  With that in place, we don't handle moribund locations
> anymore (on such targets).
> 
> On Linux, we'll get that information out of the si_code of the SIGTRAP
> siginfo.  E.g., for software breakpoint hits, Linux sets that field to
> TRAP_BRKPT.
> 
> For years I assumed that x86 just didn't include this information in
> the siginfo, because when a breakpoint trap is raised, si_code does
> not show TRAP_BRKPT, as one sees on other architectures.  Because of
> that, I had added an entry to gdb's kernel wishlist at
> https://sourceware.org/gdb/wiki/LinuxKernelWishList for TRAP_BRKPT in
> 2012...
> 
> But taking a closer look, even though the x86 port puts odd values in
> SIGTRAP's si_code, those _are_ usable as is.
> 
> Based on reading the kernel's source and experimenting things, this is
> how x86's si_code works (and has "always" worked):
> 
> | what                                     | si_code    |
> |------------------------------------------+------------|
> | software breakpoints (int3)              | SI_KERNEL  |
> | single-steps                             | TRAP_TRACE |
> | single-stepping a syscall                | TRAP_BRKPT |
> | user sent SIGTRAP                        | 0          |
> | exec SIGTRAP (when no PTRACE_EVENT_EXEC) | 0          |
> | hardware breakpoints/watchpoints         | TRAP_HWBPT |
> 
> It's almost as if TRAP_BRKPT and SI_KERNEL were mistakenly reversed...
> 
> Both user-sent traps and execs get the same si_code, but that's not
> really a problem given PTRACE_EVENT_EXEC.
> 
> If the kernel is ever "fixed" to use the right values, we can always
> detect it like we detect support for ptrace features (by having gdb
> debug itself a bit at startup, and looking at the si_code of a
> breakpoint hit).
> 
> As I can't be sure all supported architectures get si_code right, I
> left the old code paths in place, guarded behind a macro.  I prefer
> keeping that for a little longer, until we're reasonably sure no
> target is messed up by this change.  IOW, I propose enabling this by
> default everywhere, and only disabling if problems are reported.  If
> we only enable on an arch-by-arch basis, it's almost certain that even
> if all architectures can use this, we'll never end up enabling it on
> all architectures...  If after a while, no problem is ever reported,
> we can remove the fallback code then.  And if some architecture does
> indeed trip on some problem, we'll disable the new code on that arch,
> but I'll make some loud noises about fixing it in the kernel too, so
> later GDB releases can again consider dropping the old code.
> 
> Tested on:
> 
> . x86-64 Fedora 20 (decr_pc_after_break target)
> . s390 RHEL 7  (decr_pc_after_break)
> . PPC64 Fedora 18 (!decr_pc_after_break)
> 
> An earlier version was tested on:
> 
> . ARM Fedora 21 (!decr_pc_after_break).
> 
> I've tested the series incrementally at each patch on x86-64 Fedora
> 20, and I also hacked away the new feature manually in different
> manners to exercise different combinations of GDB vs GDBserver
> support, to make sure GDB and GDBserver coped with different
> combinations of GDB, GDBserver and backend support.
> 
> Pedro Alves (8):
>   enum lwp_stop_reason -> enum target_stop_reason
>   Teach GDB about targets that can tell whether a trap is a breakpoint
>     event
>   record-full/record-btrace: software/hardware breakpoint trap
>   remote+docs: software/hardware breakpoint traps
>   Linux native: Use TRAP_BRKPT/TRAP_HWBPT
>   gdbserver: Support the "swbreak"/"hwbreak" stop reasons
>   gdbserver/Linux: Use TRAP_BRKPT/TRAP_HWBPT
>   garbage collect target_decr_pc_after_break
> 
>  gdb/NEWS                      |  10 +++
>  gdb/aix-thread.c              |   2 +-
>  gdb/breakpoint.c              |  91 +++++++++++++++++--------
>  gdb/breakpoint.h              |   5 ++
>  gdb/btrace.h                  |   4 ++
>  gdb/darwin-nat.c              |   4 +-
>  gdb/doc/gdb.texinfo           |  80 ++++++++++++++++++++++
>  gdb/gdbserver/linux-low.c     | 154 ++++++++++++++++++++++++++++++++++++------
>  gdb/gdbserver/linux-low.h     |  21 +-----
>  gdb/gdbserver/linux-x86-low.c |   2 +-
>  gdb/gdbserver/remote-utils.c  |  10 +++
>  gdb/gdbserver/server.c        |  25 +++++++
>  gdb/gdbserver/server.h        |  11 +++
>  gdb/gdbserver/target.h        |  31 +++++++++
>  gdb/infrun.c                  |  72 +++++++++++++++++++-
>  gdb/linux-nat.c               | 131 ++++++++++++++++++++++++++++++-----
>  gdb/linux-nat.h               |  20 +-----
>  gdb/linux-thread-db.c         |   6 +-
>  gdb/nat/linux-ptrace.h        |  51 ++++++++++++++
>  gdb/record-btrace.c           |  69 ++++++++++++++++---
>  gdb/record-full.c             | 103 ++++++++++++++++++----------
>  gdb/record.c                  |  19 ++++++
>  gdb/record.h                  |  13 ++++
>  gdb/remote.c                  | 119 +++++++++++++++++++++++++++++---
>  gdb/target-delegates.c        | 151 +++++++++++++++++++++++++++++++++--------
>  gdb/target.c                  |  20 ------
>  gdb/target.h                  |  54 ++++++++++++---
>  gdb/target/waitstatus.h       |  19 ++++++
>  gdb/x86-linux-nat.c           |   2 +-
>  29 files changed, 1071 insertions(+), 228 deletions(-)
>