RE: [PATCH V2 5/5] Adapt siginfo fixup for the new bnd fields.

["Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>]

Pedro,


Yes, This way we don't need to rely on the systems where gdb/gdbserver was compiled.

I understood as an intermediate step in that the siginfo buffer from the system is memcopied to the internal structure and then to compat and vice versa.

I.e.:
	 buffer -> nat-> compat (x32).
Or 
	Compat (x32) -> nat -> buffer.

Thanks for your reviews!
Regards,
-Fred

-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com] 
Sent: Friday, December 18, 2015 3:43 PM
To: Tedeschi, Walfred; brobecker@adacore.com
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH V2 5/5] Adapt siginfo fixup for the new bnd fields.

On 12/17/2015 04:56 PM, Walfred Tedeschi wrote:
> New bnds fields will be always present for x86 architecture.
> Fixup for compatibility layer 32bits has to be fixed.
> 
> It was added the nat_siginfo to serving as intermediate step between 
> kernel provided siginfo and the fix up routine.
> 
> When executing compat_siginfo_from_siginfo or 
> compat_x32_siginfo_from_siginfo first the buffer read from the kernel 
> are converted into the nat_signfo for homogenization, then the fields 
> of nat_siginfo are use to set the compat and compat_x32 siginfo structures.
> 
> When executing  siginfo_from_compat_siginfo or 
> siginfo_from_compat_x32_siginfo the process happens in oposite order.
> 
> In doing this the fixups become more independent of the system underneath.

Can you expand on the rationale?  AFAICS, there's no intermediate step conversion, in the sense of a field-by-field conversion.
It's a straight memcpy to/from the host's siginfo_t.

So is the reason for this new type so that we don't rely on the siginfo_t host's type at all?  So that we can access the new fields even if gdb is not compiled on a system with the updated glibc yet?

Thanks,
Pedro Alves

