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

[John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>]

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