Re: Problems with atomics in 1.7.2

Hi Yann,

Am 14.02.23 um 14:31 schrieb Yann Ylavic:
> I think we have several issues here:
> 1. we should set WEAK_MEMORY_ORDERING for anything but __i386__,
> __x86_64__, __s390__ and __s390x__
> 2. we should not use direct 64bit load/store on 32bit systems for
> atomic operations
> 3. on 32bit systems the alignment for an uint64_t is usually 4 (not 8
> like on a 64bit systems), which hurts (at best) or prevents atomicity
> even with builtins.
> 
> For 1. and 2. the generic/mutex case was addressed in r1907541
> (apr_atomic_set64() was doing the right thing already), and the
> builtins case is addressed in r1907637 (hopefully).
> Windows code is also concerned but I believe it's doing the right
> thing already by checking _M_X64 (x86_64 CPU) or should we check for
> _WIN64 too (or APR_SIZEOF_VOIDP >= 8) for WIN32 running on x64? What
> about the alignment of uint64_t on WIN32, the Interlocked*64()
> functions seem to require 8-byte alignment so should we fall back to
> generic/mutex on WIN32 too?
> 
> For 3., r1907642 should now take care of the alignment to define (or
> not) HAVE_ATOMIC_BUILTINS64 and HAVE__ATOMIC_BUILTINS64, with a
> fallback to USE_ATOMICS_GENERIC for cases where it matters. This was
> tested by looking at the (32bit) assembly generated for different
> compilers/options/builtins here: https://godbolt.org/z/r4daf6b4v
> (comment in/out some "__attribute__((aligned(8)))" and/or "&& 0" to
> see what helps or not).
> Notably, while both gcc and clang seem to implement the same kind of
> compare-and-swap loop for _read64 and _set64 with -m32 and the legacy
> __sync_* builtins, they seem to disagree on whether __atomic builtins
> should defer to libatomic (thus possibly mutex) when an uint64_t is
> not 8-byte aligned. gcc will use some x87/FPU instructions
> (fild/fistp) regardless of the alignment whereas clang will issue
> calls to libatomic whenever apr_uint64_t is not forcibly 8-byte
> aligned.
> Dunno who's correct here (whether x87 instructions work with 4-byte
> alignment, CPU cacheline crossing and so on..), but this seems to beg
> for a forced "typedef uint64_t apr_uint64_t
> __attribute__((aligned(8)));" (an ABI change) or a new API for 64bit
> atomics (with properly aligned apr_atomic_u64_t) on the APR side
> anyway to do the right thing on 32bit systems..
> For now (after r1907642) this means that compiling a 32bit APR will
> USE_ATOMICS_BUILTINS64 with gcc and NEED_ATOMICS_GENERIC64 with clang.
> 
> Does this all work for you?

Thanks for all the fixes. I agree with all the changes you did. However, 
I don't know enough about the windows compilers to comment on that. I 
would also agree that a 8-byte aligned apr_uint64_t would probably make 
sense for apr 2.0. On x86 the FPU instructions may work without 
alignment, but I expect it to have some performance impact.

I have applied your commits to Debian's 1.7.2 and there was no test 
failure during build. I also executed testatomic on powerpc ~ 100 times 
without problems.


> Finally it seems that both -march=i[56]86 (but not i[34]86) provide
> the same atomic builtins too (with gcc), so maybe we could use them
> instead of the forced generic/mutex implementation (currently) if it's
> how distros build 32bit APR (per
> https://bz.apache.org/bugzilla/show_bug.cgi?id=66457). So I think
> something like this would be fine:

I think the intention of the current code was that libapr compiled on 
any 32bit x86 would run everywhere by default. I don't think this 
behavior should be changed for apr 1.x, but that is not a strong opinion 
because 486 is so old. In Debian, we use --enable-nonportable-atomics 
anyway.

Chers,
Stefan

> 
> Index: configure.in
> ===================================================================
> --- configure.in    (revision 1907642)
> +++ configure.in    (working copy)
> @@ -566,6 +566,9 @@ if test "$ap_cv_atomic_builtins" = "yes" -o "$ap_c
>       if test "$ap_cv__atomic_builtins" = "yes"; then
>           AC_DEFINE(HAVE__ATOMIC_BUILTINS, 1, [Define if compiler
> provides 32bit __atomic builtins])
>       fi
> +    has_atomic_builtins=yes
> +else
> +    has_atomic_builtins=no
>   fi
> 
>   AC_CACHE_CHECK([whether the compiler provides 64bit atomic builtins],
> [ap_cv_atomic_builtins64],
> @@ -829,10 +832,15 @@ AC_ARG_ENABLE(nonportable-atomics,
>      force_generic_atomics=yes
>    fi
>   ],
> -[case $host_cpu in
> -   i[[456]]86) force_generic_atomics=yes ;;
> -   *) force_generic_atomics=no
> -      case $host in
> +[force_generic_atomics=no
> +case $host_cpu in
> +   i[[34]]86) force_generic_atomics=yes;;
> +   i[[56]]86)
> +      if test "$has_atomic_builtins" != "yes"; then
> +        force_generic_atomics=yes
> +      fi
> +      ;;
> +   *) case $host in
>            *solaris2.10*)
>               AC_TRY_COMPILE(
>                   [#include <atomic.h>],
> ?
> 
> 
> Regards;
> Yann.