diff mbox

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

Message ID CAE9qxYC+VTc_s6d8P064TD8KOo3DNXMafjUQR7hYxhf=eatYtw@mail.gmail.com
State New
Headers show

Commit Message

Rostislav Pehlivanov Nov. 30, 2017, 11:55 a.m. UTC
On 28 November 2017 at 01:26, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>

Does the attached patch work?

Comments

Michael Niedermayer Nov. 30, 2017, 2:17 p.m. UTC | #1
On Thu, Nov 30, 2017 at 11:55:03AM +0000, Rostislav Pehlivanov wrote:
> On 28 November 2017 at 01:26, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
> > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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@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@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

 [...]
Hendrik Leppkes Nov. 30, 2017, 2:29 p.m. UTC | #2
On Thu, Nov 30, 2017 at 3:17 PM, Michael Niedermayer
<michael@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@niedermayer.cc>
>> wrote:
>>
>> > On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
>> > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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@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@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.

- Hendrik
Hendrik Leppkes Nov. 30, 2017, 2:44 p.m. UTC | #3
On Thu, Nov 30, 2017 at 3:29 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> On Thu, Nov 30, 2017 at 3:17 PM, Michael Niedermayer
> <michael@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@niedermayer.cc>
>>> wrote:
>>>
>>> > On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
>>> > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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@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@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?

- Hendrik
James Almer Nov. 30, 2017, 2:53 p.m. UTC | #4
On 11/30/2017 11:44 AM, Hendrik Leppkes wrote:
> On Thu, Nov 30, 2017 at 3:29 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> On Thu, Nov 30, 2017 at 3:17 PM, Michael Niedermayer
>> <michael@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@niedermayer.cc>
>>>> wrote:
>>>>
>>>>> On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
>>>>>> Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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@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@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?
wm4 Nov. 30, 2017, 4:50 p.m. UTC | #5
On Thu, 30 Nov 2017 15:44:19 +0100
Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Thu, Nov 30, 2017 at 3:29 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> > On Thu, Nov 30, 2017 at 3:17 PM, Michael Niedermayer
> > <michael@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@niedermayer.cc>
> >>> wrote:
> >>>  
> >>> > On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:  
> >>> > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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@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@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@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?

Shouldn't matter, as long as you always access the atomics via the
atomic_ functions? (Real C11 lets you access atomics directly, but you
can't hope to emulate that without language support.)
diff mbox

Patch

From b9ed59e9aff4a0fcc40f444f307c87acad55e139 Mon Sep 17 00:00:00 2001
From: Rostislav Pehlivanov <atomnuker@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@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;
 
 AVCodecParser *av_parser_next(const AVCodecParser *p)
 {
     if (p)
         return p->next;
     else
-        return av_first_parser;
+        return atomic_load(&av_first_parser);
 }
 
 void av_register_codec_parser(AVCodecParser *parser)
 {
+    AVCodecParser *old_parser;
     do {
-        parser->next = av_first_parser;
-    } while (parser->next != avpriv_atomic_ptr_cas((void * volatile *)&av_first_parser, parser->next, parser));
+        parser->next = old_parser = atomic_load(&av_first_parser);
+        atomic_compare_exchange_strong(&av_first_parser, &old_parser, parser);
+    } while (parser->next != old_parser);
 }
 
 AVCodecParserContext *av_parser_init(int codec_id)
@@ -58,7 +60,7 @@  AVCodecParserContext *av_parser_init(int codec_id)
     if (codec_id == AV_CODEC_ID_NONE)
         return NULL;
 
-    for (parser = av_first_parser; parser; parser = parser->next) {
+    for (parser = atomic_load(&av_first_parser); parser; parser = parser->next) {
         if (parser->codec_ids[0] == codec_id ||
             parser->codec_ids[1] == codec_id ||
             parser->codec_ids[2] == codec_id ||
-- 
2.15.1.424.g9478a66081