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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 15/08/2019 11:07, Florian Weimer wrote:
> * 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”?

Ack, I will use your suggestion.

> 
>> +  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.

Well this is copies from kernel source (arch/sparc/kernel/signal32.c:59),
I will change to GNU style.

> 
>> +  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?

Ack, I changed to your suggestion.

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

Ack (it seems it appears on ia64 and sparc64 as well, I changed them too).

> 
>> 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.

Ack, I used a similar text you suggested for sparc32.

> 
>> 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.  */


Btw, I used a similar text you suggested for sparc32.

>> +
>>  #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.

Ack.

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

Ack.

> 
> 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).

Good suggestion, I will change to an inline function.