[PATCH] m68k: use conventional function parameters for do_sigreturn

[PATCH] m68k: use conventional function parameters for do_sigreturn

[Greg Ungerer]

Create conventional stack parameters for the calls to do_sigreturn and
do_rt_sigreturn. The current C code for do_sigreturn and do_rt_sigreturn
dig into the stack to create local pointers to the saved switch stack
and the pt_regs structs.

Using conventional stack parameters passed to these functions means the
code here does not need to know the exact details of how the underlying
entry handler layed these structs out on the stack.

The motivation for this change is a problem with non-MMU targets that
have broken signal return paths on newer versions of gcc. It appears as
though aliasing of the regs and switch stack pointers, caused by their
construction from pointers derived from the dummy long function parameter,
is resulting in the gcc optimizer removing what it thinks is useless
updates to the regs fields. Large parts of restore_sigcontext() and
mangle_kernel_stack() functions get optimized out. Of course this results
in non-functional code causing kernel oops. This problem has been observed
with gcc version 5.2 and 5.3, and probably exists in earlier versions as
well.

The resulting code after this change is a few bytes larger (due to the
overhead of creating the stack args and their tear down). Not being hot
paths I don't think this is too much of a problem here.

This change has been compile tested on all defconfigs, and run tested on
Atari (through aranym), ColdFire with MMU (M5407EVB) and ColdFire with
no-MMU (QEMU and M5208EVB).

Signed-off-by: Greg Ungerer <gerg@uclinux.org>
---
 arch/m68k/kernel/entry.S  | 6 ++++++
 arch/m68k/kernel/signal.c | 8 ++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
index b54ac7a..97cd3ea 100644
--- a/arch/m68k/kernel/entry.S
+++ b/arch/m68k/kernel/entry.S
@@ -71,13 +71,19 @@ ENTRY(__sys_vfork)
 
 ENTRY(sys_sigreturn)
 	SAVE_SWITCH_STACK
+	movel	%sp,%sp@-		  | switch_stack pointer
+	pea	%sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer
 	jbsr	do_sigreturn
+	addql	#8,%sp
 	RESTORE_SWITCH_STACK
 	rts
 
 ENTRY(sys_rt_sigreturn)
 	SAVE_SWITCH_STACK
+	movel	%sp,%sp@-		  | switch_stack pointer
+	pea	%sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer
 	jbsr	do_rt_sigreturn
+	addql	#8,%sp
 	RESTORE_SWITCH_STACK
 	rts
 
diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
index af1c4f3..2dcee3a 100644
--- a/arch/m68k/kernel/signal.c
+++ b/arch/m68k/kernel/signal.c
@@ -737,10 +737,8 @@ badframe:
 	return 1;
 }
 
-asmlinkage int do_sigreturn(unsigned long __unused)
+asmlinkage int do_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
 {
-	struct switch_stack *sw = (struct switch_stack *) &__unused;
-	struct pt_regs *regs = (struct pt_regs *) (sw + 1);
 	unsigned long usp = rdusp();
 	struct sigframe __user *frame = (struct sigframe __user *)(usp - 4);
 	sigset_t set;
@@ -764,10 +762,8 @@ badframe:
 	return 0;
 }
 
-asmlinkage int do_rt_sigreturn(unsigned long __unused)
+asmlinkage int do_rt_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
 {
-	struct switch_stack *sw = (struct switch_stack *) &__unused;
-	struct pt_regs *regs = (struct pt_regs *) (sw + 1);
 	unsigned long usp = rdusp();
 	struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(usp - 4);
 	sigset_t set;
-- 
1.9.1

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Greg Ungerer]

Anyone have any thoughts or comments on this?


