[Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Nikunj A Dadhania]

Travis builds failure was reported for powernv boot-serial test with
qemu built with clang.

Debugging revealed that CONFIG_ATOMIC64 wasnt getting set for the clang
build because of that atomic operations weren't being used and was
resulting in MTTCG failure in the powernv boot-serial test.

libatomic is required to successfully test atomic64 and atomic128 for
clang. Introduced newer checks for the same. And on failure default to
single threaded tcg support in PPC64.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---

Reference:
https://lists.gnu.org/archive/html/qemu-ppc/2017-04/msg00277.html

---
 configure | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/configure b/configure
index d31a3e8..1e5f7af 100755
--- a/configure
+++ b/configure
@@ -4598,6 +4598,9 @@ int main(void)
 EOF
   if compile_prog "" "" ; then
     atomic128=yes
+  elif compile_prog "" "-latomic" ; then
+     atomic128=yes
+     lib_atomic="-latomic"
   fi
 fi
 
@@ -4628,6 +4631,9 @@ int main(void)
 EOF
 if compile_prog "" "" ; then
   atomic64=yes
+elif compile_prog "" "-latomic" ; then
+  atomic64=yes
+  lib_atomic="-latomic"
 fi
 
 ########################################
@@ -6065,6 +6071,16 @@ if [ "$TARGET_BASE_ARCH" = "" ]; then
   TARGET_BASE_ARCH=$TARGET_ARCH
 fi
 
+if test $atomic64 == "yes" || test $atomic128 == "yes" ; then
+    libs_softmmu="$lib_atomic $libs_softmmu"
+elif test $mttcg == "yes" && test $TARGET_BASE_ARCH == "ppc"; then
+    echo
+    echo "Note: Atomic library (-latomic) not available, falling"
+    echo "      back to single threaded mode by default"
+    echo
+    mttcg=no
+fi
+
 symlink "$source_path/Makefile.target" "$target_dir/Makefile"
 
 upper() {
-- 
2.9.3

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Peter Maydell]

On 25 April 2017 at 09:35, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> Travis builds failure was reported for powernv boot-serial test with
> qemu built with clang.
>
> Debugging revealed that CONFIG_ATOMIC64 wasnt getting set for the clang
> build because of that atomic operations weren't being used and was
> resulting in MTTCG failure in the powernv boot-serial test.
>
> libatomic is required to successfully test atomic64 and atomic128 for
> clang. Introduced newer checks for the same. And on failure default to
> single threaded tcg support in PPC64.
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

Do we really need libatomic? I thought the intention here was
that atomic operations were only done on types of width of the
pointer or less, which should all be doable by the compiler inline,
and thus don't need the external library. In the past "doesn't
work without libatomic" usually meant "accidentally tried to
do an atomic operation on a type that is too wide".

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Nikunj A Dadhania]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 April 2017 at 09:35, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>> Travis builds failure was reported for powernv boot-serial test with
>> qemu built with clang.
>>
>> Debugging revealed that CONFIG_ATOMIC64 wasnt getting set for the clang
>> build because of that atomic operations weren't being used and was
>> resulting in MTTCG failure in the powernv boot-serial test.
>>
>> libatomic is required to successfully test atomic64 and atomic128 for
>> clang. Introduced newer checks for the same. And on failure default to
>> single threaded tcg support in PPC64.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>
> Do we really need libatomic?

I was trying out the program in the configure script with clang and I do
get errors without libatomic:

    $ clang /tmp/atomic.c 
    /tmp/atomic.c:6:7: warning: implicit declaration of function '__atomic_load_8' is invalid in C99 [-Wimplicit-function-declaration]
      y = __atomic_load_8(&x, 0);
          ^
    /tmp/atomic.c:7:3: warning: implicit declaration of function '__atomic_store_8' is invalid in C99 [-Wimplicit-function-declaration]
      __atomic_store_8(&x, y, 0);
      ^
    /tmp/atomic.c:8:3: warning: implicit declaration of function '__atomic_compare_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
      __atomic_compare_exchange_8(&x, &y, x, 0, 0, 0);
      ^
    /tmp/atomic.c:9:3: warning: implicit declaration of function '__atomic_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
      __atomic_exchange_8(&x, y, 0);
      ^
    /tmp/atomic.c:10:3: warning: implicit declaration of function '__atomic_fetch_add_8' is invalid in C99 [-Wimplicit-function-declaration]
      __atomic_fetch_add_8(&x, y, 0);
      ^
    5 warnings generated.
    /tmp/atomic-1660e0.o: In function `main':
    /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
    /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
    /tmp/atomic.c:(.text+0x69): undefined reference to `__atomic_compare_exchange_8'
    /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
    /tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
    clang-3.9: error: linker command failed with exit code 1 (use -v to see invocation)

With -latomic, the linker succeeds in getting the binary.

> I thought the intention here was that atomic operations were only done
> on types of width of the pointer or less, which should all be doable
> by the compiler inline, and thus don't need the external library.

You are right, even without -latomic in libs_softmmu, tests are passing.
But then __atomic_load_8() isn't used in qemu, if it was using them,
build would fail.

> In the past "doesn't work without libatomic" usually meant
>"accidentally tried to do an atomic operation on a type that is too
>wide".

Regards
Nikunj

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Peter Maydell]

