This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH V2 1/5] Merges gdb and gdbserver implementation for siginfo.
- From: Pedro Alves <palves at redhat dot com>
- To: Walfred Tedeschi <walfred dot tedeschi at intel dot com>, brobecker at adacore dot com
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 18 Dec 2015 12:25:52 +0000
- Subject: Re: [PATCH V2 1/5] Merges gdb and gdbserver implementation for siginfo.
- Authentication-results: sourceware.org; auth=none
- References: <1450371416-24270-1-git-send-email-walfred dot tedeschi at intel dot com>
On 12/17/2015 04:56 PM, Walfred Tedeschi wrote:
> The compatible siginfo handling from amd64-linux-nat.c and
> gdbserver/linux-x86-low were extracted it into a new file
> nat/amd64-linux-siginfo.c.
>
>
> 2015-12-15 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> * nat/amd64-linux-siginfo.c: New file.
> * nat/amd64-linux-siginfo.h: New file.
> * Makefile.in (HFILES_NO_SRCDIR): Add new header to
> HFILES_NO_SRCDIR. Add native object files rule for
> amd64-linux-siginfo.o
"HFILES_NO_SRCDIR" is already the specified context. Specify the rules
as context:
* Makefile.in (HFILES_NO_SRCDIR): Add nat/amd64-linux-siginfo.h.
Add native object files rule for
(amd64-linux-siginfo.o:): New rule.
> * config/i386/linux64.mh (NATDEPFILES): Add amd64-linux-siginfo.o.
> * amd64-linux-nat.c (compat_siginfo_from_siginfo)
Mention the header inclusion:
* amd64-linux-nat.c: Include "nat/amd64-linux-siginfo.h".
(compat_siginfo_from_siginfo) ...
>
> gdbserver
>
> * configure.srv (srv_tgtobj): Add amd64-linux-siginfo.o.
Should be:
* configure.srv (x86_64-*-linux*): Add amd64-linux-siginfo.o to srv_tgtobj.
I think you also need to add this here in the gdb_cv_i386_is_x86_64 case:
i[34567]86-*-linux*) srv_regobj="$srv_i386_linux_regobj"
srv_xmlfiles="$srv_i386_linux_xmlfiles"
if test "$gdb_cv_i386_is_x86_64" = yes ; then
srv_regobj="$srv_regobj $srv_amd64_linux_regobj"
srv_xmlfiles="${srv_xmlfiles} $srv_amd64_linux_xmlfiles"
fi
> * linux-x86-low.c (compat_siginfo_from_siginfo)
Mention the include:
* linux-x86-low.c [__x86_64__]: Include "nat/amd64-linux-siginfo.h".
...
> (siginfo_from_compat_siginfo, compat_x32_siginfo_from_siginfo)
> (siginfo_from_compat_x32_siginfo and collateral structures): Move
> to nat/amd64-linux-siginfo.c.
> * Makefile.in (x86_64-*-linux*): Add amd64-linux-siginfo.o.
The last entry doesn't make sense. Seems like it was talking about the
configure.src change. You want:
* Makefile.in (amd64-linux-siginfo.o:): New rule.
> @@ -737,34 +335,15 @@ amd64_linux_siginfo_fixup (siginfo_t *native, gdb_byte *inf, int direction)
> /* Is the inferior 32-bit? If so, then do fixup the siginfo
> object. */
> if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32)
> - {
> - gdb_assert (sizeof (siginfo_t) == sizeof (compat_siginfo_t));
> -
> - if (direction == 0)
> - compat_siginfo_from_siginfo ((struct compat_siginfo *) inf, native);
> - else
> - siginfo_from_compat_siginfo (native, (struct compat_siginfo *) inf);
> -
> - return 1;
> - }
> + return amd64_linux_siginfo_fixup_common (native, inf , direction,
> + FIXUP_32);
Spurious space after "inf". Looks like that ended up in all
amd64_linux_siginfo_fixup_common calls.
> +++ b/gdb/nat/amd64-linux-siginfo.c
> +#include <signal.h>
> +#include "common-defs.h"
> +#include "amd64-linux-siginfo.h"
> +
> +/* When GDB is built as a 64-bit application on linux, the
> + PTRACE_GETSIGINFO data is always presented in 64-bit layout. Since
> + debugging a 32-bit inferior with a 64-bit GDB should look the same
> + as debugging it with a 32-bit GDB, we do the 32-bit <-> 64-bit
> + conversion in-place ourselves. With the presence of possible different
> + fields for host and target we have to guarantee that we use the
This sentence is incomplete.
> + Also, the first step is to make a copy from the original bits to the
> + internal structure which is the super set. */
> +
???
> +}
> +
> +/* Converts the compatible siginfo into system siginfo. */
> +static void
> +siginfo_from_compat_siginfo (siginfo_t *to, compat_siginfo_t *from)
There should be an empty line between intro comment and function.
Here and elsewhere.
> +
> +/* Convert a native/host siginfo object, into/from the siginfo in the
> + layout of the inferiors' architecture. Returns true if any
> + conversion was done; false otherwise. If DIRECTION is 1, then copy
> + from INF to NATIVE. If DIRECTION is 0, then copy from NATIVE to INF. */
This should be already documented in the header. Here say:
/* See whatever.h. */
> +
> +int
> +amd64_linux_siginfo_fixup_common (siginfo_t *native, gdb_byte *inf,
> + int direction,
> + enum amd64_siginfo_fixup_mode mode)
> +{
> +
> + if (mode == FIXUP_32)
Spurious empty line.
> +#ifndef AMD64_LINUX_SIGINFO_H
> +#define AMD64_LINUX_SIGINFO_H 1
> +
> +
> +/* When GDB is built as a 64-bit application on linux, the
> + PTRACE_GETSIGINFO data is always presented in 64-bit layout. Since
> + debugging a 32-bit inferior with a 64-bit GDB should look the same
> + as debugging it with a 32-bit GDB, we do the 32-bit <-> 64-bit
> + conversion in-place ourselves. With the presence of possible different
> + fields for host and target we have to guarantee that we use the
> + Also, the first step is to make a copy from the original bits to the
> + internal structure which is the super set. */
Duplicate comment. There should only be one copy, and it should probably
be in the header file.
> +
> +/* ENUM to determine the kind of siginfo fixup to be performed. */
No need to say it's an enum:
/* The kind of siginfo fixup to be performed. */
> +
> +enum amd64_siginfo_fixup_mode
> +{
> + FIXUP_32 = 1,
> + FIXUP_X32 = 2
Please document each mode.
> +};
> +
> +/* Common code for performing the fixup of the siginfo. */
> +
> +int
> +amd64_linux_siginfo_fixup_common (siginfo_t *native, gdb_byte *inf,
Function name only goes at column 0 in definitions, never in declarations.
> + int direction,
> + enum amd64_siginfo_fixup_mode mode);
> +
> +#endif
Thanks,
Pedro Alves