On 19/01/16 15:56, Greg Ungerer wrote:
> Create conventional stack parameters for the calls to do_sigreturn and
> do_rt_sigreturn. The current C code for do_sigreturn and do_rt_sigreturn
> dig into the stack to create local pointers to the saved switch stack
> and the pt_regs structs.
> 
> Using conventional stack parameters passed to these functions means the
> code here does not need to know the exact details of how the underlying
> entry handler layed these structs out on the stack.
> 
> The motivation for this change is a problem with non-MMU targets that
> have broken signal return paths on newer versions of gcc. It appears as
> though aliasing of the regs and switch stack pointers, caused by their
> construction from pointers derived from the dummy long function parameter,
> is resulting in the gcc optimizer removing what it thinks is useless
> updates to the regs fields. Large parts of restore_sigcontext() and
> mangle_kernel_stack() functions get optimized out. Of course this results
> in non-functional code causing kernel oops. This problem has been observed
> with gcc version 5.2 and 5.3, and probably exists in earlier versions as
> well.
> 
> The resulting code after this change is a few bytes larger (due to the
> overhead of creating the stack args and their tear down). Not being hot
> paths I don't think this is too much of a problem here.
> 
> This change has been compile tested on all defconfigs, and run tested on
> Atari (through aranym), ColdFire with MMU (M5407EVB) and ColdFire with
> no-MMU (QEMU and M5208EVB).
> 
> Signed-off-by: Greg Ungerer <gerg@uclinux.org>
> ---
>  arch/m68k/kernel/entry.S  | 6 ++++++
>  arch/m68k/kernel/signal.c | 8 ++------
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/m68k/kernel/entry.S b/arch/m68k/kernel/entry.S
> index b54ac7a..97cd3ea 100644
> --- a/arch/m68k/kernel/entry.S
> +++ b/arch/m68k/kernel/entry.S
> @@ -71,13 +71,19 @@ ENTRY(__sys_vfork)
>  
>  ENTRY(sys_sigreturn)
>  	SAVE_SWITCH_STACK
> +	movel	%sp,%sp@-		  | switch_stack pointer
> +	pea	%sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer
>  	jbsr	do_sigreturn
> +	addql	#8,%sp
>  	RESTORE_SWITCH_STACK
>  	rts
>  
>  ENTRY(sys_rt_sigreturn)
>  	SAVE_SWITCH_STACK
> +	movel	%sp,%sp@-		  | switch_stack pointer
> +	pea	%sp@(SWITCH_STACK_SIZE+4) | pt_regs pointer
>  	jbsr	do_rt_sigreturn
> +	addql	#8,%sp
>  	RESTORE_SWITCH_STACK
>  	rts
>  
> diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
> index af1c4f3..2dcee3a 100644
> --- a/arch/m68k/kernel/signal.c
> +++ b/arch/m68k/kernel/signal.c
> @@ -737,10 +737,8 @@ badframe:
>  	return 1;
>  }
>  
> -asmlinkage int do_sigreturn(unsigned long __unused)
> +asmlinkage int do_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
>  {
> -	struct switch_stack *sw = (struct switch_stack *) &__unused;
> -	struct pt_regs *regs = (struct pt_regs *) (sw + 1);
>  	unsigned long usp = rdusp();
>  	struct sigframe __user *frame = (struct sigframe __user *)(usp - 4);
>  	sigset_t set;
> @@ -764,10 +762,8 @@ badframe:
>  	return 0;
>  }
>  
> -asmlinkage int do_rt_sigreturn(unsigned long __unused)
> +asmlinkage int do_rt_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
>  {
> -	struct switch_stack *sw = (struct switch_stack *) &__unused;
> -	struct pt_regs *regs = (struct pt_regs *) (sw + 1);
>  	unsigned long usp = rdusp();
>  	struct rt_sigframe __user *frame = (struct rt_sigframe __user *)(usp - 4);
>  	sigset_t set;
> 

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Andreas Schwab]

Greg Ungerer <gerg@uclinux.org> writes:

> The motivation for this change is a problem with non-MMU targets that
> have broken signal return paths on newer versions of gcc. It appears as
> though aliasing of the regs and switch stack pointers, caused by their
> construction from pointers derived from the dummy long function parameter,
> is resulting in the gcc optimizer removing what it thinks is useless
> updates to the regs fields.

I wonder why gcc is doing that.  The kernel is explicitly built without
strict aliasing, so this looks like something to investigate.

