[PATCH] sh: lib: Restore r4 in shift helpers

[PATCH] sh: lib: Restore r4 in shift helpers

[Florian Fuchs]

Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
that use r4 as a scratch register while dispatching to the selected shift
sequence. But, the helpers return without restoring r4. With GCC 17, the
register allocator can keep a live value in r4 across the helper call.
Clobbering it results in runtime data corruption. Restore r4 before
jumping to the selected shift sequence.

Fixes: 940d4113f330 ("sh: New gcc support")
Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
---
Without the patch, the early boot on e.g J2 gets a kernel BUG at
mm/percpu.c:2604 / "can't handle more than one group."
        PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
As the static condition in mm/percpu-km.c wasn't true:
        ai->nr_groups != 1
nr_groups contained 60 - the clobbered value from the shift helper.

This change was tested on the J2 core on the Mimas v2 board. It can
theoretically also target other SH2 devices, but I don't have any other
than J2 sadly.

The flow of operations matches now the state in the libgcc, see also
in gcc: libgcc/config/sh/lib1funcs.S
---
 arch/sh/lib/ashlsi3.S | 3 ++-
 arch/sh/lib/ashrsi3.S | 3 ++-
 arch/sh/lib/lshrsi3.S | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
index 4df4401cdf31..73a9d709b169 100644
--- a/arch/sh/lib/ashlsi3.S
+++ b/arch/sh/lib/ashlsi3.S
@@ -63,8 +63,9 @@ __ashlsi3_r0:
 	mova	ashlsi3_table,r0
 	mov.b	@(r0,r4),r4
 	add	r4,r0
+	mov.l	@r15+,r4
 	jmp	@r0
-	mov.l	@r15+,r0
+	mov	r4,r0
 
 	.align	2
 ashlsi3_table:
diff --git a/arch/sh/lib/ashrsi3.S b/arch/sh/lib/ashrsi3.S
index bf3c4e03e6ff..9962d7c587df 100644
--- a/arch/sh/lib/ashrsi3.S
+++ b/arch/sh/lib/ashrsi3.S
@@ -62,8 +62,9 @@ __ashrsi3_r0:
 	mova	ashrsi3_table,r0
 	mov.b	@(r0,r4),r4
 	add	r4,r0
+	mov.l	@r15+,r4
 	jmp	@r0
-	mov.l	@r15+,r0
+	mov	r4,r0
 
 	.align	2
 ashrsi3_table:
diff --git a/arch/sh/lib/lshrsi3.S b/arch/sh/lib/lshrsi3.S
index b79b8170061f..9218f0bad1cc 100644
--- a/arch/sh/lib/lshrsi3.S
+++ b/arch/sh/lib/lshrsi3.S
@@ -62,8 +62,9 @@ __lshrsi3_r0:
 	mova	lshrsi3_table,r0
 	mov.b	@(r0,r4),r4
 	add	r4,r0
+	mov.l	@r15+,r4
 	jmp	@r0
-	mov.l	@r15+,r0
+	mov	r4,r0
 
 	.align	2
 lshrsi3_table:
-- 
2.43.0

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz]

Hi Florian,

