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