> -asmlinkage int do_sigreturn(unsigned long __unused)
> +asmlinkage int do_sigreturn(struct pt_regs *regs, struct switch_stack *sw)

Does it work to use this signature:

asmlinkage int do_sigreturn(struct switch_stack sw, struct pt_regs regs)

without changing the caller?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Greg Ungerer]

Hi Andreas,

On 02/02/16 04:39, Andreas Schwab wrote:
> Greg Ungerer <gerg@uclinux.org> writes:
> 
>> The motivation for this change is a problem with non-MMU targets that
>> have broken signal return paths on newer versions of gcc. It appears as
>> though aliasing of the regs and switch stack pointers, caused by their
>> construction from pointers derived from the dummy long function parameter,
>> is resulting in the gcc optimizer removing what it thinks is useless
>> updates to the regs fields.
> 
> I wonder why gcc is doing that.  The kernel is explicitly built without
> strict aliasing, so this looks like something to investigate.

Agreed. My initial thoughts were this is a problem with gcc.
And I am not entirely sure it still isn't.


>> -asmlinkage int do_sigreturn(unsigned long __unused)
>> +asmlinkage int do_sigreturn(struct pt_regs *regs, struct switch_stack *sw)
> 
> Does it work to use this signature:
> 
> asmlinkage int do_sigreturn(struct switch_stack sw, struct pt_regs regs)
> 
> without changing the caller?

No, same problem. (I had to fix the use of regs inside the function,
but that was trivial). The generated code is essentially similar,
and large parts if it end up optimized away.

Original unmodified kernel code generates a do_sigreturn() as:

40021aaa <do_sigreturn>:
40021aaa:       4e56 ffd4       linkw %fp,#-44
40021aae:       2f0a            movel %a2,%sp@-
40021ab0:       4e68            movel %usp,%a0
40021ab2:       2028 0018       movel %a0@(24),%d0
40021ab6:       2d40 ffd4       movel %d0,%fp@(-44)
40021aba:       2448            moveal %a0,%a2
40021abc:       2d68 0014 ffd8  movel %a0@(20),%fp@(-40)
40021ac2:       486e ffd4       pea %fp@(-44)
40021ac6:       4eb9 4002 f0b8  jsr 4002f0b8 <set_current_blocked>
40021acc:       200f            movel %sp,%d0
40021ace:       0280 ffff e000  andil #-8192,%d0
40021ad4:       2240            moveal %d0,%a1
40021ad6:       203c 4002 f086  movel #1073934470,%d0
40021adc:       2051            moveal %a1@,%a0
40021ade:       2140 0154       movel %d0,%a0@(340)
40021ae2:       2d6a 0020 ffe4  movel %a2@(32),%fp@(-28)
40021ae8:       2d6a 001c ffe0  movel %a2@(28),%fp@(-32)
40021aee:       202e ffe4       movel %fp@(-28),%d0
40021af2:       206e ffe0       moveal %fp@(-32),%a0
40021af6:       4e60            movel %a0,%usp
40021af8:       246e ffd0       moveal %fp@(-48),%a2
40021afc:       588f            addql #4,%sp
40021afe:       4e5e            unlk %fp
40021b00:       4e75            rts


With your suggested change above it is generated as:

40021aaa <do_sigreturn>:
40021aaa:       4e56 ffd4       linkw %fp,#-44
40021aae:       2f0a            movel %a2,%sp@-
40021ab0:       4e68            movel %usp,%a0
40021ab2:       2448            moveal %a0,%a2
40021ab4:       202a 0018       movel %a2@(24),%d0
40021ab8:       2d40 ffd4       movel %d0,%fp@(-44)
40021abc:       2d6a 0014 ffd8  movel %a2@(20),%fp@(-40)
40021ac2:       486e ffd4       pea %fp@(-44)
40021ac6:       4eb9 4002 f0b8  jsr 4002f0b8 <set_current_blocked>
40021acc:       200f            movel %sp,%d0
40021ace:       0280 ffff e000  andil #-8192,%d0
40021ad4:       2240            moveal %d0,%a1
40021ad6:       203c 4002 f086  movel #1073934470,%d0
40021adc:       2051            moveal %a1@,%a0
40021ade:       2140 0154       movel %d0,%a0@(340)
40021ae2:       2d6a 001c ffe0  movel %a2@(28),%fp@(-32)
40021ae8:       2d6a 0020 ffe4  movel %a2@(32),%fp@(-28)
40021aee:       206e ffe0       moveal %fp@(-32),%a0
40021af2:       4e60            movel %a0,%usp
40021af4:       202e ffe4       movel %fp@(-28),%d0
40021af8:       588f            addql #4,%sp
40021afa:       246e ffd0       moveal %fp@(-48),%a2
40021afe:       4e5e            unlk %fp
40021b00:       4e75            rts


