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
signature.asc
Description: PGP signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Strace-devel mailing list Strace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/strace-devel