On Tue, 2026-07-14 at 12:41 +0200, Florian Fuchs wrote:
> Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
> that use r4 as a scratch register while dispatching to the selected shift
> sequence. But, the helpers return without restoring r4. With GCC 17, the
> register allocator can keep a live value in r4 across the helper call.
> Clobbering it results in runtime data corruption. Restore r4 before
> jumping to the selected shift sequence.
> 
> Fixes: 940d4113f330 ("sh: New gcc support")
> Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
> ---
> Without the patch, the early boot on e.g J2 gets a kernel BUG at
> mm/percpu.c:2604 / "can't handle more than one group."
>         PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
> As the static condition in mm/percpu-km.c wasn't true:
>         ai->nr_groups != 1
> nr_groups contained 60 - the clobbered value from the shift helper.
> 
> This change was tested on the J2 core on the Mimas v2 board. It can
> theoretically also target other SH2 devices, but I don't have any other
> than J2 sadly.
> 
> The flow of operations matches now the state in the libgcc, see also
> in gcc: libgcc/config/sh/lib1funcs.S
> ---
>  arch/sh/lib/ashlsi3.S | 3 ++-
>  arch/sh/lib/ashrsi3.S | 3 ++-
>  arch/sh/lib/lshrsi3.S | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
> index 4df4401cdf31..73a9d709b169 100644
> --- a/arch/sh/lib/ashlsi3.S
> +++ b/arch/sh/lib/ashlsi3.S
> @@ -63,8 +63,9 @@ __ashlsi3_r0:
>  	mova	ashlsi3_table,r0
>  	mov.b	@(r0,r4),r4
>  	add	r4,r0
> +	mov.l	@r15+,r4
>  	jmp	@r0
> -	mov.l	@r15+,r0
> +	mov	r4,r0
>  
>  	.align	2
>  ashlsi3_table:
> diff --git a/arch/sh/lib/ashrsi3.S b/arch/sh/lib/ashrsi3.S
> index bf3c4e03e6ff..9962d7c587df 100644
> --- a/arch/sh/lib/ashrsi3.S
> +++ b/arch/sh/lib/ashrsi3.S
> @@ -62,8 +62,9 @@ __ashrsi3_r0:
>  	mova	ashrsi3_table,r0
>  	mov.b	@(r0,r4),r4
>  	add	r4,r0
> +	mov.l	@r15+,r4
>  	jmp	@r0
> -	mov.l	@r15+,r0
> +	mov	r4,r0
>  
>  	.align	2
>  ashrsi3_table:
> diff --git a/arch/sh/lib/lshrsi3.S b/arch/sh/lib/lshrsi3.S
> index b79b8170061f..9218f0bad1cc 100644
> --- a/arch/sh/lib/lshrsi3.S
> +++ b/arch/sh/lib/lshrsi3.S
> @@ -62,8 +62,9 @@ __lshrsi3_r0:
>  	mova	lshrsi3_table,r0
>  	mov.b	@(r0,r4),r4
>  	add	r4,r0
> +	mov.l	@r15+,r4
>  	jmp	@r0
> -	mov.l	@r15+,r0
> +	mov	r4,r0
>  
>  	.align	2
>  lshrsi3_table:

The change looks reasonable to me, but I'm not 100% sure I understand the
code in detail at the moment, so I'll have to take another look tomorrow
when I'm more awake and had a coffee.

Given the fact that this unbreaks J2, we should pick it up for v7.4 once
I have fully understood the changes.

@Geert: Could you have another look at this and let me know what you think?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz]

Hi Florian.

On Tue, 2026-07-14 at 12:41 +0200, Florian Fuchs wrote:
> Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
> that use r4 as a scratch register while dispatching to the selected shift
> sequence. But, the helpers return without restoring r4. With GCC 17, the
> register allocator can keep a live value in r4 across the helper call.
> Clobbering it results in runtime data corruption. Restore r4 before
> jumping to the selected shift sequence.
> 
> Fixes: 940d4113f330 ("sh: New gcc support")
> Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
> ---
> Without the patch, the early boot on e.g J2 gets a kernel BUG at
> mm/percpu.c:2604 / "can't handle more than one group."
>         PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
> As the static condition in mm/percpu-km.c wasn't true:
>         ai->nr_groups != 1
> nr_groups contained 60 - the clobbered value from the shift helper.
> 
> This change was tested on the J2 core on the Mimas v2 board. It can
> theoretically also target other SH2 devices, but I don't have any other
> than J2 sadly.
> 
> The flow of operations matches now the state in the libgcc, see also
> in gcc: libgcc/config/sh/lib1funcs.S
> ---
>  arch/sh/lib/ashlsi3.S | 3 ++-
>  arch/sh/lib/ashrsi3.S | 3 ++-
>  arch/sh/lib/lshrsi3.S | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
> index 4df4401cdf31..73a9d709b169 100644
> --- a/arch/sh/lib/ashlsi3.S
> +++ b/arch/sh/lib/ashlsi3.S
> @@ -63,8 +63,9 @@ __ashlsi3_r0:
>  	mova	ashlsi3_table,r0
>  	mov.b	@(r0,r4),r4
>  	add	r4,r0
> +	mov.l	@r15+,r4
>  	jmp	@r0
> -	mov.l	@r15+,r0
> +	mov	r4,r0
> 

Please correct me if I'm wrong, but after reading the code in [1], it looks
to me as your patch doesn't preserve r4 but it's actually storing it into
r0 to be used by the selected shift sequence.

With the previous code, r4 is pushed onto the stack first but not restored
before the shift sequence is jumped to, so what your patch does not make sure
that r4 is restored across the complete call of the shift helper but rather
restore the input value in r4 from the stack before calling the jump sequence.