And finally with the patch I posted it generates:

40021aba <do_sigreturn>:
40021aba:	4e56 ffc0      	linkw %fp,#-64
40021abe:	48d7 0c1c      	moveml %d2-%d4/%a2-%a3,%sp@
40021ac2:	246e 0008      	moveal %fp@(8),%a2
40021ac6:	4e68           	movel %usp,%a0
40021ac8:	2648           	moveal %a0,%a3
40021aca:	202b 0018      	movel %a3@(24),%d0
40021ace:	2d40 ffd4      	movel %d0,%fp@(-44)
40021ad2:	2d6b 0014 ffd8 	movel %a3@(20),%fp@(-40)
40021ad8:	486e ffd4      	pea %fp@(-44)
40021adc:	4eb9 4002 f17c 	jsr 4002f17c <set_current_blocked>
40021ae2:	200f           	movel %sp,%d0
40021ae4:	0280 ffff e000 	andil #-8192,%d0
40021aea:	2240           	moveal %d0,%a1
40021aec:	203c 4002 f14a 	movel #1073934666,%d0
40021af2:	72ff           	moveq #-1,%d1
40021af4:	2051           	moveal %a1@,%a0
40021af6:	2140 0154      	movel %d0,%a0@(340)
40021afa:	2d6b 0020 ffe4 	movel %a3@(32),%fp@(-28)
40021b00:	2d6b 001c ffe0 	movel %a3@(28),%fp@(-32)
40021b06:	2d6b 0024 ffe8 	movel %a3@(36),%fp@(-24)
40021b0c:	2d6b 0028 ffec 	movel %a3@(40),%fp@(-20)
40021b12:	202e ffe4      	movel %fp@(-28),%d0
40021b16:	206e ffe0      	moveal %fp@(-32),%a0
40021b1a:	2d6b 002c fff0 	movel %a3@(44),%fp@(-16)
40021b20:	2d6b 0034 fff8 	movel %a3@(52),%fp@(-8)
40021b26:	2d6b 0038 fffc 	movel %a3@(56),%fp@(-4)
40021b2c:	24ae ffe8      	movel %fp@(-24),%a2@
40021b30:	256e ffec 0014 	movel %fp@(-20),%a2@(20)
40021b36:	256e fff0 0018 	movel %fp@(-16),%a2@(24)
40021b3c:	256e fffa 0030 	movel %fp@(-6),%a2@(48)
40021b42:	156e fff9 002f 	moveb %fp@(-7),%a2@(47)
40021b48:	2540 0020      	movel %d0,%a2@(32)
40021b4c:	2541 0024      	movel %d1,%a2@(36)
40021b50:	4e60           	movel %a0,%usp
40021b52:	780f           	moveq #15,%d4
40021b54:	588f           	addql #4,%sp
40021b56:	322e fffe      	movew %fp@(-2),%d1
40021b5a:	162a 002c      	moveb %a2@(44),%d3
40021b5e:	2401           	movel %d1,%d2
40021b60:	e08a           	lsrl #8,%d2
40021b62:	c684           	andl %d4,%d3
40021b64:	0282 0000 00f0 	andil #240,%d2
40021b6a:	0281 0000 0fff 	andil #4095,%d1
40021b70:	8483           	orl %d3,%d2
40021b72:	1542 002c      	moveb %d2,%a2@(44)
40021b76:	342a 002c      	movew %a2@(44),%d2
40021b7a:	0282 ffff f000 	andil #-4096,%d2
40021b80:	8282           	orl %d2,%d1
40021b82:	3541 002c      	movew %d1,%a2@(44)
40021b86:	4cee 0c1c ffc0 	moveml %fp@(-64),%d2-%d4/%a2-%a3
40021b8c:	4e5e           	unlk %fp
40021b8e:	4e75           	rts

