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