[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@
> -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,

> +AM_CPPFLAGS = -I$(builddir)/gdbserver/$(ARCH) \
> +	      -I$(srcdir)/gdbserver/$(ARCH) \
> +	      -I$(builddir)/gdbserver \
> +	      -I$(srcdir)/gdbserver $(OS_CPPFLAGS)
> +else
> +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.

-------------- 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