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

[Pedro Alves <palves@redhat.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