The only generated code that works of these is last one -
with my patch applied.

Regards
Greg

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Andreas Schwab]

Greg Ungerer <gerg@uclinux.org> writes:

>> Does it work to use this signature:
>> 
>> asmlinkage int do_sigreturn(struct switch_stack sw, struct pt_regs regs)
>> 
>> without changing the caller?
>
> No, same problem.

So this is unrelated to aliasing.  Can you create a test case?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Greg Ungerer]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On 03/02/16 05:25, Andreas Schwab wrote:
> Greg Ungerer <gerg@uclinux.org> writes:
>>> Does it work to use this signature:
>>>
>>> asmlinkage int do_sigreturn(struct switch_stack sw, struct pt_regs regs)
>>>
>>> without changing the caller?
>>
>> No, same problem.
> 
> So this is unrelated to aliasing.  Can you create a test case?

Attached is a test case - derived from the original signal.c code.
I removed a lot to get it to a manageable size. It still exhibits
the problem. I compile this test case with:

  m68k-uclinux-gcc -Wall -mcpu=5208 -fno-strict-aliasing -O2 -c -o signal.o signal.i

I get the same result if I use a m68k-linux-gcc as well (I am
using gcc-5.3 based toolchains).

Regards
Greg




[-- Attachment #2: signal.i --]
[-- Type: text/plain, Size: 7447 bytes --]

typedef unsigned int __u32;
typedef unsigned int u32;
typedef unsigned long long u64;
typedef unsigned int __kernel_size_t;
typedef int __kernel_clockid_t;
typedef __kernel_clockid_t clockid_t;
typedef __kernel_size_t size_t;

typedef struct {
 unsigned long fds_bits[1024 / (8 * sizeof(long))];
} __kernel_fd_set;

typedef struct {
 unsigned long sig[(64 / 32)];
} sigset_t;

typedef struct sigaltstack {
 void *ss_sp;
 int ss_flags;
 size_t ss_size;
} stack_t;

struct sigcontext {
 unsigned long sc_mask;
 unsigned long sc_usp;
 unsigned long sc_d0;
 unsigned long sc_d1;
 unsigned long sc_a0;
 unsigned long sc_a1;
 unsigned long sc_a5;
 unsigned short sc_sr;
 unsigned long sc_pc;
 unsigned short sc_formatvec;
};
struct pt_regs {
 long d1;
 long d2;
 long d3;
 long d4;
 long d5;
 long a0;
 long a1;
 long a2;
 long d0;
 long orig_d0;
 long stkadj;

 unsigned format : 4;
 unsigned vector : 12;
 unsigned short sr;
 unsigned long pc;
};

struct switch_stack {
 unsigned long d6;
 unsigned long d7;
 unsigned long a3;
 unsigned long a4;
 unsigned long a5;
 unsigned long a6;
 unsigned long retpc;
};

struct timespec;

struct restart_block {
 long (*fn)(struct restart_block *);
 union {
  struct {
   u32 *uaddr;
   u32 val;
   u32 flags;
   u32 bitset;
   u64 time;
   u32 *uaddr2;
  } futex;

  struct {
   clockid_t clockid;
   struct timespec *rmtp;
   u64 expires;
  } nanosleep;

  struct {
   struct pollfd *ufds;
   int nfds;
   int has_timeout;
   unsigned long tv_sec;
   unsigned long tv_nsec;
  } poll;
 };
};

struct thread_struct {
 unsigned long ksp;
 unsigned long usp;
 unsigned short sr;
 unsigned short fs;
 unsigned long crp[2];
 unsigned long esp0;
 unsigned long faddr;
 int signo, code;
 unsigned long fp[8*3];
 unsigned long fpcntl[3];
 unsigned char fpstate[(0)];
};

struct mm_struct {
 u32 vmacache_seqnum;
 unsigned long start_code, end_code, start_data, end_data;
 unsigned long start_brk, brk, start_stack;
 unsigned long arg_start, arg_end, env_start, env_end;
};
struct task_struct {
 volatile long state;
 void *stack;
 struct mm_struct *mm, *active_mm;
 struct restart_block restart_block;
 int exit_state;
 int exit_code, exit_signal;
};

typedef struct {
 unsigned long seg;
} mm_segment_t;

struct thread_info {
 struct task_struct *task;
 unsigned long flags;
 mm_segment_t addr_limit;
 int preempt_count;
 __u32 cpu;
 unsigned long tp_value;
};

typedef int greg_t;
typedef greg_t gregset_t[18];

typedef struct fpregset {
 int f_fpcntl[3];
 int f_fpregs[8*3];
} fpregset_t;

struct mcontext {
 int version;
 gregset_t gregs;
 fpregset_t fpregs;
};

struct ucontext {
 unsigned long uc_flags;
 struct ucontext *uc_link;
 stack_t uc_stack;
 struct mcontext uc_mcontext;
 unsigned long uc_filler[80];
 sigset_t uc_sigmask;
};

struct sigframe
{
 char *pretcode;
 int sig;
 int code;
 struct sigcontext *psc;
 char retcode[8];
 unsigned long extramask[(64 / 32)-1];
 struct sigcontext sc;
};

extern void force_sig(int, struct task_struct *);
extern void set_current_blocked(sigset_t *);
extern long do_no_restart_syscall(struct restart_block *parm);
extern int __builtin_memcmp(const void *, const void *, __kernel_size_t);
extern int __get_user_bad(void);

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) struct thread_info *current_thread_info(void)
{
 struct thread_info *ti;
 __asm__(
  "move.l %%sp, %0 \n\t"
  "and.l  %1, %0"
  : "=&d"(ti)
  : "di" (~(((1UL) << 13)-1))
  );
 return ti;
}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) struct task_struct *get_current(void)
{
 return(current_thread_info()->task);
}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) unsigned long rdusp(void)
{
 register unsigned long usp __asm__("a0");
 __asm__ __volatile__(".word 0x4e68" : "=a" (usp));
 return usp;

}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) void wrusp(unsigned long usp)
{
 register unsigned long a0 __asm__("a0") = usp;
 __asm__ __volatile__(".word 0x4e60" : : "a" (a0) );

}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) int _access_ok(unsigned long addr, unsigned long size)
{
 return 1;
}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) int frame_extra_sizes(int f)
{
 return 0;
}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) int restore_fpu_state(struct sigcontext *sc)
{
 return 0;
}

