Re: Fwd: sh:fixed issue in xchg_u32 function when val==r15.

Srinivas KANDAGATLA
2011-06-23T12:27:48Z

Hi Carmelo & Salvo,

I agree with your observation..

Initially we had similar understanding, however after discussing with compiler guys and reading http://gcc.gnu.org/bugzilla/show_bug.cgi?id\x11807 posts, which made us decide adding 'val' to constraints rather than clobber list.
As r15 is the stack pointer, adding r15(SP) to clobber list might not work as expected(as we have no idea what compiler is going to do), One guess is that It will have unknown side effects(which is proved as shown below...).

Yesterday, Stuart and I spend some time to test and see the side-effect of adding r15 to clobber list in xchg_u32 after reverting my changes on 7141(ST40) board, We got lucky.. and encountered crash in every boot.

PC and PR ended up at 0x0 as r15 got corrupted because of the below code generated as result of r15 in clobber list.

0x8004b128 <+36>: mova 0x8004b134 <bdi_sync_supers+48>,r0
0x8004b12a <+38>: nop
0x8004b12c <+40>: mov r15,r1
0x8004b12e <+42>: mov #-4,r15
0x8004b130 <+44>: mov.l @r2,r3
0x8004b132 <+46>: mov.l r8,@r2
0x8004b134 <+48>: mov r1,r15
0x8004b136 <+50>: jsr @r10
0x8004b138 <+52>: nop
0x8004b13a <+54>: jsr @r9
0x8004b13c <+56>: nop
0x8004b13e <+58>: jsr @r11
0x8004b140 <+60>: nop
0x8004b142 <+62>: tst r0,r0
=> 0x8004b144 <+64>: bt.s 0x8004b124 <bdi_sync_supers+32>
=> 0x8004b146 <+66>: lds.l @r15+,pr
0x8004b148 <+68>: mov.l @r15+,r11
0x8004b14a <+70>: mov #0,r0
0x8004b14c <+72>: mov.l @r15+,r10
0x8004b14e <+74>: mov.l @r15+,r9
0x8004b150 <+76>: rts

If you look at the highlighted lines of code, bt is delayed branch instruction, which ends up incrementing r15 for every loop, as a result we see PC and PR end's up at 0.

with latest code (with my patches) below code is generated for the same subroutine.
298: 02 c7 mova 2a4 <bdi_sync_supers+0x34>,r0
29a: 09 00 nop
29c: f3 61 mov r15,r1
29e: fc ef mov #-4,r15
2a0: 22 67 mov.l @r2,r7
2a2: 32 22 mov.l r3,@r2
2a4: 13 6f mov r1,r15
2a6: 0b 49 jsr @r9
2a8: 09 00 nop
2aa: 0b 48 jsr @r8
2ac: 09 00 nop
2ae: 0b 4b jsr @r11
2b0: 09 00 nop
2b2: 08 20 tst r0,r0
=> 2b4: ec 89 bt 290 <bdi_sync_supers+0x20>
=> 2b6: 26 4f lds.l @r15+,pr
2b8: f6 6b mov.l @r15+,r11
2ba: 00 e0 mov #0,r0
2bc: f6 6a mov.l @r15+,r10
2be: f6 69 mov.l @r15+,r9
2c0: 0b 00 rts

There might be other such instances of side effects of r15(stack pointer) in clobber list.
I hope we answered why 'val' ended up in input/output constraints rather than r15 in clobber list.

Thanks,
srini

Carmelo AMOROSO wrote:
> On 22/06/2011 9.05, Carmelo Amoroso wrote:
>> ---------- Forwarded message ----------
>> From: Srinivas KANDAGATLA <srinivas.kandagatla@st.com>
>> Date: 19 April 2011 18:28
>> Subject: Re: sh:fixed issue in xchg_u32 function when val=r15.
>> To: Paul Mundt <lethal@linux-sh.org>
>> Cc: linux-sh@vger.kernel.org, Stuart Menefy <stuart.menefy@st.com>
>>
>>
>> Hi Paul,
>> Thanks for the suggestion.
>>
>> Attached is the reworked patch for potential gUSA rollback users.
>>
>> Thanks,
>> srini
>>
>>
>> Paul Mundt wrote:
>>> On Thu, Apr 07, 2011 at 01:57:09PM +0100, Srinivas KANDAGATLA wrote:
>>>
>>>> Recently we have discovered a bug in xchg_u32 function of GUSA_RB feature.
>>>> This function breaks if one of the input parameter 'val' is r15.
>>>>
>>>> 168: 02 c7 mova 174 <exit_mm+0x54>,r0
>>>> 16a: 09 00 nop 16c: f3 61 mov r15,r1
>>>> 16e: fc ef mov #-4,r15
>>>>
>>> The -4 here is part of the gUSA login sequence, so if you're seeing the
>>> problem with gUSA based xchg_u32 it seems like you're going to have to
>>> apply the same fix for all gUSA rollback users.
>>>
>
> Hi Paul, Srini
> instead of making 'val' an input/output contraint (to force gcc not
> using r15 directly), should not be better (or more correct) to add r15
> to the clobbered register list ? r15 is actually modified for the
> login/logout gUSA sequence.
> Indeed, 'val' is not an output parameter... so we are benefiting from a
> side effect.
>
> Further, looking at __cmpxchg_u32, we think that we should make the
> first argument (volatile int *m) an input/output constraint as well (as
> it happens in xchg_u8 and xchg_32), and include r15 instead in the
> clobber list, keeping 'val' just an input parameter.
>
> Comments are welcome.
>
> Cheers,
> Carmelo & Salvo
>