Re: [PATCH V2 1/5] Merges gdb and gdbserver implementation for siginfo.

Pedro Alves
2015-12-18T12:25:57Z

> 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