[FFmpeg-devel] [PATCH 1/4] lavc/parser: use C11 atomics

[James Almer]

On 11/30/2017 11:44 AM, Hendrik Leppkes wrote:
> On Thu, Nov 30, 2017 at 3:29 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
>> On Thu, Nov 30, 2017 at 3:17 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>>> On Thu, Nov 30, 2017 at 11:55:03AM +0000, Rostislav Pehlivanov wrote:
>>>> On 28 November 2017 at 01:26, Michael Niedermayer <michael at niedermayer.cc>
>>>> wrote:
>>>>
>>>>> On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
>>>>>> Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
>>>>>> ---
>>>>>>  libavcodec/parser.c | 14 ++++++++------
>>>>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>>>>>> index 670680ea7c..baf1de4d88 100644
>>>>>> --- a/libavcodec/parser.c
>>>>>> +++ b/libavcodec/parser.c
>>>>>> @@ -23,30 +23,32 @@
>>>>>>  #include <inttypes.h>
>>>>>>  #include <stdint.h>
>>>>>>  #include <string.h>
>>>>>> +#include <stdatomic.h>
>>>>>>
>>>>>>  #include "libavutil/avassert.h"
>>>>>> -#include "libavutil/atomic.h"
>>>>>>  #include "libavutil/internal.h"
>>>>>>  #include "libavutil/mem.h"
>>>>>>
>>>>>>  #include "internal.h"
>>>>>>  #include "parser.h"
>>>>>>
>>>>>> -static AVCodecParser *av_first_parser = NULL;
>>>>>> +static _Atomic(AVCodecParser *)av_first_parser = NULL;
>>>>>
>>>>> This doesnt build here
>>>>>
>>>>> libavcodec/parser.c:35:8: warning: return type defaults to ‘int’ [enabled
>>>>> by default]
>>>>>  static _Atomic(AVCodecParser *)av_first_parser = NULL;
>>>>>         ^
>>>>> libavcodec/parser.c: In function ‘_Atomic’:
>>>>> libavcodec/parser.c:35:32: error: expected declaration specifiers before
>>>>> ‘av_first_parser’
>>>>>  static _Atomic(AVCodecParser *)av_first_parser = NULL;
>>>>>                                 ^
>>>>> libavcodec/parser.c:38:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
>>>>> ‘__attribute__’ before ‘{’ token
>>>>>  {
>>>>>  ^
>>>>> libavcodec/parser.c:46:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
>>>>> ‘__attribute__’ before ‘{’ token
>>>>>  {
>>>>>  ^
>>>>> libavcodec/parser.c:55:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__a
>>>>>
>>>>>
>>>>> [...]
>>>>> --
>>>>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>>>
>>>>> Observe your enemies, for they first find out your faults. -- Antisthenes
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel at ffmpeg.org
>>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>>>
>>>>
>>>> Does the attached patch work?
>>>
>>>>  parser.c |   14 ++++++++------
>>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>> 71f66a37fb46e29626f27da4658f34927e48033c  v2-0001-lavc-parser-use-C11-atomics.patch
>>>> From b9ed59e9aff4a0fcc40f444f307c87acad55e139 Mon Sep 17 00:00:00 2001
>>>> From: Rostislav Pehlivanov <atomnuker at gmail.com>
>>>> Date: Mon, 27 Nov 2017 01:56:41 +0000
>>>> Subject: [PATCH v2 1/2] lavc/parser: use C11 atomics
>>>>
>>>> Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
>>>> ---
>>>>  libavcodec/parser.c | 14 ++++++++------
>>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>>>> index 670680ea7c..d6df62bf70 100644
>>>> --- a/libavcodec/parser.c
>>>> +++ b/libavcodec/parser.c
>>>> @@ -23,30 +23,32 @@
>>>>  #include <inttypes.h>
>>>>  #include <stdint.h>
>>>>  #include <string.h>
>>>> +#include <stdatomic.h>
>>>>
>>>>  #include "libavutil/avassert.h"
>>>> -#include "libavutil/atomic.h"
>>>>  #include "libavutil/internal.h"
>>>>  #include "libavutil/mem.h"
>>>>
>>>>  #include "internal.h"
>>>>  #include "parser.h"
>>>>
>>>> -static AVCodecParser *av_first_parser = NULL;
>>>> +static _Atomic(AVCodecParser *) av_first_parser = NULL;
>>>
>>> no
>>>
>>>
>>> libavcodec/parser.c:35:8: warning: return type defaults to ‘int’ [enabled by default]
>>>  static _Atomic(AVCodecParser *) av_first_parser = NULL;
>>>         ^
>>> libavcodec/parser.c: In function ‘_Atomic’:
>>> libavcodec/parser.c:35:33: error: expected declaration specifiers before ‘av_first_parser’
>>>  static _Atomic(AVCodecParser *) av_first_parser = NULL;
>>>
>>> also see
>>> compat/atomics/gcc/stdatomic.h
>>>
>>> i think what you do is not supportd in that
>>>
>>>
>>
>> _Atomic is a keywork like volatile, so its syntax should be the same.
>> Perhaps those emulations should just define it to "volatile" if its
>> missing - the win32 emulation certainly would expect it to be
>> volatile, and I would expect this to be true for the others as well.
>>
> 
> On that note, it seems a bit odd that the emulations don't typedef the
> atomic types to be volatile. We replace "volatile int" with
> "atomic_int" for example, and with win32 or gcc emulation for
> stdatomic.h, the volatile is basically just being dropped.
> Sounds like that might be missing?
> 

Probably. Guess nobody really noticed it because the emulation wrappers
are rarely used (msvc, gcc < 4.9, suncc).
Maybe someone with old gcc could try a thread sanitizer run or two and
compare the results with those from gcc >= 4.9?