What am I missing?

Adrian

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sh/lib/ashlsi3.S

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[yoshinori.sato]

On Thu, 17 Sep 2026 16:17:00 +0900,
John Paul Adrian Glaubitz wrote:
> 
> Hi Florian.
> 
> On Tue, 2026-07-14 at 12:41 +0200, Florian Fuchs wrote:
> > Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
> > that use r4 as a scratch register while dispatching to the selected shift
> > sequence. But, the helpers return without restoring r4. With GCC 17, the
> > register allocator can keep a live value in r4 across the helper call.
> > Clobbering it results in runtime data corruption. Restore r4 before
> > jumping to the selected shift sequence.
> > 
> > Fixes: 940d4113f330 ("sh: New gcc support")
> > Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
> > ---
> > Without the patch, the early boot on e.g J2 gets a kernel BUG at
> > mm/percpu.c:2604 / "can't handle more than one group."
> >         PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
> > As the static condition in mm/percpu-km.c wasn't true:
> >         ai->nr_groups != 1
> > nr_groups contained 60 - the clobbered value from the shift helper.
> > 
> > This change was tested on the J2 core on the Mimas v2 board. It can
> > theoretically also target other SH2 devices, but I don't have any other
> > than J2 sadly.
> > 
> > The flow of operations matches now the state in the libgcc, see also
> > in gcc: libgcc/config/sh/lib1funcs.S
> > ---
> >  arch/sh/lib/ashlsi3.S | 3 ++-
> >  arch/sh/lib/ashrsi3.S | 3 ++-
> >  arch/sh/lib/lshrsi3.S | 3 ++-
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
> > index 4df4401cdf31..73a9d709b169 100644
> > --- a/arch/sh/lib/ashlsi3.S
> > +++ b/arch/sh/lib/ashlsi3.S
> > @@ -63,8 +63,9 @@ __ashlsi3_r0:
> >  	mova	ashlsi3_table,r0
> >  	mov.b	@(r0,r4),r4
> >  	add	r4,r0
> > +	mov.l	@r15+,r4
> >  	jmp	@r0
> > -	mov.l	@r15+,r0
> > +	mov	r4,r0
> > 
> 
> Please correct me if I'm wrong, but after reading the code in [1], it looks
> to me as your patch doesn't preserve r4 but it's actually storing it into
> r0 to be used by the selected shift sequence.
> 
> With the previous code, r4 is pushed onto the stack first but not restored
> before the shift sequence is jumped to, so what your patch does not make sure
> that r4 is restored across the complete call of the shift helper but rather
> restore the input value in r4 from the stack before calling the jump sequence.
> 
> What am I missing?
> 
> Adrian
> 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sh/lib/ashlsi3.S
> 
> -- 
>  .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer
> `. `'   Physicist
>   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Here is the modified code snippet.

	and	#31,r0
	mov.l	r4,@-r15
	mov	r0,r4
	mova	ashlsi3_table,r0
	mov.b	@(r0,r4),r4
	add	r4,r0
	mov.l	@r15+,r4
	jmp	@r0
	mov	r4,r0

I understand that r4 no longer changes as a result of this modification.
While the sh function-calling convention permits the `r4` register to be
clobbered, the `libgcc` included with GCC does not actually clobber `r4`,
and GCC's own rules appear to rely on `r4` remaining intact during such calls.

This change won't break builds with older versions of GCC, so I think it's
fine to go ahead and apply it.

-- 
Yosinori Sato

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz]

Hi Yoshinori,

