Suppress duplicate write race for the same value.

Suppress duplicate write race for the same value.

All calls into the fft end up rerunning the same piece of cosine
table initialization; i.e. they all write the same values to the
same place before any fft functions can be used.

Modifying upstream and then adding a lazy instance / global lock
to downstream seems overkill to resolve this, so just suppress the
benign race.

BUG=508425
TEST=none

[DaleCurtis, 2015-10-22 18:29:43]

dalecurtis@chromium.org changed reviewers:
+ eugenis@chromium.org

[DaleCurtis, 2015-10-22 18:29:44]

WDYT eugenis? I know we normally frown on suppressions, but the overhead of
working around this harmless race seems high.

[Evgeniy Stepanov, 2015-10-22 18:41:21]

Description was changed from

==========
Suppress duplicate write race for the same value.

All calls into the fft end up rerunning the same piece of cosine
table initialization; i.e. they all write the same values to the
same place before any fft functions can be used.

Modifying upstream and then adding a lazy instance / global lock
to downstream seems overkill to resolve this, so just suppress the
benign race.

BUG=508425
TEST=none
==========

to

==========
Suppress duplicate write race for the same value.

All calls into the fft end up rerunning the same piece of cosine
table initialization; i.e. they all write the same values to the
same place before any fft functions can be used.

Modifying upstream and then adding a lazy instance / global lock
to downstream seems overkill to resolve this, so just suppress the
benign race.

BUG=508425
TEST=none
==========

[Evgeniy Stepanov, 2015-10-22 18:41:21]

eugenis@google.com changed reviewers:
+ dvyukov@chromium.org
- eugenis@chromium.org

[Evgeniy Stepanov, 2015-10-22 18:42:47]

On 2015/10/22 18:29:44, DaleCurtis wrote:
> WDYT eugenis? I know we normally frown on suppressions, but the overhead of
> working around this harmless race seems high.

I'll leave this for Dmitry to decide. I like the upstream idea of wrapping this
in ff_thread_once (I assume it's like pthread_once).

[DaleCurtis, 2015-10-22 18:46:09]

That idea is only half baked, it doesn't address how partial initialization
would be handled, likely there'd need to be an array of pthread_once() calls for
each fft size (4..16).

Additionally, the code for ff_thread_once is only pthread_once on POSIX, on
other platforms (Windows in our case) it appears to try and rely on thread safe
statics...

http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavutil/thread.h;h=3d1573775...

[Dmitry Vyukov, 2015-10-23 04:07:19]

On 2015/10/22 18:46:09, DaleCurtis wrote:
> That idea is only half baked, it doesn't address how partial initialization
> would be handled, likely there'd need to be an array of pthread_once() calls
for
> each fft size (4..16).
> 
> Additionally, the code for ff_thread_once is only pthread_once on POSIX, on
> other platforms (Windows in our case) it appears to try and rely on thread
safe
> statics...
> 
>
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavutil/thread.h;h=3d1573775...


Hi Dale,

You seem to assume that compiler generates machine code that stores to the table
only once. This is not necessary true. The expression in question is quite
complex:

tab[i] = cos(i*freq)*s > max ? max : cos(i*freq)*s < min ? min : cos(i*freq)*s;

The simplest reasonable transformation of this code that will break your
assumption is:

...
tmp = cos(i*freq)*s;
tab[i] = tmp;
if (UNLIKELY(tmp > max))
    tab[i] = max;
...

This already can lead to _persistent_ corruption of the table. Consider that one
thread temporary stores the incorrect value while the second thread executes the
subsequent loop:

    for(i=1; i<m/4; i++)
        tab[m/2-i] = tab[i];

The second thread loads the incorrect value into a registers, then the first
thread replaces the incorrect value with a correct value and finishes executing
the function. Then the second thread resumes with the incorrect value in a
register and stores in back into the array. We end up with the incorrect value
being stored in the array persistently.


> Additionally, the code for ff_thread_once is only pthread_once on POSIX, on
other platforms (Windows in our case) it appears to try and rely on thread safe
statics...

As far as I see:
http://git.videolan.org/?p=ffmpeg.git;a=blob;f=compat/w32pthreads.h
windows code uses InitOnceBeginInitialize. Anyway, if there are two bugs, we
need to fix two bugs. Rather than consider it as an excuse to not fix any bugs.

I would say that ff_cos_tab elements should be augmented with AVOnce and init
function. Or maybe the partial initialization in not needed at all.

[DaleCurtis, 2015-10-23 04:38:39]

Thanks Dmitry, that's a great explanation and sorry I missed the w32 pthread
once. It's not currently in Chrome's repro, so we'll have to wait until we
update ffmpeg for M48 to pick up the pthread_once. I'll try to contribute the
upstream fix before then.

[DaleCurtis, 2015-10-24 00:51:26]

Closing this in favor of
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-October/181938.html