static int mangle_kernel_stack(struct pt_regs *regs, int formatvec, void *fp)
{
 int fsize = frame_extra_sizes(formatvec >> 12);
 if (fsize < 0) {
  return 1;
 }
 if (!fsize) {
  regs->format = formatvec >> 12;
  regs->vector = formatvec & 0xfff;
 } else {
  struct switch_stack *sw = (struct switch_stack *)regs - 1;
  unsigned long buf[fsize / 2];

  if ((__builtin_memcpy(buf + fsize / 4, fp, fsize), 0))
   return 1;

  regs->format = formatvec >> 12;
  regs->vector = formatvec & 0xfff;

  __asm__ __volatile__ (

    "   movel %0,%/sp\n\t"
    "   bra ret_from_signal\n"
    :
    : "a" (sw), "d" (fsize), "d" ((sizeof(struct pt_regs)+sizeof(struct switch_stack))/4-1),
      "n" ((sizeof(struct pt_regs)+sizeof(struct switch_stack))), "a" (buf + fsize/4)
    : "a0");

 }
 return 0;
}

static inline __attribute__((always_inline)) __attribute__((no_instrument_function)) int
restore_sigcontext(struct pt_regs *regs, struct sigcontext *usc, void *fp)
{
 int formatvec;
 struct sigcontext context;
 int err = 0;

 get_current()->restart_block.fn = do_no_restart_syscall;

 if ((__builtin_memcpy(&context, usc, sizeof(context)), 0))
  goto badframe;

 regs->d0 = context.sc_d0;
 regs->d1 = context.sc_d1;
 regs->a0 = context.sc_a0;
 regs->a1 = context.sc_a1;
 regs->sr = (regs->sr & 0xff00) | (context.sc_sr & 0xff);
 regs->pc = context.sc_pc;
 regs->orig_d0 = -1;
 wrusp(context.sc_usp);
 formatvec = context.sc_formatvec;

 err = restore_fpu_state(&context);

 if (err || mangle_kernel_stack(regs, formatvec, fp))
  goto badframe;

 return 0;

badframe:
 return 1;
}

 int do_sigreturn(unsigned long __unused)
{
 struct switch_stack *sw = (struct switch_stack *) &__unused;
 struct pt_regs *regs = (struct pt_regs *) (sw + 1);
 unsigned long usp = rdusp();
 struct sigframe *frame = (struct sigframe *)(usp - 4);
 sigset_t set;

 if (!_access_ok((unsigned long)(frame),(sizeof(*frame))))
  goto badframe;
 if (({ int __gu_err = 0; typeof(set.sig[0]) __gu_val = 0; switch (sizeof(*(&frame->sc.sc_mask))) { case 1: __asm__ ("move" "b" " %1,%0" : "=d" (__gu_val) : "m" (*((unsigned long *)(&frame->sc.sc_mask)))); break; case 2: __asm__ ("move" "w" " %1,%0" : "=d" (__gu_val) : "m" (*((unsigned long *)(&frame->sc.sc_mask)))); break; case 4: __asm__ ("move" "l" " %1,%0" : "=d" (__gu_val) : "m" (*((unsigned long *)(&frame->sc.sc_mask)))); break; case 8: __builtin_memcpy((void *) &__gu_val, &frame->sc.sc_mask, sizeof (*(&frame->sc.sc_mask))); break; default: __gu_val = 0; __gu_err = __get_user_bad(); break; } (set.sig[0]) = (typeof(*(&frame->sc.sc_mask))) __gu_val; __gu_err; }) ||
     ((64 / 32) > 1 &&
      (__builtin_memcpy(&set.sig[1], &frame->extramask, sizeof(frame->extramask)), 0)
                                  ))
  goto badframe;

 set_current_blocked(&set);

 if (restore_sigcontext(regs, &frame->sc, frame + 1))
  goto badframe;
 return regs->d0;

badframe:
 force_sig(11, get_current());
 return 0;
}

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Andreas Schwab]