On Tue, 2026-09-22 at 22:19 +0900, yoshinori.sato@nifty.com wrote:
> On Thu, 17 Sep 2026 16:17:00 +0900,
> John Paul Adrian Glaubitz wrote:
> > 
> > Hi Florian.
> > 
> > On Tue, 2026-07-14 at 12:41 +0200, Florian Fuchs wrote:
> > > Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
> > > that use r4 as a scratch register while dispatching to the selected shift
> > > sequence. But, the helpers return without restoring r4. With GCC 17, the
> > > register allocator can keep a live value in r4 across the helper call.
> > > Clobbering it results in runtime data corruption. Restore r4 before
> > > jumping to the selected shift sequence.
> > > 
> > > Fixes: 940d4113f330 ("sh: New gcc support")
> > > Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
> > > ---
> > > Without the patch, the early boot on e.g J2 gets a kernel BUG at
> > > mm/percpu.c:2604 / "can't handle more than one group."
> > >         PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
> > > As the static condition in mm/percpu-km.c wasn't true:
> > >         ai->nr_groups != 1
> > > nr_groups contained 60 - the clobbered value from the shift helper.
> > > 
> > > This change was tested on the J2 core on the Mimas v2 board. It can
> > > theoretically also target other SH2 devices, but I don't have any other
> > > than J2 sadly.
> > > 
> > > The flow of operations matches now the state in the libgcc, see also
> > > in gcc: libgcc/config/sh/lib1funcs.S
> > > ---
> > >  arch/sh/lib/ashlsi3.S | 3 ++-
> > >  arch/sh/lib/ashrsi3.S | 3 ++-
> > >  arch/sh/lib/lshrsi3.S | 3 ++-
> > >  3 files changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
> > > index 4df4401cdf31..73a9d709b169 100644
> > > --- a/arch/sh/lib/ashlsi3.S
> > > +++ b/arch/sh/lib/ashlsi3.S
> > > @@ -63,8 +63,9 @@ __ashlsi3_r0:
> > >  	mova	ashlsi3_table,r0
> > >  	mov.b	@(r0,r4),r4
> > >  	add	r4,r0
> > > +	mov.l	@r15+,r4
> > >  	jmp	@r0
> > > -	mov.l	@r15+,r0
> > > +	mov	r4,r0
> > > 
> > 
> > Please correct me if I'm wrong, but after reading the code in [1], it looks
> > to me as your patch doesn't preserve r4 but it's actually storing it into
> > r0 to be used by the selected shift sequence.
> > 
> > With the previous code, r4 is pushed onto the stack first but not restored
> > before the shift sequence is jumped to, so what your patch does not make sure
> > that r4 is restored across the complete call of the shift helper but rather
> > restore the input value in r4 from the stack before calling the jump sequence.
> > 
> > What am I missing?
> > 
> > Adrian
> > 
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sh/lib/ashlsi3.S
> > 
> > -- 
> >  .''`.  John Paul Adrian Glaubitz
> > : :' :  Debian Developer
> > `. `'   Physicist
> >   `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913
> 
> Here is the modified code snippet.
> 
> 	and	#31,r0
> 	mov.l	r4,@-r15
> 	mov	r0,r4
> 	mova	ashlsi3_table,r0
> 	mov.b	@(r0,r4),r4
> 	add	r4,r0
> 	mov.l	@r15+,r4
> 	jmp	@r0
> 	mov	r4,r0
> 
> I understand that r4 no longer changes as a result of this modification.
> While the sh function-calling convention permits the `r4` register to be
> clobbered, the `libgcc` included with GCC does not actually clobber `r4`,
> and GCC's own rules appear to rely on `r4` remaining intact during such calls.
> 
> This change won't break builds with older versions of GCC, so I think it's
> fine to go ahead and apply it.

Can you please add your Reviewed-by?

Also, could you please update your mail address in MAINTAINERS?

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz]

Hi Florian,

