[FFmpeg-devel] configure: link to libatomic when it's present

C11 atomics in some configurations (e.g. 64bit operations on ppc64 with
GCC) require linking to libatomic.
---
Testing welcome, especially in configurations where
* libatomic is not present
* libatomic is actually needed
---
 configure | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

On 1/19/2022 8:54 AM, Anton Khirnov wrote:
> C11 atomics in some configurations (e.g. 64bit operations on ppc64 with
> GCC) require linking to libatomic.
> ---
> Testing welcome, especially in configurations where
> * libatomic is not present
> * libatomic is actually needed
> ---
>   configure | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 1413122d87..1ff5dbee5b 100755
> --- a/configure
> +++ b/configure
> @@ -6324,7 +6324,14 @@ check_headers asm/types.h
>   # it seems there are versions of clang in some distros that try to use the
>   # gcc headers, which explodes for stdatomic
>   # so we also check that atomics actually work here
> -check_builtin stdatomic stdatomic.h "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"
> +#
> +# some configurations also require linking to libatomic, so try
> +# both with -latomic and without
> +for LATOMIC in "-latomic" ""; do

Shouldn't you try without it first? On my toolchain libatomic is 
present, but libraries compile without linking to it just fine. That 
changes after this patch, where it starts linking to it explicitly.

> +    check_builtin stdatomic stdatomic.h                                                 \
> +        "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"  \
> +        $LATOMIC && add_extralibs $LATOMIC && break

You should probably add it to the required libraries' extralibs only.
Just replace the add_extralibs part with setting stdatomic_extralibs to 
$LATOMIC, and then add stdatomic to all the libraries' _suggest lists, 
same as we do for libm.

> +done
>   
>   check_lib advapi32 "windows.h"            RegCloseKey          -ladvapi32
>   check_lib bcrypt   "windows.h bcrypt.h"   BCryptGenRandom      -lbcrypt &&

James Almer:
> 
> 
> On 1/19/2022 8:54 AM, Anton Khirnov wrote:
>> C11 atomics in some configurations (e.g. 64bit operations on ppc64 with
>> GCC) require linking to libatomic.
>> ---
>> Testing welcome, especially in configurations where
>> * libatomic is not present
>> * libatomic is actually needed
>> ---
>>   configure | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index 1413122d87..1ff5dbee5b 100755
>> --- a/configure
>> +++ b/configure
>> @@ -6324,7 +6324,14 @@ check_headers asm/types.h
>>   # it seems there are versions of clang in some distros that try to
>> use the
>>   # gcc headers, which explodes for stdatomic
>>   # so we also check that atomics actually work here
>> -check_builtin stdatomic stdatomic.h "atomic_int foo, bar =
>> ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"
>> +#
>> +# some configurations also require linking to libatomic, so try
>> +# both with -latomic and without
>> +for LATOMIC in "-latomic" ""; do
> 
> Shouldn't you try without it first? On my toolchain libatomic is
> present, but libraries compile without linking to it just fine. That
> changes after this patch, where it starts linking to it explicitly.
> 

This would work if this test checked for all the atomic operations that
might be needed; but it doesn't: It checks just for atomic increment and
only for atomic_int. What is if atomic_increment can be done without
recourse to libatomic because of hardware support whereas another atomic
operation needs libatomic?

>> +    check_builtin stdatomic
>> stdatomic.h                                                 \
>> +        "atomic_int foo, bar = ATOMIC_VAR_INIT(-1);
>> atomic_store(&foo, 0); foo += bar"  \
>> +        $LATOMIC && add_extralibs $LATOMIC && break
> 
> You should probably add it to the required libraries' extralibs only.
> Just replace the add_extralibs part with setting stdatomic_extralibs to
> $LATOMIC, and then add stdatomic to all the libraries' _suggest lists,
> same as we do for libm.
> 
>> +done
>>     check_lib advapi32 "windows.h"            RegCloseKey         
>> -ladvapi32
>>   check_lib bcrypt   "windows.h bcrypt.h"   BCryptGenRandom     
>> -lbcrypt &&

Quoting James Almer (2022-01-19 13:34:20)
> 
> 
> On 1/19/2022 8:54 AM, Anton Khirnov wrote:
> > C11 atomics in some configurations (e.g. 64bit operations on ppc64 with
> > GCC) require linking to libatomic.
> > ---
> > Testing welcome, especially in configurations where
> > * libatomic is not present
> > * libatomic is actually needed
> > ---
> >   configure | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/configure b/configure
> > index 1413122d87..1ff5dbee5b 100755
> > --- a/configure
> > +++ b/configure
> > @@ -6324,7 +6324,14 @@ check_headers asm/types.h
> >   # it seems there are versions of clang in some distros that try to use the
> >   # gcc headers, which explodes for stdatomic
> >   # so we also check that atomics actually work here
> > -check_builtin stdatomic stdatomic.h "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"
> > +#
> > +# some configurations also require linking to libatomic, so try
> > +# both with -latomic and without
> > +for LATOMIC in "-latomic" ""; do
> 
> Shouldn't you try without it first? On my toolchain libatomic is 
> present, but libraries compile without linking to it just fine. That 
> changes after this patch, where it starts linking to it explicitly.

No, because it may only be needed for some atomic sizes and operations,
which the test in configure doesn't necessarily catch.
And because we pass as-needed to the linker, the built libraries
shouldn't actually require libatomic unless it's really needed.

