Re: [committed] Update config/pa/linux-atomic.c and enable sync tests on hppa-linux

[Richard Henderson <rth at redhat dot com>]

On 08/10/2010 07:28 PM, John David Anglin wrote:
> The attached patches are essentially identical to that recently applied
> to arm.
> 
> Tested on hppa-unknown-linux-gnu with no observed regressions.  Committed
> to trunk.  Plan to backport to 4.4 and 4.5 after testing.

FWIW, I had a browse through this file and it's a bit off.  E.g.:

> __sync_val_compare_and_swap_4 (int *ptr, int oldval, int newval)
> {
>   int actual_oldval, fail;
>     
>   while (1)
>     {
>       actual_oldval = *ptr;
> 
>       if (oldval != actual_oldval)
>         return actual_oldval;
> 
>       fail = __kernel_cmpxchg (actual_oldval, newval, ptr);
>   
>       if (!fail)
>         return oldval;
>     }
> }

This should not be returning the OLDVAL that the user passed,
it should be returning the lws_ret that the kernel returned.

This is because the user of __sync_val_c_a_s wants avoid the
reload of the value, like so (from libgomp/iter.c):

>       tmp = __sync_val_compare_and_swap (&ws->next, start, nend);
>       if (__builtin_expect (tmp == start, 1))
>         break;
>       start = tmp;

You might be better writing

static int
__kernel_cmpxchg (int oldval, int newval, int *mem)
{
  do {
    asm(...);
  } while (lws_errno == -EAGAIN);

  /* Other possibilities are EFAULT, ENOSYS, EDEADLOCK.  */
  if (__builtin_expect (lws_errno != 0, 0))
    {
      ABORT_INSTRUCTION;
      __builtin_unreachable ();
    }
    
  return lws_ret;
}

and totally hiding the possibility of kernel failure.

This, incidentally, is exactly the implementation of
__sync_val_c_a_s, with arguments in a different order.


r~