> 
> Caveat: No support for MPX on x32.
> 
> 2015-12-08  Walfred Tedeschi  <walfred.tedeschi@intel.com>
> 
> 	* amd64-linux-siginfo.c (nat_siginfo_t, nat_sigval_t, nat_timeval):
> 	New types.
> 	(compat_siginfo): New bnd fields added.
> 	(compat_x32_siginfo): New field added.
> 	(cpt_si_addr_lsb): New define.
> 	(compat_siginfo_from_siginfo): Use nat_siginfo.
> 	(siginfo_from_compat_siginfo): Use nat_siginfo.
> 	(compat_x32_siginfo_from_siginfo): Likewise.
> 	(siginfo_from_compat_x32_siginfo): Likewise.
> 
> ---
>  gdb/nat/amd64-linux-siginfo.c | 311 
> ++++++++++++++++++++++++++++--------------
>  1 file changed, 211 insertions(+), 100 deletions(-)
> 
> diff --git a/gdb/nat/amd64-linux-siginfo.c 
> b/gdb/nat/amd64-linux-siginfo.c index 22e3552..8f0f486 100644
> --- a/gdb/nat/amd64-linux-siginfo.c
> +++ b/gdb/nat/amd64-linux-siginfo.c
> @@ -30,6 +30,92 @@
>     Also, the first step is to make a copy from the original bits to the
>     internal structure which is the super set. */
>  
> +/* These types below (native_*) define a siginfo type that is layout
> +   the most complete siginfo available for the architecture.  */
> +
> +typedef int nat_int_t;
> +typedef void* nat_uptr_t;
> +
> +typedef int nat_time_t;
> +typedef int nat_timer_t;
> +
> +/* For native 64-bit, clock_t in _sigchld is 64bit aligned at 4 
> +bytes.  */ typedef long __attribute__ ((__aligned__ (4))) 
> +nat_clock_t;
> +
> +struct nat_timeval
> +{
> +  nat_time_t tv_sec;
> +  int tv_usec;
> +};
> +
> +typedef union nat_sigval
> +{
> +  nat_int_t sival_int;
> +  nat_uptr_t sival_ptr;
> +} nat_sigval_t;
> +
> +typedef struct nat_siginfo
> +{
> +  int si_signo;
> +  int si_errno;
> +  int si_code;
> +
> +  union
> +  {
> +    int _pad[((128 / sizeof (int)) - 4)];
> +    /* kill() */
> +    struct
> +    {
> +      unsigned int _pid;
> +      unsigned int _uid;
> +    } _kill;
> +
> +    /* POSIX.1b timers */
> +    struct
> +    {
> +      nat_timer_t _tid;
> +      int _overrun;
> +      nat_sigval_t _sigval;
> +    } _timer;
> +
> +    /* POSIX.1b signals */
> +    struct
> +    {
> +      unsigned int _pid;
> +      unsigned int _uid;
> +      nat_sigval_t _sigval;
> +    } _rt;
> +
> +    /* SIGCHLD */
> +    struct
> +    {
> +      unsigned int _pid;
> +      unsigned int _uid;
> +      int _status;
> +      nat_clock_t _utime;
> +      nat_clock_t _stime;
> +    } _sigchld;
> +
> +    /* SIGILL, SIGFPE, SIGSEGV, SIGBUS */
> +    struct
> +    {
> +      nat_uptr_t _addr;
> +      short int _addr_lsb;
> +      struct
> +      {
> +	nat_uptr_t _lower;
> +	nat_uptr_t _upper;
> +      } si_addr_bnd;
> +    } _sigfault;
> +
> +    /* SIGPOLL */
> +    struct
> +    {
> +      int _band;
> +      int _fd;
> +    } _sigpoll;
> +  } _sifields;
> +} nat_siginfo_t __attribute__ ((__aligned__ (8)));
>  
>  /* These types below (compat_*) define a siginfo type that is layout
>     compatible with the siginfo type exported by the 32-bit userspace 
> @@ -101,6 +187,12 @@ typedef struct compat_siginfo
>      struct
>      {
>        unsigned int _addr;
> +      short int _addr_lsb;
> +      struct
> +      {
> +	unsigned int _lower;
> +	unsigned int _upper;
> +      } si_addr_bnd;
>      } _sigfault;
>  
>      /* SIGPOLL */
> @@ -162,6 +254,7 @@ typedef struct compat_x32_siginfo
>      struct
>      {
>        unsigned int _addr;
> +      unsigned int _addr_lsb;
>      } _sigfault;
>  
>      /* SIGPOLL */
> @@ -184,6 +277,7 @@ typedef struct compat_x32_siginfo  #define 
> cpt_si_stime _sifields._sigchld._stime  #define cpt_si_ptr 
> _sifields._rt._sigval.sival_ptr  #define cpt_si_addr 
> _sifields._sigfault._addr
> +#define cpt_si_addr_lsb _sifields._sigfault._addr_lsb
>  #define cpt_si_band _sifields._sigpoll._band  #define cpt_si_fd 
> _sifields._sigpoll._fd
>  
> @@ -200,54 +294,58 @@ typedef struct compat_x32_siginfo  static void  
> compat_siginfo_from_siginfo (compat_siginfo_t *to, siginfo_t *from)  {
> +  nat_siginfo_t from_nat;
> +
> +  gdb_assert (sizeof (nat_siginfo_t) == sizeof (siginfo_t));  memcpy 
> + (&from_nat, from, sizeof (from_nat));
>    memset (to, 0, sizeof (*to));
>  
> -  to->si_signo = from->si_signo;
> -  to->si_errno = from->si_errno;
> -  to->si_code = from->si_code;
> +  to->si_signo = from_nat.si_signo;
> +  to->si_errno = from_nat.si_errno;
> +  to->si_code = from_nat.si_code;
>  
>    if (to->si_code == SI_TIMER)
>      {
> -      to->cpt_si_timerid = from->si_timerid;
> -      to->cpt_si_overrun = from->si_overrun;
> -      to->cpt_si_ptr = (intptr_t) from->si_ptr;
> +      to->cpt_si_timerid = from_nat.cpt_si_timerid;
> +      to->cpt_si_overrun = from_nat.cpt_si_overrun;
> +      to->cpt_si_ptr = (intptr_t) from_nat.cpt_si_ptr;
>      }
>    else if (to->si_code == SI_USER)
>      {
> -      to->cpt_si_pid = from->si_pid;
> -      to->cpt_si_uid = from->si_uid;
> +      to->cpt_si_pid = from_nat.cpt_si_pid;
> +      to->cpt_si_uid = from_nat.cpt_si_uid;
>      }
>    else if (to->si_code < 0)
>      {
> -      to->cpt_si_pid = from->si_pid;
> -      to->cpt_si_uid = from->si_uid;
> -      to->cpt_si_ptr = (intptr_t) from->si_ptr;
> +      to->cpt_si_pid = from_nat.cpt_si_pid;
> +      to->cpt_si_uid = from_nat.cpt_si_uid;
> +      to->cpt_si_ptr = (intptr_t) from_nat.cpt_si_ptr;
>      }
>    else
>      {
>        switch (to->si_signo)
>  	{
>  	case SIGCHLD:
> -	  to->cpt_si_pid = from->si_pid;
> -	  to->cpt_si_uid = from->si_uid;
> -	  to->cpt_si_status = from->si_status;
> -	  to->cpt_si_utime = from->si_utime;
> -	  to->cpt_si_stime = from->si_stime;
> +	  to->cpt_si_pid = from_nat.cpt_si_pid;
> +	  to->cpt_si_uid = from_nat.cpt_si_uid;
> +	  to->cpt_si_status = from_nat.cpt_si_status;
> +	  to->cpt_si_utime = from_nat.cpt_si_utime;
> +	  to->cpt_si_stime = from_nat.cpt_si_stime;
>  	  break;
>  	case SIGILL:
>  	case SIGFPE:
>  	case SIGSEGV:
>  	case SIGBUS:
> -	  to->cpt_si_addr = (intptr_t) from->si_addr;
> +	  to->cpt_si_addr = (intptr_t) from_nat.cpt_si_addr;
>  	  break;
>  	case SIGPOLL:
> -	  to->cpt_si_band = from->si_band;
> -	  to->cpt_si_fd = from->si_fd;
> +	  to->cpt_si_band = from_nat.cpt_si_band;
> +	  to->cpt_si_fd = from_nat.cpt_si_fd;
>  	  break;
>  	default:
> -	  to->cpt_si_pid = from->si_pid;
> -	  to->cpt_si_uid = from->si_uid;
> -	  to->cpt_si_ptr = (intptr_t) from->si_ptr;
> +	  to->cpt_si_pid = from_nat.cpt_si_pid;
> +	  to->cpt_si_uid = from_nat.cpt_si_uid;
> +	  to->cpt_si_ptr = (intptr_t) from_nat.cpt_si_ptr;
>  	  break;
>  	}
>      }
> @@ -257,57 +355,62 @@ compat_siginfo_from_siginfo (compat_siginfo_t 
> *to, siginfo_t *from)  static void  siginfo_from_compat_siginfo 
> (siginfo_t *to, compat_siginfo_t *from)  {
> -  memset (to, 0, sizeof (*to));
> +  nat_siginfo_t to_nat;
>  
> -  to->si_signo = from->si_signo;
> -  to->si_errno = from->si_errno;
> -  to->si_code = from->si_code;
> +  gdb_assert (sizeof (nat_siginfo_t) == sizeof (siginfo_t));  memset 
> + (&to_nat, 0, sizeof (to_nat));
>  
> -  if (to->si_code == SI_TIMER)
> +  to_nat.si_signo = from->si_signo;
> +  to_nat.si_errno = from->si_errno;
> +  to_nat.si_code = from->si_code;
> +
> +  if (to_nat.si_code == SI_TIMER)
>      {
> -      to->si_timerid = from->cpt_si_timerid;
> -      to->si_overrun = from->cpt_si_overrun;
> -      to->si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
> +      to_nat.cpt_si_timerid = from->cpt_si_timerid;
> +      to_nat.cpt_si_overrun = from->cpt_si_overrun;
> +      to_nat.cpt_si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
>      }
> -  else if (to->si_code == SI_USER)
> +  else if (to_nat.si_code == SI_USER)
>      {
> -      to->si_pid = from->cpt_si_pid;
> -      to->si_uid = from->cpt_si_uid;
> +      to_nat.cpt_si_pid = from->cpt_si_pid;
> +      to_nat.cpt_si_uid = from->cpt_si_uid;
>      }
> -  if (to->si_code < 0)
> +  if (to_nat.si_code < 0)
>      {
> -      to->si_pid = from->cpt_si_pid;
> -      to->si_uid = from->cpt_si_uid;
> -      to->si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
> +      to_nat.cpt_si_pid = from->cpt_si_pid;
> +      to_nat.cpt_si_uid = from->cpt_si_uid;
> +      to_nat.cpt_si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
>      }
>    else
>      {
> -      switch (to->si_signo)
> +      switch (to_nat.si_signo)
>  	{
>  	case SIGCHLD:
> -	  to->si_pid = from->cpt_si_pid;
> -	  to->si_uid = from->cpt_si_uid;
> -	  to->si_status = from->cpt_si_status;
> -	  to->si_utime = from->cpt_si_utime;
> -	  to->si_stime = from->cpt_si_stime;
> +	  to_nat.cpt_si_pid = from->cpt_si_pid;
> +	  to_nat.cpt_si_uid = from->cpt_si_uid;
> +	  to_nat.cpt_si_status = from->cpt_si_status;
> +	  to_nat.cpt_si_utime = from->cpt_si_utime;
> +	  to_nat.cpt_si_stime = from->cpt_si_stime;
>  	  break;
>  	case SIGILL:
>  	case SIGFPE:
>  	case SIGSEGV:
>  	case SIGBUS:
> -	  to->si_addr = (void *) (intptr_t) from->cpt_si_addr;
> +	  to_nat.cpt_si_addr = (void *) (intptr_t) from->cpt_si_addr;
> +	  to_nat.cpt_si_addr_lsb = (short) from->cpt_si_addr_lsb;
>  	  break;
>  	case SIGPOLL:
> -	  to->si_band = from->cpt_si_band;
> -	  to->si_fd = from->cpt_si_fd;
> +	  to_nat.cpt_si_band = from->cpt_si_band;
> +	  to_nat.cpt_si_fd = from->cpt_si_fd;
>  	  break;
>  	default:
> -	  to->si_pid = from->cpt_si_pid;
> -	  to->si_uid = from->cpt_si_uid;
> -	  to->si_ptr = (void* ) (intptr_t) from->cpt_si_ptr;
> +	  to_nat.cpt_si_pid = from->cpt_si_pid;
> +	  to_nat.cpt_si_uid = from->cpt_si_uid;
> +	  to_nat.cpt_si_ptr = (void* ) (intptr_t) from->cpt_si_ptr;
>  	  break;
>  	}
>      }
> +  memcpy (to, &to_nat, sizeof (to_nat));
>  }
>  
>  /*  Converts the system provided siginfo into compatible x32 siginfo.  
> */ @@ -315,56 +418,60 @@ static void  compat_x32_siginfo_from_siginfo 
> (compat_x32_siginfo_t *to,
>  				 siginfo_t *from)
>  {
> +  nat_siginfo_t from_nat;
> +
> +  gdb_assert (sizeof (nat_siginfo_t) == sizeof (siginfo_t));  memcpy 
> + (&from_nat, from, sizeof (from_nat));
>    memset (to, 0, sizeof (*to));
>  
> -  to->si_signo = from->si_signo;
> -  to->si_errno = from->si_errno;
> -  to->si_code = from->si_code;
> +  to->si_signo = from_nat.si_signo;
> +  to->si_errno = from_nat.si_errno;
> +  to->si_code = from_nat.si_code;
>  
>    if (to->si_code == SI_TIMER)
>      {
> -      to->cpt_si_timerid = from->si_timerid;
> -      to->cpt_si_overrun = from->si_overrun;
> -      to->cpt_si_ptr = (intptr_t) from->si_ptr;
> +      to->cpt_si_timerid = from_nat.cpt_si_timerid;
> +      to->cpt_si_overrun = from_nat.cpt_si_overrun;
> +      to->cpt_si_ptr = (intptr_t) from_nat.cpt_si_ptr;
>      }
>    else if (to->si_code == SI_USER)
>      {
> -      to->cpt_si_pid = from->si_pid;
> -      to->cpt_si_uid = from->si_uid;
> +      to->cpt_si_pid = from_nat.cpt_si_pid;
> +      to->cpt_si_uid = from_nat.cpt_si_uid;
>      }
>    else if (to->si_code < 0)
>      {
> -      to->cpt_si_pid = from->si_pid;
> -      to->cpt_si_uid = from->si_uid;
> -      to->cpt_si_ptr = (intptr_t) from->si_ptr;
> +      to->cpt_si_pid = from_nat.cpt_si_pid;
> +      to->cpt_si_uid = from_nat.cpt_si_uid;
> +      to->cpt_si_ptr = (intptr_t) from_nat.cpt_si_ptr;
>      }
>    else
>      {
>        switch (to->si_signo)
>  	{
>  	case SIGCHLD:
> -	  to->cpt_si_pid = from->si_pid;
> -	  to->cpt_si_uid = from->si_uid;
> -	  to->cpt_si_status = from->si_status;
> -	  memcpy (&to->cpt_si_utime, &from->si_utime,
> +	  to->cpt_si_pid = from_nat.cpt_si_pid;
> +	  to->cpt_si_uid = from_nat.cpt_si_uid;
> +	  to->cpt_si_status = from_nat.cpt_si_status;
> +	  memcpy (&to->cpt_si_utime, &from_nat.cpt_si_utime,
>  		  sizeof (to->cpt_si_utime));
> -	  memcpy (&to->cpt_si_stime, &from->si_stime,
> +	  memcpy (&to->cpt_si_stime, &from_nat.cpt_si_stime,
>  		  sizeof (to->cpt_si_stime));
>  	  break;
>  	case SIGILL:
>  	case SIGFPE:
>  	case SIGSEGV:
>  	case SIGBUS:
> -	  to->cpt_si_addr = (intptr_t) from->si_addr;
> +	  to->cpt_si_addr = (intptr_t) from_nat.cpt_si_addr;
>  	  break;
>  	case SIGPOLL:
> -	  to->cpt_si_band = from->si_band;
> -	  to->cpt_si_fd = from->si_fd;
> +	  to->cpt_si_band = from_nat.cpt_si_band;
> +	  to->cpt_si_fd = from_nat.cpt_si_fd;
>  	  break;
>  	default:
> -	  to->cpt_si_pid = from->si_pid;
> -	  to->cpt_si_uid = from->si_uid;
> -	  to->cpt_si_ptr = (intptr_t) from->si_ptr;
> +	  to->cpt_si_pid = from_nat.cpt_si_pid;
> +	  to->cpt_si_uid = from_nat.cpt_si_uid;
> +	  to->cpt_si_ptr = (intptr_t) from_nat.cpt_si_ptr;
>  	  break;
>  	}
>      }
> @@ -375,59 +482,63 @@ static void
>  siginfo_from_compat_x32_siginfo (siginfo_t *to,
>  				 compat_x32_siginfo_t *from)
>  {
> -  memset (to, 0, sizeof (*to));
> +  nat_siginfo_t to_nat;
>  
> -  to->si_signo = from->si_signo;
> -  to->si_errno = from->si_errno;
> -  to->si_code = from->si_code;
> +  gdb_assert (sizeof (nat_siginfo_t) == sizeof (siginfo_t));  memset 
> + (&to_nat, 0, sizeof (to_nat));
>  
> -  if (to->si_code == SI_TIMER)
> +  to_nat.si_signo = from->si_signo;
> +  to_nat.si_errno = from->si_errno;
> +  to_nat.si_code = from->si_code;
> +
> +  if (to_nat.si_code == SI_TIMER)
>      {
> -      to->si_timerid = from->cpt_si_timerid;
> -      to->si_overrun = from->cpt_si_overrun;
> -      to->si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
> +      to_nat.cpt_si_timerid = from->cpt_si_timerid;
> +      to_nat.cpt_si_overrun = from->cpt_si_overrun;
> +      to_nat.cpt_si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
>      }
> -  else if (to->si_code == SI_USER)
> +  else if (to_nat.si_code == SI_USER)
>      {
> -      to->si_pid = from->cpt_si_pid;
> -      to->si_uid = from->cpt_si_uid;
> +      to_nat.cpt_si_pid = from->cpt_si_pid;
> +      to_nat.cpt_si_uid = from->cpt_si_uid;
>      }
> -  if (to->si_code < 0)
> +  if (to_nat.si_code < 0)
>      {
> -      to->si_pid = from->cpt_si_pid;
> -      to->si_uid = from->cpt_si_uid;
> -      to->si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
> +      to_nat.cpt_si_pid = from->cpt_si_pid;
> +      to_nat.cpt_si_uid = from->cpt_si_uid;
> +      to_nat.cpt_si_ptr = (void *) (intptr_t) from->cpt_si_ptr;
>      }
>    else
>      {
> -      switch (to->si_signo)
> +      switch (to_nat.si_signo)
>  	{
>  	case SIGCHLD:
> -	  to->si_pid = from->cpt_si_pid;
> -	  to->si_uid = from->cpt_si_uid;
> -	  to->si_status = from->cpt_si_status;
> -	  memcpy (&to->si_utime, &from->cpt_si_utime,
> -		  sizeof (to->si_utime));
> -	  memcpy (&to->si_stime, &from->cpt_si_stime,
> -		  sizeof (to->si_stime));
> +	  to_nat.cpt_si_pid = from->cpt_si_pid;
> +	  to_nat.cpt_si_uid = from->cpt_si_uid;
> +	  to_nat.cpt_si_status = from->cpt_si_status;
> +	  memcpy (&to_nat.cpt_si_utime, &from->cpt_si_utime,
> +		  sizeof (to_nat.cpt_si_utime));
> +	  memcpy (&to_nat.cpt_si_stime, &from->cpt_si_stime,
> +		  sizeof (to_nat.cpt_si_stime));
>  	  break;
>  	case SIGILL:
>  	case SIGFPE:
>  	case SIGSEGV:
>  	case SIGBUS:
> -	  to->si_addr = (void *) (intptr_t) from->cpt_si_addr;
> +	  to_nat.cpt_si_addr = (void *) (intptr_t) from->cpt_si_addr;
>  	  break;
>  	case SIGPOLL:
> -	  to->si_band = from->cpt_si_band;
> -	  to->si_fd = from->cpt_si_fd;
> +	  to_nat.cpt_si_band = from->cpt_si_band;
> +	  to_nat.cpt_si_fd = from->cpt_si_fd;
>  	  break;
>  	default:
> -	  to->si_pid = from->cpt_si_pid;
> -	  to->si_uid = from->cpt_si_uid;
> -	  to->si_ptr = (void* ) (intptr_t) from->cpt_si_ptr;
> +	  to_nat.cpt_si_pid = from->cpt_si_pid;
> +	  to_nat.cpt_si_uid = from->cpt_si_uid;
> +	  to_nat.cpt_si_ptr = (void* ) (intptr_t) from->cpt_si_ptr;
>  	  break;
>  	}
>      }
> +  memcpy (to, &to_nat, sizeof (to_nat));
>  }
>  
>  /* Convert a native/host siginfo object, into/from the siginfo in the
> 

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928