On 25 April 2017 at 09:58, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> I was trying out the program in the configure script with clang and I do
> get errors without libatomic:
>
>     $ clang /tmp/atomic.c
>     /tmp/atomic.c:6:7: warning: implicit declaration of function '__atomic_load_8' is invalid in C99 [-Wimplicit-function-declaration]
>       y = __atomic_load_8(&x, 0);
>           ^
>     /tmp/atomic.c:7:3: warning: implicit declaration of function '__atomic_store_8' is invalid in C99 [-Wimplicit-function-declaration]
>       __atomic_store_8(&x, y, 0);
>       ^
>     /tmp/atomic.c:8:3: warning: implicit declaration of function '__atomic_compare_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
>       __atomic_compare_exchange_8(&x, &y, x, 0, 0, 0);
>       ^
>     /tmp/atomic.c:9:3: warning: implicit declaration of function '__atomic_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
>       __atomic_exchange_8(&x, y, 0);
>       ^
>     /tmp/atomic.c:10:3: warning: implicit declaration of function '__atomic_fetch_add_8' is invalid in C99 [-Wimplicit-function-declaration]
>       __atomic_fetch_add_8(&x, y, 0);
>       ^
>     5 warnings generated.
>     /tmp/atomic-1660e0.o: In function `main':
>     /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
>     /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
>     /tmp/atomic.c:(.text+0x69): undefined reference to `__atomic_compare_exchange_8'
>     /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
>     /tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
>     clang-3.9: error: linker command failed with exit code 1 (use -v to see invocation)
>
> With -latomic, the linker succeeds in getting the binary.

What host is this on ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Nikunj A Dadhania]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 April 2017 at 09:58, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>> I was trying out the program in the configure script with clang and I do
>> get errors without libatomic:
>>
>>     $ clang /tmp/atomic.c
>>     /tmp/atomic.c:6:7: warning: implicit declaration of function '__atomic_load_8' is invalid in C99 [-Wimplicit-function-declaration]
>>       y = __atomic_load_8(&x, 0);
>>           ^
>>     /tmp/atomic.c:7:3: warning: implicit declaration of function '__atomic_store_8' is invalid in C99 [-Wimplicit-function-declaration]
>>       __atomic_store_8(&x, y, 0);
>>       ^
>>     /tmp/atomic.c:8:3: warning: implicit declaration of function '__atomic_compare_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
>>       __atomic_compare_exchange_8(&x, &y, x, 0, 0, 0);
>>       ^
>>     /tmp/atomic.c:9:3: warning: implicit declaration of function '__atomic_exchange_8' is invalid in C99 [-Wimplicit-function-declaration]
>>       __atomic_exchange_8(&x, y, 0);
>>       ^
>>     /tmp/atomic.c:10:3: warning: implicit declaration of function '__atomic_fetch_add_8' is invalid in C99 [-Wimplicit-function-declaration]
>>       __atomic_fetch_add_8(&x, y, 0);
>>       ^
>>     5 warnings generated.
>>     /tmp/atomic-1660e0.o: In function `main':
>>     /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
>>     /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
>>     /tmp/atomic.c:(.text+0x69): undefined reference to `__atomic_compare_exchange_8'
>>     /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
>>     /tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
>>     clang-3.9: error: linker command failed with exit code 1 (use -v to see invocation)
>>
>> With -latomic, the linker succeeds in getting the binary.
>
> What host is this on ?

$ uname -a
Linux abhimanyu 4.9.13-200.fc25.x86_64 #1 SMP Mon Feb 27 16:48:42 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
$

Regards
Nikunj

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Peter Maydell]

On 25 April 2017 at 10:16, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 25 April 2017 at 09:58, Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> wrote:
>>>     /tmp/atomic-1660e0.o: In function `main':
>>>     /tmp/atomic.c:(.text+0x28): undefined reference to `__atomic_load_8'
>>>     /tmp/atomic.c:(.text+0x40): undefined reference to `__atomic_store_8'
>>>     /tmp/atomic.c:(.text+0x69): undefined reference to `__atomic_compare_exchange_8'
>>>     /tmp/atomic.c:(.text+0x7d): undefined reference to `__atomic_exchange_8'
>>>     /tmp/atomic.c:(.text+0x91): undefined reference to `__atomic_fetch_add_8'
>>>     clang-3.9: error: linker command failed with exit code 1 (use -v to see invocation)
>>>
>>> With -latomic, the linker succeeds in getting the binary.
>>
>> What host is this on ?
>
> $ uname -a
> Linux abhimanyu 4.9.13-200.fc25.x86_64 #1 SMP Mon Feb 27 16:48:42 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

Hmm, I can repro that, but there's a problem even if we do link against
libatomic. If you disassemble the resulting binaries, gcc produces
what we want:
 f0 48 01 45 e8          lock add %rax,-0x18(%rbp)
and other inline atomic instructions.
clang on the other hand produces
 e8 33 fe ff ff          callq  400610 <__atomic_fetch_add_8@plt>
which is an expensive call to an out of line subroutine.

We need to figure out how to get clang to just emit the inline
operations.

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC] configure: fix clang failure for libatomic

[Richard Henderson]

On 04/25/2017 10:35 AM, Nikunj A Dadhania wrote:
>     if compile_prog "" "" ; then
>       atomic128=yes
> +  elif compile_prog "" "-latomic" ; then
> +     atomic128=yes
> +     lib_atomic="-latomic"
>     fi

This is a problem, because I think you'll find that gcc now advertises 
CONFIG_ATOMIC128 for *all* hosts.

This is because by definition, libatomic supplies fallback routines for types 
of arbitrary size.  However, the fallback routines use locking, not actual 
atomic operations.  So the configure test is no longer testing what we intended.


r~