Re: PATCH: Support x32 siginfo_t conversion

[Mark Kettenis <mark.kettenis@xs4all.nl>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4432 bytes --]

> Date: Fri, 11 May 2012 13:55:29 -0700
> From: "H.J. Lu" <hjl.tools@gmail.com>
> 
> On Fri, May 11, 2012 at 1:11 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >> Date: Fri, 11 May 2012 12:21:33 -0700
> >> From: "H.J. Lu" <hongjiu.lu@intel.com>
> >>
> >> Hi,
> >>
> >> This patch implements x32 siginfo_t conversion.  Tested on Linux/x86-64.
> >> OK to install?
> >>
> >> Thanks.
> >>
> >>
> >> H.J.
> >> --
> >>       * amd64-linux-nat.c (compat_x32_clock_t): New.
> >>       (compat_x32_siginfo_t): Likewise.
> >>       (compat_x32_siginfo_from_siginfo): Likewise.
> >>       (siginfo_from_compat_x32_siginfo): Likewise.
> >>       (amd64_linux_siginfo_fixup): Call compat_x32_siginfo_from_siginfo
> >>       and siginfo_from_compat_x32_siginfo for x32.
> >>
> >> diff --git a/gdb/amd64-linux-nat.c b/gdb/amd64-linux-nat.c
> >> index 3be8404..97c9a49 100644
> >> --- a/gdb/amd64-linux-nat.c
> >> +++ b/gdb/amd64-linux-nat.c
> >> @@ -591,6 +591,67 @@ typedef struct compat_siginfo
> >>    } _sifields;
> >>  } compat_siginfo_t;
> >>
> >> +/* For x32, clock_t in _sigchld is 64bit aligned at 4 bytes.  */
> >> +typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
> >
> > Sorry, but that isn't acceptable.
> >
> > Is your X32 ABI really that broken?
> >
> >> +typedef struct compat_x32_siginfo
> >> +{
> >> +  int si_signo;
> >> +  int si_errno;
> >> +  int si_code;
> >> +
> >> +  union
> >> +  {
> >> +    int _pad[((128 / sizeof (int)) - 3)];
> >> +
> >> +    /* kill() */
> >> +    struct
> >> +    {
> >> +      unsigned int _pid;
> >> +      unsigned int _uid;
> >> +    } _kill;
> >> +
> >> +    /* POSIX.1b timers */
> >> +    struct
> >> +    {
> >> +      compat_timer_t _tid;
> >> +      int _overrun;
> >> +      compat_sigval_t _sigval;
> >> +    } _timer;
> >> +
> >> +    /* POSIX.1b signals */
> >> +    struct
> >> +    {
> >> +      unsigned int _pid;
> >> +      unsigned int _uid;
> >> +      compat_sigval_t _sigval;
> >> +    } _rt;
> >> +
> >> +    /* SIGCHLD */
> >> +    struct
> >> +    {
> >> +      unsigned int _pid;
> >> +      unsigned int _uid;
> >> +      int _status;
> >> +      compat_x32_clock_t _utime;
> >> +      compat_x32_clock_t _stime;
> >> +    } _sigchld;
> >> +
> >> +    /* SIGILL, SIGFPE, SIGSEGV, SIGBUS */
> >> +    struct
> >> +    {
> >> +      unsigned int _addr;
> >> +    } _sigfault;
> >> +
> >> +    /* SIGPOLL */
> >> +    struct
> >> +    {
> >> +      int _band;
> >> +      int _fd;
> >> +    } _sigpoll;
> >> +  } _sifields;
> >> +} compat_x32_siginfo_t __attribute__ ((__aligned__ (8)));
> >
> > Same here.  I don't think you need alignment here, even with the broken ABI.
> >
> > If it really is too late to fix the X32 ABI, you'll have to write this
> > portably by splitting _utime and _stime into two 32-bit variables and
> > write code that correctly sets the upper and lower 32-bits.
> >
> >
> 
> X32 ABI choice is done on purpose.  X32 siginfo_t has
> 
> typedef long __attribute__ ((__aligned__ (4))) compat_x32_clock_t;
> 
> typedef struct compat_x32_siginfo
> {
>   int si_signo;
>   int si_errno;
>   int si_code;
> 
>   union
>   {
> ...
>     /* SIGCHLD */
>     struct
>     {
>       unsigned int _pid;
>       unsigned int _uid;
>       int _status;
>       compat_x32_clock_t _utime;
>       compat_x32_clock_t _stime;
>     } _sigchld;
> ...
> } compat_x32_siginfo_t __attribute__ ((__aligned__ (8)));
> 
> struct info is aligned at 8 bytes and type of _utime/_stime is aligned
> at 4 bytes.  However,  _utime offset is 3 * 4 + 3 * 4 == 24 bytes. So
> in reality, the addresses of _utime/_stime are 8 bytes aligned.  There
> are no needs to split _utime and _stime into two 32-bit variables
> since their addresses are 64bits aligned.

But there is no way you can easily express that syntax with standard C
syntax[1].  That's why you had to resort to using GCC's __attribute__
syntax.  For GDB you'll have to figure out a way to do this without
using __attribute__ ((__aligned__ (...))).

My recommendation would be to define a compat_x32 structure just for
SIGCHLD (without using a union) and use the normal 32-bit comap
structure for all the other conversions.

Cheers,

Mark


[1] Well there is in C11, but you can't rely on that being properly
    implemented for at least another 5 years or so.