On Thu, 2026-09-17 at 09:17 +0200, John Paul Adrian Glaubitz wrote:
> Hi Florian.
> 
> On Tue, 2026-07-14 at 12:41 +0200, Florian Fuchs wrote:
> > Commit 940d4113f330 ("sh: New gcc support") added new shift helpers
> > that use r4 as a scratch register while dispatching to the selected shift
> > sequence. But, the helpers return without restoring r4. With GCC 17, the
> > register allocator can keep a live value in r4 across the helper call.
> > Clobbering it results in runtime data corruption. Restore r4 before
> > jumping to the selected shift sequence.
> > 
> > Fixes: 940d4113f330 ("sh: New gcc support")
> > Signed-off-by: Florian Fuchs <fuchsfl@gmail.com>
> > ---
> > Without the patch, the early boot on e.g J2 gets a kernel BUG at
> > mm/percpu.c:2604 / "can't handle more than one group."
> >         PCPU_SETUP_BUG_ON(pcpu_verify_alloc_info(ai) < 0);
> > As the static condition in mm/percpu-km.c wasn't true:
> >         ai->nr_groups != 1
> > nr_groups contained 60 - the clobbered value from the shift helper.
> > 
> > This change was tested on the J2 core on the Mimas v2 board. It can
> > theoretically also target other SH2 devices, but I don't have any other
> > than J2 sadly.
> > 
> > The flow of operations matches now the state in the libgcc, see also
> > in gcc: libgcc/config/sh/lib1funcs.S
> > ---
> >  arch/sh/lib/ashlsi3.S | 3 ++-
> >  arch/sh/lib/ashrsi3.S | 3 ++-
> >  arch/sh/lib/lshrsi3.S | 3 ++-
> >  3 files changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/sh/lib/ashlsi3.S b/arch/sh/lib/ashlsi3.S
> > index 4df4401cdf31..73a9d709b169 100644
> > --- a/arch/sh/lib/ashlsi3.S
> > +++ b/arch/sh/lib/ashlsi3.S
> > @@ -63,8 +63,9 @@ __ashlsi3_r0:
> >  	mova	ashlsi3_table,r0
> >  	mov.b	@(r0,r4),r4
> >  	add	r4,r0
> > +	mov.l	@r15+,r4
> >  	jmp	@r0
> > -	mov.l	@r15+,r0
> > +	mov	r4,r0
> > 
> 
> Please correct me if I'm wrong, but after reading the code in [1], it looks
> to me as your patch doesn't preserve r4 but it's actually storing it into
> r0 to be used by the selected shift sequence.
> 
> With the previous code, r4 is pushed onto the stack first but not restored
> before the shift sequence is jumped to, so what your patch does not make sure
> that r4 is restored across the complete call of the shift helper but rather
> restore the input value in r4 from the stack before calling the jump sequence.
> 
> What am I missing?
> 
> Adrian
> 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/sh/lib/ashlsi3.S

Could you comment on this and maybe rephrase your commit message if you agree
with my analysis? The patch itself is fine, but I think the description is
somewhat inaccurate.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[Florian Fuchs]

On 06 Oct 07:41, John Paul Adrian Glaubitz wrote:
> > Please correct me if I'm wrong, but after reading the code in [1], it looks
> > to me as your patch doesn't preserve r4 but it's actually storing it into
> > r0 to be used by the selected shift sequence.

Yes, the value is copied to r0 for the shift sequence to use, but only after
the r4 original value has been popped back into r4.

The shift sequence themselfs only operate on r0 and then rts, so nothing
touches r4 after this point.

So the caller gets the original r4 back, which is what GCC expects.

> > With the previous code, r4 is pushed onto the stack first but not restored
> > before the shift sequence is jumped to, so what your patch does not make sure
> > that r4 is restored across the complete call of the shift helper but rather
> > restore the input value in r4 from the stack before calling the jump sequence.

Yes, it needs the selected shift sequence to not touch r4 as well.

> Could you comment on this and maybe rephrase your commit message if you agree
> with my analysis? The patch itself is fine, but I think the description is
> somewhat inaccurate.

I think your analysis is correct. The selected shift sequences don't touch 
other register beside r0, so I think it is currently not necessary to make
sure that r4 survives the complete call, even if the selected sequence would
touch r4.

Is it more accurate or which aspect would need a better message, or do
you dislike the "across the whole call" aspect? While the patch doesn't
really make sure r4 is always preserved, it reflects the behaviour after
the patch.

	Commit 940d4113f330 ("sh: New gcc support") reuses r4 as scratch for
	the jump table offset and pops the saved value into r0 in the jmp
	delay slot, so the helpers return with r4 still holding the table
	byte. GCC can keep a live value in r4 across the call, which leads to
	runtime data corruption.

	GCC calls __ashlsi3, __ashrsi3 and __lshrsi3 with a special convention:
	value in r4, result in r0, and only r0 and T clobbered.

	Pop the saved value back into r4 before the jmp and copy it to r0 in
	the delay slot. The shift sequences only operate on r0, so r4 is now
	preserved across the whole call.

Regards
Florian

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz]

Hi Florian,

