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@xxxxxxxxx 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@xxxxxxxxx>
> > > ---
> > > 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