[FFmpeg-devel,0/7] convert most of the remaining files to stdatomic

On 3/22/2017 11:01 PM, James Almer wrote:
> On 3/22/2017 10:01 PM, Michael Niedermayer wrote:
>> On Wed, Mar 22, 2017 at 08:34:05PM -0300, James Almer wrote:
>>> This set deals with most of the remaining cases of libavutil/atomic.h usage.
>>> "make fate THREADS=4" passes here on mingw-w64, but I'm not 100% sure fate
>>> has proper coverage of some of these functions.
>>>
>>> Only ones remaining are Decklink (It's C++98 and I'm not sure how wise would
>>> it be to include a C11 header in there, so i'm leaving that to someone else),
>>> and error_resilience.
>>>
>>> James Almer (7):
>>>   avcodec/mediacodec: convert to stdatomic
>>>   avcodec/utils: convert to stdatomic
>>>   avcodec/parser: convert to stdatomic
>>>   avformat/format: convert to stdatomic
>>>   avfilter/avfilter: convert to stdatomic
>>>   avutil/opencl: convert to stdatomic
>>>   avcodec/videotoolboxenc: remove unused atomic header
>>>
>>>  libavcodec/mediacodec.c           |  5 ++---
>>>  libavcodec/mediacodecdec.c        |  1 -
>>>  libavcodec/mediacodecdec_common.c | 14 ++++++--------
>>>  libavcodec/mediacodecdec_common.h |  5 +++--
>>>  libavcodec/parser.c               | 14 ++++++++++----
>>>  libavcodec/utils.c                | 19 +++++++++++--------
>>>  libavcodec/videotoolboxenc.c      |  1 -
>>>  libavfilter/avfilter.c            |  6 ++++--
>>>  libavformat/format.c              |  9 ++++++---
>>>  libavutil/opencl.c                |  5 +++--
>>>  10 files changed, 45 insertions(+), 34 deletions(-)
>>
>> This breaks building with clang
>>
>> CC      libavfilter/avfilter.o
>> CC      libavformat/format.o
>> CC      libavcodec/parser.o
>> CC      libavcodec/utils.o
>> src/libavformat/format.c:68:51: error: address argument to atomic operation must be a pointer to _Atomic type ('AVInputFormat **' (aka 'struct AVInputFormat **') invalid)
>>     while(p != &format->next && !format->next && !__c11_atomic_compare_exchange_strong(p, &cmp, format, 5, 5))
>>                                                   ^                                    ~
>> src/libavformat/format.c:81:51: error: address argument to atomic operation must be a pointer to _Atomic type ('AVOutputFormat **' (aka 'struct AVOutputFormat **') invalid)
>>     while(p != &format->next && !format->next && !__c11_atomic_compare_exchange_strong(p, &cmp, format, 5, 5))
>>                                                   ^                                    ~
>> 2 errors generated.
>> make: *** [libavformat/format.o] Error 1
> 
> Casting the first argument to (atomic_intptr_t *) on GCC 6 gives me a warning
> about making integer from pointer, but still works nonetheless.
> 
> I'm not sure how to fix this. Got any idea?

Does the attached patch compile with Clang? GCC 6 seems happy with it.
It's getting kind of ugly in any case.

On Thu, 23 Mar 2017 00:28:15 -0300
James Almer <jamrial@gmail.com> wrote:

> From 3bf230d5f611ae5d1511e4629f742fc96b379b7f Mon Sep 17 00:00:00 2001
> From: James Almer <jamrial@gmail.com>
> Date: Wed, 22 Mar 2017 23:43:54 -0300
> Subject: [PATCH] avformat/format: convert to stdatomic
> 
> ---
>  libavformat/format.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/format.c b/libavformat/format.c
> index 38ca2a3465..781f341936 100644
> --- a/libavformat/format.c
> +++ b/libavformat/format.c
> @@ -19,7 +19,8 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> -#include "libavutil/atomic.h"
> +#include <stdatomic.h>
> +
>  #include "libavutil/avstring.h"
>  #include "libavutil/bprint.h"
>  #include "libavutil/opt.h"
> @@ -61,9 +62,10 @@ AVOutputFormat *av_oformat_next(const AVOutputFormat *f)
>  void av_register_input_format(AVInputFormat *format)
>  {
>      AVInputFormat **p = last_iformat;
> +    const AVInputFormat *cmp = NULL;
>  
>      // Note, format could be added after the first 2 checks but that implies that *p is no longer NULL
> -    while(p != &format->next && !format->next && avpriv_atomic_ptr_cas((void * volatile *)p, NULL, format))
> +    while(p != &format->next && !format->next && !atomic_compare_exchange_strong((atomic_intptr_t *)p, (intptr_t *)&cmp, (intptr_t)format))
>          p = &(*p)->next;
>  
>      if (!format->next)
> @@ -73,9 +75,10 @@ void av_register_input_format(AVInputFormat *format)
>  void av_register_output_format(AVOutputFormat *format)
>  {
>      AVOutputFormat **p = last_oformat;
> +    const AVOutputFormat *cmp = NULL;
>  
>      // Note, format could be added after the first 2 checks but that implies that *p is no longer NULL
> -    while(p != &format->next && !format->next && avpriv_atomic_ptr_cas((void * volatile *)p, NULL, format))
> +    while(p != &format->next && !format->next && !atomic_compare_exchange_strong((atomic_intptr_t *)p, (intptr_t *)&cmp, (intptr_t)format))
>          p = &(*p)->next;
>  
>      if (!format->next)

Looks like undefined behavior. Atomic variables must be, well, atomic.
You can't just cast them. Declare them as atomic in the first place.

For some silly reason, we don't have pointer atomics, only
atomic_intptr_t. So it'll be a bit of a pain.

On 3/23/2017 3:08 AM, wm4 wrote:
> On Thu, 23 Mar 2017 00:28:15 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> From 3bf230d5f611ae5d1511e4629f742fc96b379b7f Mon Sep 17 00:00:00 2001
>> From: James Almer <jamrial@gmail.com>
>> Date: Wed, 22 Mar 2017 23:43:54 -0300
>> Subject: [PATCH] avformat/format: convert to stdatomic
>>
>> ---
>>  libavformat/format.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/libavformat/format.c b/libavformat/format.c
>> index 38ca2a3465..781f341936 100644
>> --- a/libavformat/format.c
>> +++ b/libavformat/format.c
>> @@ -19,7 +19,8 @@
>>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>>   */
>>  
>> -#include "libavutil/atomic.h"
>> +#include <stdatomic.h>
>> +
>>  #include "libavutil/avstring.h"
>>  #include "libavutil/bprint.h"
>>  #include "libavutil/opt.h"
>> @@ -61,9 +62,10 @@ AVOutputFormat *av_oformat_next(const AVOutputFormat *f)
>>  void av_register_input_format(AVInputFormat *format)
>>  {
>>      AVInputFormat **p = last_iformat;
>> +    const AVInputFormat *cmp = NULL;
>>  
>>      // Note, format could be added after the first 2 checks but that implies that *p is no longer NULL
>> -    while(p != &format->next && !format->next && avpriv_atomic_ptr_cas((void * volatile *)p, NULL, format))
>> +    while(p != &format->next && !format->next && !atomic_compare_exchange_strong((atomic_intptr_t *)p, (intptr_t *)&cmp, (intptr_t)format))
>>          p = &(*p)->next;
>>  
>>      if (!format->next)
>> @@ -73,9 +75,10 @@ void av_register_input_format(AVInputFormat *format)
>>  void av_register_output_format(AVOutputFormat *format)
>>  {
>>      AVOutputFormat **p = last_oformat;
>> +    const AVOutputFormat *cmp = NULL;
>>  
>>      // Note, format could be added after the first 2 checks but that implies that *p is no longer NULL
>> -    while(p != &format->next && !format->next && avpriv_atomic_ptr_cas((void * volatile *)p, NULL, format))
>> +    while(p != &format->next && !format->next && !atomic_compare_exchange_strong((atomic_intptr_t *)p, (intptr_t *)&cmp, (intptr_t)format))
>>          p = &(*p)->next;
>>  
>>      if (!format->next)
> 
> Looks like undefined behavior. Atomic variables must be, well, atomic.
> You can't just cast them. Declare them as atomic in the first place.

GCC seems lax and accepts non atomic variables as first argument, but
Clang evidently doesn't.

> 
> For some silly reason, we don't have pointer atomics, only
> atomic_intptr_t. So it'll be a bit of a pain.

I already dealt with the different kind of return value for
atomic_compare_exchange_strong vs the one from avpriv_atomic_ptr_cas
and adapted the add_and_fetch calls to work with fetch_add/sub, so
I'll leave solving this to someone else. 
It's getting really annoying and i can only test with one compiler
that doesn't even warn me if i do things wrong.