On Tue, 2026-10-06 at 20:33 +0200, Florian Fuchs wrote:
> On 06 Oct 07:41, John Paul Adrian Glaubitz wrote:
> > > Please correct me if I'm wrong, but after reading the code in [1], it looks
> > > to me as your patch doesn't preserve r4 but it's actually storing it into
> > > r0 to be used by the selected shift sequence.
> 
> Yes, the value is copied to r0 for the shift sequence to use, but only after
> the r4 original value has been popped back into r4.
> 
> The shift sequence themselfs only operate on r0 and then rts, so nothing
> touches r4 after this point.
> 
> So the caller gets the original r4 back, which is what GCC expects.
> 
> > > With the previous code, r4 is pushed onto the stack first but not restored
> > > before the shift sequence is jumped to, so what your patch does not make sure
> > > that r4 is restored across the complete call of the shift helper but rather
> > > restore the input value in r4 from the stack before calling the jump sequence.
> 
> Yes, it needs the selected shift sequence to not touch r4 as well.
> 
> > Could you comment on this and maybe rephrase your commit message if you agree
> > with my analysis? The patch itself is fine, but I think the description is
> > somewhat inaccurate.
> 
> I think your analysis is correct. The selected shift sequences don't touch 
> other register beside r0, so I think it is currently not necessary to make
> sure that r4 survives the complete call, even if the selected sequence would
> touch r4.
> 
> Is it more accurate or which aspect would need a better message, or do
> you dislike the "across the whole call" aspect? While the patch doesn't
> really make sure r4 is always preserved, it reflects the behaviour after
> the patch.
> 
> 	Commit 940d4113f330 ("sh: New gcc support") reuses r4 as scratch for
> 	the jump table offset and pops the saved value into r0 in the jmp
> 	delay slot, so the helpers return with r4 still holding the table
> 	byte. GCC can keep a live value in r4 across the call, which leads to
> 	runtime data corruption.
> 
> 	GCC calls __ashlsi3, __ashrsi3 and __lshrsi3 with a special convention:
> 	value in r4, result in r0, and only r0 and T clobbered.
> 
> 	Pop the saved value back into r4 before the jmp and copy it to r0 in
> 	the delay slot. The shift sequences only operate on r0, so r4 is now
> 	preserved across the whole call.

Yes, this is much better. Can you send a v2?

Thanks,
Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[David Laight]

On Tue, 06 Oct 2026 20:54:30 +0200
John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:

> Hi Florian,
> 
> On Tue, 2026-10-06 at 20:33 +0200, Florian Fuchs wrote:
> > On 06 Oct 07:41, John Paul Adrian Glaubitz wrote:  
> > > > Please correct me if I'm wrong, but after reading the code in [1], it looks
> > > > to me as your patch doesn't preserve r4 but it's actually storing it into
> > > > r0 to be used by the selected shift sequence.  
> > 
> > Yes, the value is copied to r0 for the shift sequence to use, but only after
> > the r4 original value has been popped back into r4.
> > 
> > The shift sequence themselfs only operate on r0 and then rts, so nothing
> > touches r4 after this point.
> > 
> > So the caller gets the original r4 back, which is what GCC expects.
> >   
> > > > With the previous code, r4 is pushed onto the stack first but not restored
> > > > before the shift sequence is jumped to, so what your patch does not make sure
> > > > that r4 is restored across the complete call of the shift helper but rather
> > > > restore the input value in r4 from the stack before calling the jump sequence.  
> > 
> > Yes, it needs the selected shift sequence to not touch r4 as well.
> >   
> > > Could you comment on this and maybe rephrase your commit message if you agree
> > > with my analysis? The patch itself is fine, but I think the description is
> > > somewhat inaccurate.  
> > 
> > I think your analysis is correct. The selected shift sequences don't touch 
> > other register beside r0, so I think it is currently not necessary to make
> > sure that r4 survives the complete call, even if the selected sequence would
> > touch r4.
> > 
> > Is it more accurate or which aspect would need a better message, or do
> > you dislike the "across the whole call" aspect? While the patch doesn't
> > really make sure r4 is always preserved, it reflects the behaviour after
> > the patch.
> > 
> > 	Commit 940d4113f330 ("sh: New gcc support") reuses r4 as scratch for
> > 	the jump table offset and pops the saved value into r0 in the jmp
> > 	delay slot, so the helpers return with r4 still holding the table
> > 	byte. GCC can keep a live value in r4 across the call, which leads to
> > 	runtime data corruption.

I'm sure you can write that more concisely.

> > 	GCC calls __ashlsi3, __ashrsi3 and __lshrsi3 with a special convention:
> > 	value in r4, result in r0, and only r0 and T clobbered.

That misses out the other value passed in r5.
What is T?

> > 
> > 	Pop the saved value back into r4 before the jmp and copy it to r0 in
> > 	the delay slot. The shift sequences only operate on r0, so r4 is now
> > 	preserved across the whole call.  

