Re: [PATCH] sh: Fix calculation address in atomic function

[Nobuhiro Iwamatsu <iwamatsu at nigauri dot org>]

Hi,

Thank you for your review.

2012/6/11 Kaz Kojima <kkojima@rr.iij4u.or.jp>:
> Nobuhiro Iwamatsu <iwamatsu@nigauri.org> wrote:
>> For example, in atomic_exchange_and_add, %1(__tmp) is added
>> to r2 by "add %1, r2\n\".
>> If interruption enters at this time, it will return to label 0.
>> And %1(__tmp) is added to r2 by "add %1, r2\n\", agian.
>
> Ugh, I thought that these were fixed already. ?My bad.

This problem was not noticed for long time....

>
>> ? ? ? * sysdeps/unix/sysv/linux/sh/bits/atomic.h (atomic_exchange_and_add):
>> ? ? ? Fix calculate address by gUSA.
>
> "Fix gUSA sequence." would be an appropriate ChangeLog entry,
> since the issue is in the value when rewound, not in the address.

OK,  I wil fix this.

>
> The hunks for atomic_exchange_and_add change the semantics.
>
>> @@ -138,8 +138,8 @@ typedef uintmax_t uatomic_max_t;
>> ? ? ? ? mov r15,r1\n\
>> ? ? ? ? mov #-6,r15\n\
>> ? ? ? ? 0: mov.b @%2,%0\n\
>> - ? ? ? add %0,%1\n\
>> - ? ? ? mov.b %1,@%2\n\
>> + ? ? ? add %1,%0\n\
>> + ? ? ? mov.b %0,@%2\n\
>> ? ? ? ? 1: mov r1,r15"\
>> ? ? ? : "=&r" (__result), "=&r" (__tmp) : rNOSP (mem), "1" (__value) \
>> ? ? ? : "r0", "r1", "memory"); \
>
> atomic_exchange_and_add should return the old value of memory
> and it should be fixed like as
>
> ? ? ? __asm __volatile ("\
> ? ? ? ? ?mova 1f,r0\n\
> ? ? ? ? ?.align 2\n\
> ? ? ? ? ?mov r15,r1\n\
> ? ? ? ? ?mov #(0f-1f),r15\n\
> ? ? ? 0: mov.b @%2,%0\n\
> ? ? ? ? ?mov %1,r2\n\
> ? ? ? ? ?add %0,r2\n\
> ? ? ? ? ?mov.b r2,@%2\n\
> ? ? ? 1: mov r1,r15"\
> ? ? ? ?: "=&r" (__result), "=&r" (__tmp) : rNOSP (mem), "1" (__value) \
> ? ? ? ?: "r0", "r1", "r2", "memory"); ? ? ? ? ? ? ? ? \
>

I see. I had made mistake in the understanding.
I will fix this and resend.

> The changes for the other functions look OK.
>
>> ? ? ? * sysdeps/unix/sysv/linux/sh/bits/atomic.h (atomic_bit_set): Likewise.
>> ? ? ? * sysdeps/unix/sysv/linux/sh/bits/atomic.h (atomic_bit_test_set): Likewise.
>
> The patch doesn't include changes for atomic_bit_set and
> atomic_bit_test_set, though it seems that atomic_bit_set doesn't
> need any changes.

Oh, sorry. This is mistake for paste..
I will fix this too.

Best regards,
  Nobuhiro

-- 
Nobuhiro Iwamatsu
?? iwamatsu at {nigauri.org / debian.org}
?? GPG ID: 40AD1FA6