Greg Ungerer <gerg@uclinux.org> writes:

> Attached is a test case - derived from the original signal.c code.

It looks like with all those trivial functions in the non-MMU case the
compiler sees enough to consider the writes to the (pseudo) function
arguments dead.

How about this:

diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
index af1c4f3..3cc9eaa 100644
--- a/arch/m68k/kernel/signal.c
+++ b/arch/m68k/kernel/signal.c
@@ -757,6 +757,11 @@ asmlinkage int do_sigreturn(unsigned long __unused)
 
 	if (restore_sigcontext(regs, &frame->sc, frame + 1))
 		goto badframe;
+	/*
+	 * Force a barrier so that the compiler does not consider writes
+	 * to *sw and *regs as dead.
+	 */
+	barrier();
 	return regs->d0;
 
 badframe:
@@ -781,6 +786,11 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused)
 
 	if (rt_restore_ucontext(regs, sw, &frame->uc))
 		goto badframe;
+	/*
+	 * Force a barrier so that the compiler does not consider writes
+	 * to *sw and *regs as dead.
+	 */
+	barrier();
 	return regs->d0;
 
 badframe:

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Greg Ungerer]

Hi Andreas,

On 09/02/16 09:31, Andreas Schwab wrote:
> Greg Ungerer <gerg@uclinux.org> writes:
>> Attached is a test case - derived from the original signal.c code.
> 
> It looks like with all those trivial functions in the non-MMU case the
> compiler sees enough to consider the writes to the (pseudo) function
> arguments dead.
> 
> How about this:
> 
> diff --git a/arch/m68k/kernel/signal.c b/arch/m68k/kernel/signal.c
> index af1c4f3..3cc9eaa 100644
> --- a/arch/m68k/kernel/signal.c
> +++ b/arch/m68k/kernel/signal.c
> @@ -757,6 +757,11 @@ asmlinkage int do_sigreturn(unsigned long __unused)
>  
>  	if (restore_sigcontext(regs, &frame->sc, frame + 1))
>  		goto badframe;
> +	/*
> +	 * Force a barrier so that the compiler does not consider writes
> +	 * to *sw and *regs as dead.
> +	 */
> +	barrier();
>  	return regs->d0;
>  
>  badframe:
> @@ -781,6 +786,11 @@ asmlinkage int do_rt_sigreturn(unsigned long __unused)
>  
>  	if (rt_restore_ucontext(regs, sw, &frame->uc))
>  		goto badframe;
> +	/*
> +	 * Force a barrier so that the compiler does not consider writes
> +	 * to *sw and *regs as dead.
> +	 */
> +	barrier();
>  	return regs->d0;
>  
>  badframe:

