[PATCH] Add gdb remote protocol handling to strace
Dmitry V. Levin
ldv at altlinux.org
Thu Nov 2 03:23:24 UTC 2017
On Tue, Oct 17, 2017 at 02:29:31PM -0400, Stan Cox wrote:
> This is the latest version of the gdbserver backend patch. It is
> rebased to master, supports the new event dispatch scheme,and has
> changes including buffer handling improvements and a rewrite of
> notification queue handling. It is a large patch but much of it is
> boilerplate changes due to adding the backend struct. The branch is
> located at:
> https://github.com/stanfordcox/strace
>
> -configure*: configure support
>
> -defs.h: move enum trace_event here
> struct backend: backend dispatch table. All of the gdbserver
> version 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
>
> -strace.c
> alloctcb and droptcb are accessed from the gdbs backend.
> handle_arg, end_init, final_cleanup, verify_args, start_init are
> generic default versions of backend methods.
> 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.
> -gdbrsp.test
> New test
>
> -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.
> ---
Stan, first of all thanks for working on this interesting project.
I suppose Eugene is going to look into it soon and provide the feedback
you're waiting for, so here is just a very cursory review.
The first issue I've stumbled upon is whitespace issues:
Applying: Add gdb remote protocol handling to strace
.git/rebase-apply/patch:1123: trailing whitespace.
"GDB server doesn't support vAttach!",
.git/rebase-apply/patch:1129: trailing whitespace.
"GDB server doesn't support vAttach!",
.git/rebase-apply/patch:1212: space before tab in indent.
stop = gdb_recv_stop(&stop);
.git/rebase-apply/patch:1214: space before tab in indent.
stop = gdb_recv_stop(NULL);
.git/rebase-apply/patch:2091: trailing whitespace.
case '%':
.git/rebase-apply/patch:2136: trailing whitespace.
case '*':
.git/rebase-apply/patch:2203: trailing whitespace.
.git/rebase-apply/patch:2207: trailing whitespace.
/* (See gdb_recv_stop for non-stop packet order)
.git/rebase-apply/patch:2208: trailing whitespace.
If a notification arrived while expecting another packet
.git/rebase-apply/patch:3586: trailing whitespace.
gdb -batch -iex 'target remote | gdbserver --remote-debug - /usr/bin/true' >| /tmp/,gdb 2>&1
.git/rebase-apply/patch:3601: trailing whitespace.
$STRACE -G localhost:65432 $(readlink -f ../gdbrsp) >& $LOG
.git/rebase-apply/patch:3623: trailing whitespace.
$STRACE -G 'localhost:65432;non-stop' $(readlink -f ../gdbrsp) >& $LOG
.git/rebase-apply/patch:3647: trailing whitespace.
$STRACE -G localhost:65432 -e write,read $(readlink -f ../gdbrsp) >& $LOG
fatal: 13 lines add whitespace errors.
Please correct them. I recommend enabling the pre-commit hook that comes
with git, it makes dealing with whitespace issues easier.
> diff --git a/Makefile.am b/Makefile.am
> index 4aa9846..3ea564e 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -47,13 +47,22 @@ ARCH = @arch@
>
> ACLOCAL_AMFLAGS = -I m4
> AM_CFLAGS = $(WARN_CFLAGS)
> -AM_CPPFLAGS = -I$(builddir)/$(OS)/$(ARCH) \
> +OS_CPPFLAGS = -I$(builddir)/$(OS)/$(ARCH) \
> -I$(srcdir)/$(OS)/$(ARCH) \
> -I$(builddir)/$(OS) \
> -I$(srcdir)/$(OS) \
> -I$(builddir) \
> -I$(srcdir)
If you move these flags to OS_CPPFLAGS,
> +if ENABLE_GDBSERVER
> +AM_CPPFLAGS = -I$(builddir)/gdbserver/$(ARCH) \
> + -I$(srcdir)/gdbserver/$(ARCH) \
> + -I$(builddir)/gdbserver \
> + -I$(srcdir)/gdbserver $(OS_CPPFLAGS)
> +else
> +AM_CPPFLAGS = $(OS_CPPFLAGS)
> +endif
let's move gdbserver specific flags to something like GDBSERVER_CPPFLAGS,
so that AM_CPPFLAGS initialization would look simple.
> diff --git a/defs.h b/defs.h
> index 34261f2..eebaf20 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -35,6 +35,8 @@
> # include "config.h"
> #endif
>
> +#include <sys/types.h>
<sys/types.h> is included a few lines later, right after <stdint.h>,
so I bet this is not needed at all.
> +#include <signal.h>
This change is NO-NO. It essentially reverts the key part of commit
v4.10-184-g0e946ab (defs.h: do not include <signal.h>) and breaks
mpers support.
If you try to build this on a system where configure test prints:
checking whether mpers.sh -m32 works... yes
e.g. on x86_64 with a proper -m32 support, you'll see that the build
of mpersified printsiginfo fails:
In file included from /usr/include/signal.h:79:0,
from defs.h:39,
from printsiginfo.c:36:
./mpers-m32/siginfo_t.h:11:9: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
int32_t si_pid;
^
./mpers-m32/siginfo_t.h:16:9: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
int32_t si_overrun;
^
./mpers-m32/siginfo_t.h:23:9: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
int32_t si_pid;
^
./mpers-m32/siginfo_t.h:31:9: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
int32_t si_pid;
^
./mpers-m32/siginfo_t.h:38:13: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
mpers_ptr_t si_addr;
^
./mpers-m32/siginfo_t.h:47:9: error: expected ':', ',', ';', '}' or '__attribute__' before '.' token
int32_t si_band;
^
and so on.
That is, I couldn't build strace with your patch applied.
--
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/20171102/baabe42b/attachment.bin>
More information about the Strace-devel
mailing list