[FFmpeg-devel] [PATCH] avutil/utils: Remove racy check from avutil_version()

[Andreas Rheinhardt]

James Almer:
> On 9/26/2021 6:46 AM, Andreas Rheinhardt wrote:
>> avutil_version() currently performs several checks before
>> just returning the version. There is a static int that aims
>> to ensure that these tests are run only once. The reason is that
>> there used to be a slightly expensive check, but it has been removed
>> in 92e3a6fdac73f7e1d69d69717219a7538877d7a0. Today running only
>> once is unnecessary and can be counterproductive: GCC 10 optimizes
>> all the actual checks away, but the checks_done variable and the code
>> setting it has been kept. Given that this check is inherently racy
>> (it uses non-atomic variables), it is best to just remove it.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>> A part of me just wants to nuke all those checks.
>> (We have zero callers of avutil_version() in the codebase and
>> I don't see a reason why external users should call it more often,
>> so all those checks probably don't fulfill their aim.)
> 
> We could replace them with C11's _Static_assert (Making sure that for
> C99 compilers like msvc it just becomes av_assert0). That way they will
> trigger at compilation time instead, which is more in line to what they
> were added for.
> 

It is not guaranteed that this works for the llrint and the
av_sat_dadd32 check, because a static assert requires a constant
expression; and it makes no sense to ever use av_assert0(0) for those
checks that can be done at compile-time, as there already is a way to
emulate static asserts in pre-C11 C: Declare an array (a type is enough)
with a negative number of elements in case of failure. See AV_CHECK_OFFSET.

>>
>>   libavutil/utils.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/libavutil/utils.c b/libavutil/utils.c
>> index c1cd452eee..ea9b5097b8 100644
>> --- a/libavutil/utils.c
>> +++ b/libavutil/utils.c
>> @@ -37,10 +37,6 @@ const char *av_version_info(void)
>>     unsigned avutil_version(void)
>>   {
>> -    static int checks_done;
>> -    if (checks_done)
>> -        return LIBAVUTIL_VERSION_INT;
>> -
>>       av_assert0(AV_SAMPLE_FMT_DBLP == 9);
>>       av_assert0(AVMEDIA_TYPE_ATTACHMENT == 4);
>>       av_assert0(AV_PICTURE_TYPE_BI == 7);
>> @@ -58,7 +54,6 @@ unsigned avutil_version(void)
>>           av_log(NULL, AV_LOG_ERROR, "Libavutil has been linked to a
>> broken llrint()\n");
>>       }
>>   -    checks_done = 1;
>>       return LIBAVUTIL_VERSION_INT;
>>   }
>>