> 
> > +    check_builtin stdatomic stdatomic.h                                                 \
> > +        "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"  \
> > +        $LATOMIC && add_extralibs $LATOMIC && break
> 
> You should probably add it to the required libraries' extralibs only.
> Just replace the add_extralibs part with setting stdatomic_extralibs to 
> $LATOMIC, and then add stdatomic to all the libraries' _suggest lists, 
> same as we do for libm.

Then we need to actively track which libraries actually use atomics,
which also depends on which features are enabled. Given that this only
fails on less-common arches, this sounds like a recipe for obscure build
failures. Given that it's only really linked when needed, it seems
better to just add it unconditionally.

On 1/19/2022 9:40 AM, Anton Khirnov wrote:
> Quoting James Almer (2022-01-19 13:34:20)
>>
>>
>> On 1/19/2022 8:54 AM, Anton Khirnov wrote:
>>> C11 atomics in some configurations (e.g. 64bit operations on ppc64 with
>>> GCC) require linking to libatomic.
>>> ---
>>> Testing welcome, especially in configurations where
>>> * libatomic is not present
>>> * libatomic is actually needed
>>> ---
>>>    configure | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/configure b/configure
>>> index 1413122d87..1ff5dbee5b 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -6324,7 +6324,14 @@ check_headers asm/types.h
>>>    # it seems there are versions of clang in some distros that try to use the
>>>    # gcc headers, which explodes for stdatomic
>>>    # so we also check that atomics actually work here
>>> -check_builtin stdatomic stdatomic.h "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"
>>> +#
>>> +# some configurations also require linking to libatomic, so try
>>> +# both with -latomic and without
>>> +for LATOMIC in "-latomic" ""; do
>>
>> Shouldn't you try without it first? On my toolchain libatomic is
>> present, but libraries compile without linking to it just fine. That
>> changes after this patch, where it starts linking to it explicitly.
> 
> No, because it may only be needed for some atomic sizes and operations,
> which the test in configure doesn't necessarily catch.
> And because we pass as-needed to the linker, the built libraries
> shouldn't actually require libatomic unless it's really needed.

Ah, didn't consider --as-needed.

> 
>>
>>> +    check_builtin stdatomic stdatomic.h                                                 \
>>> +        "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"  \
>>> +        $LATOMIC && add_extralibs $LATOMIC && break
>>
>> You should probably add it to the required libraries' extralibs only.
>> Just replace the add_extralibs part with setting stdatomic_extralibs to
>> $LATOMIC, and then add stdatomic to all the libraries' _suggest lists,
>> same as we do for libm.
> 
> Then we need to actively track which libraries actually use atomics,
> which also depends on which features are enabled. Given that this only
> fails on less-common arches, this sounds like a recipe for obscure build
> failures. Given that it's only really linked when needed, it seems
> better to just add it unconditionally.

Then just add it to all of them, like we do for libm.

What i want to avoid is having it in EXTRALIBS. There was a huge 
configure rework long ago that removed everything from that variable 
(leaving it as the place where user defined --extra-libs arguments are 
dumped), and fine tuned ld arguments in a per library/module basis. I'd 
like to not go back to start dumping everything in EXTRALIBS.

Like this, on top of this patch:

> diff --git a/configure b/configure
> index 1ff5dbee5b..43713a7679 100755
> --- a/configure
> +++ b/configure
> @@ -3794,20 +3794,20 @@ cws2fws_extralibs="zlib_extralibs"
> 
>  # libraries, in any order
>  avcodec_deps="avutil"
> -avcodec_suggest="libm"
> +avcodec_suggest="libm stdatomic"
>  avdevice_deps="avformat avcodec avutil"
> -avdevice_suggest="libm"
> +avdevice_suggest="libm stdatomic"
>  avfilter_deps="avutil"
> -avfilter_suggest="libm"
> +avfilter_suggest="libm stdatomic"
>  avformat_deps="avcodec avutil"
> -avformat_suggest="libm network zlib"
> -avutil_suggest="clock_gettime ffnvcodec libm libdrm libmfx opencl user32 vaapi vulkan videotoolbox corefoundation corevideo coremedia bcrypt"
> +avformat_suggest="libm network stdatomic zlib"
> +avutil_suggest="clock_gettime ffnvcodec libm libdrm libmfx opencl stdatomic user32 vaapi vulkan videotoolbox corefoundation corevideo coremedia bcrypt"
>  postproc_deps="avutil gpl"
> -postproc_suggest="libm"
> +postproc_suggest="libm stdatomic"
>  swresample_deps="avutil"
> -swresample_suggest="libm libsoxr"
> +swresample_suggest="libm libsoxr stdatomic"
>  swscale_deps="avutil"
> -swscale_suggest="libm"
> +swscale_suggest="libm stdatomic"
> 
>  avcodec_extralibs="pthreads_extralibs iconv_extralibs dxva2_extralibs"
>  avfilter_extralibs="pthreads_extralibs"
> @@ -6330,7 +6330,7 @@ check_headers asm/types.h
>  for LATOMIC in "-latomic" ""; do
>      check_builtin stdatomic stdatomic.h                                                 \
>          "atomic_int foo, bar = ATOMIC_VAR_INIT(-1); atomic_store(&foo, 0); foo += bar"  \
> -        $LATOMIC && add_extralibs $LATOMIC && break
> +        $LATOMIC && eval stdatomic_extralibs="\$LATOMIC" && break
>  done
> 
>  check_lib advapi32 "windows.h"            RegCloseKey          -ladvapi32