That seems unnecessary detail for a commit message.
Why not just:
	GCC uses a special calling convention for __ashlsi3, __ashrsi3 and
	__lshrsi3 that requires all registers except r0 (which contains the
	result) be preserved.
	Commit 940d4113f330 ("sh: New gcc support") reused r4 as a scratch
	register leading to data corruption.
	Change the code so that r4 is preserved.

If you are reading sh assembler you should know it has delay slots
after branches.
(I've not looked at it before, but it is the only way the code could
be valid.)

David

> 
> Yes, this is much better. Can you send a v2?
> 
> Thanks,
> Adrian
> 

Re: [PATCH] sh: lib: Restore r4 in shift helpers

[John Paul Adrian Glaubitz]

On Tue, 2026-10-06 at 22:44 +0100, David Laight wrote:
> On Tue, 06 Oct 2026 20:54:30 +0200
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
> 
> > Hi Florian,
> > 
> > On Tue, 2026-10-06 at 20:33 +0200, Florian Fuchs wrote:
> > > On 06 Oct 07:41, John Paul Adrian Glaubitz wrote:  
> > > > > Please correct me if I'm wrong, but after reading the code in [1], it looks
> > > > > to me as your patch doesn't preserve r4 but it's actually storing it into
> > > > > r0 to be used by the selected shift sequence.  
> > > 
> > > Yes, the value is copied to r0 for the shift sequence to use, but only after
> > > the r4 original value has been popped back into r4.
> > > 
> > > The shift sequence themselfs only operate on r0 and then rts, so nothing
> > > touches r4 after this point.
> > > 
> > > So the caller gets the original r4 back, which is what GCC expects.
> > >   
> > > > > With the previous code, r4 is pushed onto the stack first but not restored
> > > > > before the shift sequence is jumped to, so what your patch does not make sure
> > > > > that r4 is restored across the complete call of the shift helper but rather
> > > > > restore the input value in r4 from the stack before calling the jump sequence.  
> > > 
> > > Yes, it needs the selected shift sequence to not touch r4 as well.
> > >   
> > > > Could you comment on this and maybe rephrase your commit message if you agree
> > > > with my analysis? The patch itself is fine, but I think the description is
> > > > somewhat inaccurate.  
> > > 
> > > I think your analysis is correct. The selected shift sequences don't touch 
> > > other register beside r0, so I think it is currently not necessary to make
> > > sure that r4 survives the complete call, even if the selected sequence would
> > > touch r4.
> > > 
> > > Is it more accurate or which aspect would need a better message, or do
> > > you dislike the "across the whole call" aspect? While the patch doesn't
> > > really make sure r4 is always preserved, it reflects the behaviour after
> > > the patch.
> > > 
> > > 	Commit 940d4113f330 ("sh: New gcc support") reuses r4 as scratch for
> > > 	the jump table offset and pops the saved value into r0 in the jmp
> > > 	delay slot, so the helpers return with r4 still holding the table
> > > 	byte. GCC can keep a live value in r4 across the call, which leads to
> > > 	runtime data corruption.
> 
> I'm sure you can write that more concisely.

In what sense?

> > > 	GCC calls __ashlsi3, __ashrsi3 and __lshrsi3 with a special convention:
> > > 	value in r4, result in r0, and only r0 and T clobbered.
> 
> That misses out the other value passed in r5.

Well, technically yes. But I assume Florian omitted it because the change mainly
concerns r4. But yes, r5 could be mentioned as the number of shifts parameter.

> What is T?

T is the test bit of the status register.

> > > 
> > > 	Pop the saved value back into r4 before the jmp and copy it to r0 in
> > > 	the delay slot. The shift sequences only operate on r0, so r4 is now
> > > 	preserved across the whole call.  
> 
> That seems unnecessary detail for a commit message.
> Why not just:
> 	GCC uses a special calling convention for __ashlsi3, __ashrsi3 and
> 	__lshrsi3 that requires all registers except r0 (which contains the
> 	result) be preserved.
> 	Commit 940d4113f330 ("sh: New gcc support") reused r4 as a scratch
> 	register leading to data corruption.
> 	Change the code so that r4 is preserved.

Well, I wanted the description to be more explicit as it helps understand
what's going on easier.

> If you are reading sh assembler you should know it has delay slots
> after branches.
> (I've not looked at it before, but it is the only way the code could
> be valid.)

I don't see how mentioning that is adding too much information.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913