[PATCH] Add gdb remote protocol backend to strace

[Dmitry V. Levin]

Hi,

On Tue, Feb 11, 2020 at 10:03:26PM -0500, Stan Cox wrote:
> This is the latest version of the gdbserver backend patch.  It
> integrates Eugene's ptrace instantiation of the backend dispatch
> mechanism added for the gdbserver backend.  It is a large patch but
> much of it is boilerplate changes due to adding and use of the
> tracing_backend struct.  The code is located on the branch
> 'gdbserver0' in the repository:
>   https://github.com/stanfordcox/strace

Just to make things clear, that gdbserver0 branch contains 104 commits,
7 of them are merge commits with strace master branch, the last of these
merge commits is a merge with v5.5.

> configure*: configure support
> 
> -tracing_backend.h:
>   struct tracing_backend:  backend dispatch table.   All of the 
> gdbserver/ptrace
>   versions of these routines are independent and self contained except
>   for get_regs which needs access to the register structures
>   Any change not mentioned is only a change to allow for calling via a
>   backend routine
> 
> -ptrace_backend.{c,h}:
>   ptrace instantiation of tracing_backend.  It dispatches to existing 
> routines
> 
> -strace.c/syscall.c
>   dispatch to the ptrace_* routines defined in ptrace_backend.h
>   Add 'G' option to indicate how to connect to gdbs backend
>   start_init invokes gdbserver, startup_child, startup_attach (via
>   attach_tcb) start or connect to the child,
> -syscall.c
>   invoke backend.get_regs, which needs to access the iovec.  gdbserver
>   does not require a separate routine to get the syscall number.
> 
> -gdbserver.c
>   the gdbserver of the backend methods are defined here.  Communicates
>   with gdbserver via the gdb remote protocol and converts to the
>   packets that strace expects.  gdbserver has a syscall mode that
>   returns information on syscalls via stop packets.  gdbserver has two
>   protocol modes, all stop and non stop; both are supported.
> -protocol.c
>   support routines: gdb remote packets are deciphered here
> -protocol.h
> -signals.def
> -signals.h
>   gdb to native signal translation
> -gdb_get_regs.c
>   Gets the register set from gdbserver and converts to strace structures.
> 
> -strace.1.in
>   Document the -G gdbserver backend enabling option
> 
> -tests/gdbrsp.test
>   New test
> 
> -tests/Makefile.am
> -tests/gdbrsp.c
> -tests/gdbrsp.test
> -tests/gen_tests.in
> -tests/gen_tests.sh
> -tests/init.sh
> -tests/options-syntax.test
>   Added gdbserver versions of the generated tests

>  Makefile.am                      |   36 ++-
>  aux_children.c                   |  154 ++++++++++
>  aux_children.h                   |   74 +++++
>  configure.ac                     |   28 ++
>  defs.h                           |   19 +-
>  dist/README                      |    4 +
>  gdbserver/gdbserver.c            | 1397 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdbserver/protocol.c             |  803 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdbserver/protocol.h             |   82 ++++++
>  gdbserver/signals.def            |  200 +++++++++++++
>  gdbserver/signals.h              |   58 ++++
>  gdbserver/x86_64/gdb_arch_defs.h |    1 +
>  gdbserver/x86_64/gdb_get_regs.c  |  132 +++++++++
>  number_set.c                     |   39 +++
>  number_set.h                     |    5 +
>  pathtrace.c                      |    2 +-
>  ptrace_backend.c                 |  146 ++++++++++
>  ptrace_backend.h                 |   91 ++++++
>  socketutils.c                    |   10 +-
>  strace.1.in                      |   12 +
>  strace.c                         |  347 +++++++++++++++++------
>  syscall.c                        |   54 +++-
>  tests/.gitignore                 |    1 +
>  tests/Makefile.am                |    2 +
>  tests/gdbrsp.c                   |   79 ++++++
>  tests/gdbrsp.test                |  108 +++++++
>  tests/gen_tests.in               |   12 +-
>  tests/gen_tests.sh               |   28 +-
>  tests/init.sh                    |   30 +-
>  tests/options-syntax.test        |    2 +-
>  tracing_backend.c                |   41 +++
>  tracing_backend.h                |  425 ++++++++++++++++++++++++++++
>  ucopy.c                          |    8 +-
>  upeek.c                          |    2 +-
>  upoke.c                          |    2 +-
>  util.c                           |    9 +-
>  36 files changed, 4318 insertions(+), 125 deletions(-)

Thanks, this is indeed a large patch.  Very impressive.
In fact, this looks like a several years of development.
What do you suggest us to do with it now? :)

We do not use merge commits, our master branch contains a linear history
of commits, where every commit is free from whitespace errors, doesn't
fail tests, and have a commit message explaining what's being changed
and why.

When looking at your gdbserver0 branch, I sometimes cannot tell why those
changes are being made.  For example, the code is migrated to posix_spawn
in the beginning of the series, then migrated back to fork+exec in the
end, and I'm at a loss why each of these big moves was necessary.

Could you transform your patch series to a form suitable for review,
please?  This would mean rebasing on top of the master branch, fixing
whitespace errors in each commit, making each commit pass the test suite,
and improving many commit messages that are not informative enough.

Thanks,


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20200413/29eeb2ed/attachment.bin>