Re: [PATCH v2 1/2] Refactor sigcontextinfo.h

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h b/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
> index 9eec807a23..af1bcd21e2 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/sigcontextinfo.h
> @@ -16,5 +16,38 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#define SIGCONTEXT struct sigcontext *
> -#define GET_PC(__ctx)	((void *) ((__ctx)->si_regs.pc))
> +#ifndef _SIGCONTEXTINFO_H
> +#define _SIGCONTEXTINFO_H
> +
> +/* The kernel sparc32 signal frame for SA_SIGINFO is layout as:

“is defined as”?

> +  struct rt_signal_frame32 {
> +   struct sparc_stackf32 ss;
> +   compat_siginfo_t info;
> +   struct pt_regs32 regs;
> +   compat_sigset_t mask;   
> +   u32 fpu_save;
> +   unsigned int insns[2];
> +   compat_stack_t stack;
> +   unsigned int extra_size;
> +   siginfo_extra_v8plus_t v8plus;
> +   u32 rwin_save;
> +  } __attribute__((aligned(8)));

Isn't it GNU style to put the { on a line by itself (unindenteed)?
This is also relevant to the code, see struct kernel_sigset_t a bit
below in the posted patch.

> +  And different that other architectures, SPARC32 pass the pt_regs32
> +  REGS pointer as third argument for sa_sigaction handler with 
> +  SA_SIGINFO.  */

“Unlike other architectures, sparc32 passes pt_regs32 REGS pointers as
the third argument to a sa_sigaction handler with SA_SIGINFO enabled.”
or something like that?

“Different that” appears in a few other places as well which also need
fixing.

> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h b/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
> index 0f1078ad9e..722f06e09d 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/register-dump.h
> @@ -60,17 +60,36 @@ hexvalue (unsigned long int value, char *buf, size_t len)
>      *--cp = '0';
>  }
>  
> +/* Different that other architectures, SPARC32 pass a pt_regs (or pt_regs32
> +   in 32 bits compat mode) struct pointer as third argument for sa_sigaction
> +   handler with SA_SIGINFO.

I think this is the sparc64 header, so the comment needs to be adjusted.

> diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h b/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
> index a2f2b1f7de..744b51c585 100644
> --- a/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
> +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/sigcontextinfo.h
> @@ -16,8 +16,45 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#ifndef _SIGCONTEXTINFO_H
> +#define _SIGCONTEXTINFO_H
> +
> +#include <bits/types/siginfo_t.h>
> +
> +/* The kernel sparc64 signal frame for SA_SIGINFO is layout as:
> +
> +   struct rt_signal_frame {
> +     struct sparc_stackf ss;
> +     siginfo_t info;
> +     struct pt_regs regs; 
> +     __siginfo_fpu_t *fpu_save;
> +     stack_t stack;
> +     sigset_t mask;
> +     __siginfo_rwin_t *rwin_save; 
> +   };
> +
> +   And different that other architectures, SPARC64 pass the siginfo_t
> +   INFO pointer as third argument for sa_sigaction handler with 
> +   SA_SIGINFO.  */
> +
>  #ifndef STACK_BIAS
>  #define STACK_BIAS 2047
>  #endif
> -#define SIGCONTEXT struct sigcontext *
> -#define GET_PC(__ctx)	((void *) ((__ctx)->sigc_regs.tpc))
> +
> +struct pt_regs
> +{
> + unsigned long u_regs[16];
> + unsigned long tstate;
> + unsigned long tpc;
> + unsigned long tnpc;
> + unsigned int y;
> + unsigned int magic;
> +}; 

Wrong indentation, and “unsigned long int” should be used.

The patch has trailing whitespace in a few places, FWIW.

Overall, I'm in favor of this change.  I do wonder if GET_PC should be
an inline function instead, so that we get type checking for its
argument (even if the implementation needs to cast the pointer to
something else).

Thnaks,
Florian