Yes that works too. It forces generation of correct code.

So what is the best path forward here?
Should I submit a gcc bug report?

Regards
Greg

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Andreas Schwab]

Greg Ungerer <gerg@uclinux.org> writes:

> Should I submit a gcc bug report?

No, it's not a bug.  Function parameters are dead after return, so any
writes to them can be elided.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Greg Ungerer]

On 09/02/16 19:48, Andreas Schwab wrote:
> Greg Ungerer <gerg@uclinux.org> writes:
>
>> Should I submit a gcc bug report?
>
> No, it's not a bug.  Function parameters are dead after return, so any
> writes to them can be elided.

Given the original code is:

   asmlinkage int do_sigreturn(unsigned long __unused)
   {
         struct switch_stack *sw = (struct switch_stack *) &__unused;
         struct pt_regs *regs = (struct pt_regs *) (sw + 1);

So gcc is determining that what sw and regs points to are
part of do_sigreturn() parameters here?

Given the ugliness of this grunging around in the stack I
still think my original patch to use correct pointer args is
much cleaner all round.

Regards
Greg

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Andreas Schwab]

Greg Ungerer <gerg@uclinux.org> writes:

> Given the original code is:
>
>   asmlinkage int do_sigreturn(unsigned long __unused)
>   {
>         struct switch_stack *sw = (struct switch_stack *) &__unused;
>         struct pt_regs *regs = (struct pt_regs *) (sw + 1);
>
> So gcc is determining that what sw and regs points to are
> part of do_sigreturn() parameters here?

It's basically what varags used to do.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Philippe De Muyter]

On Tue, Feb 09, 2016 at 02:17:24PM +0100, Andreas Schwab wrote:
> Greg Ungerer <gerg@uclinux.org> writes:
> 
> > Given the original code is:
> >
> >   asmlinkage int do_sigreturn(unsigned long __unused)
> >   {
> >         struct switch_stack *sw = (struct switch_stack *) &__unused;
> >         struct pt_regs *regs = (struct pt_regs *) (sw + 1);
> >
> > So gcc is determining that what sw and regs points to are
> > part of do_sigreturn() parameters here?
> 
> It's basically what varags used to do.

Given what gcc does now, how can scanf still work then ?

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Andreas Schwab]

Philippe De Muyter <phdm@macq.eu> writes:

> Given what gcc does now, how can scanf still work then ?

In which way is that related to scanf?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] m68k: use conventional function parameters for do_sigreturn

[Philippe De Muyter]

On Tue, Feb 09, 2016 at 03:36:14PM +0100, Andreas Schwab wrote:
> Philippe De Muyter <phdm@macq.eu> writes:
> 
> > Given what gcc does now, how can scanf still work then ?
> 
> In which way is that related to scanf?

The scanf family uses/used varargs to determine the addresses of the
returned values to fill, like do_sigreturn.

But I see the difference now : with scanf only the addresses are on the
stack, while with do_sigreturn the values themselves are on the stack.

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles