[FFmpeg-devel] libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL is greater than 1

[Marton Balint]

On Fri, 21 Apr 2017, Marton Balint wrote:

>
> On Thu, 20 Apr 2017, Aaron Levinson wrote:
>
>> On 4/19/2017 2:27 PM, Marton Balint wrote:
>>>
>>> On Mon, 17 Apr 2017, James Almer wrote:
>>>
>>>> On 4/17/2017 5:39 AM, Clément Bœsch wrote:
>>>>> On Sun, Apr 16, 2017 at 05:20:02PM -0700, Aaron Levinson wrote:
>>>>>> From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001
>>>>>> From: Aaron Levinson <alevinsn at aracnet.com>
>>>>>> Date: Sun, 16 Apr 2017 17:13:31 -0700
>>>>>> Subject: [PATCH] libavutil/thread.h:  Fixed g++ build error when
>>>>>>  ASSERT_LEVEL is greater than 1
>>>>>>
>>>>>> Purpose: libavutil/thread.h: Fixed g++ build error when ASSERT_LEVEL
>>>>>> is greater than 1.  This is only relevant when thread.h is included by
>>>>>> C++ files.  In this case, the relevant code is only defined if
>>>>>> HAVE_PTHREADS is defined as 1.  Use configure --assert-level=2 to do
>>>>>> so.
>>>>>>
>>>>>> Note: Issue discovered as a result of Coverity build failure.  Cause
>>>>>> of build failure pinpointed by Hendrik Leppkes.
>>>>>>
>>>>>> Comments:
>>>>>>
>>>>>> -- libavutil/thread.h: Altered ASSERT_PTHREAD_NORET definition such
>>>>>>    that it uses av_make_error_string instead of av_err2str().
>>>>>>    av_err2str() uses a "parenthesized type followed by an initializer
>>>>>>    list", which is apparently not valid C++.  This issue started
>>>>>>    occurring because thread.h is now included by the DeckLink C++
>>>>>>    files.  The alteration does the equivalent of what av_err2str()
>>>>>>    does, but instead declares the character buffer as a local
>>>>>>    variable.
>>>>>> ---
>>>>>>  libavutil/thread.h | 5 ++++-
>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavutil/thread.h b/libavutil/thread.h
>>>>>> index 6e57447..f108e20 100644
>>>>>> --- a/libavutil/thread.h
>>>>>> +++ b/libavutil/thread.h
>>>>>> @@ -36,8 +36,11 @@
>>>>>>  #define ASSERT_PTHREAD_NORET(func, ...) do
>>>>>> {                            \
>>>>>>      int ret =
>>>>>> func(__VA_ARGS__);                                        \
>>>>>>      if (ret)
>>>>>> {                                                          \
>>>>>> +        char errbuf[AV_ERROR_MAX_STRING_SIZE] =
>>>>>> "";                     \
>>>>>>          av_log(NULL, AV_LOG_FATAL,
>>>>>> AV_STRINGIFY(func)                   \
>>>>>> -               " failed with error: %s\n",
>>>>>> av_err2str(AVERROR(ret)));   \
>>>>>> +               " failed with error:
>>>>>> %s\n",                              \
>>>>>> +               av_make_error_string(errbuf,
>>>>>> AV_ERROR_MAX_STRING_SIZE,   \
>>>>>> +
>>>>>> AVERROR(ret)));                     \
>>>>>>
>>>>>> abort();                                                        \
>>>>>>
>>>>>> }                                                                   \
>>>>>>  } while (0)
>>>>>
>>>>> I don't like limiting ourselves in the common C code of the project
>>>>> because C++ is a bad and limited language. Can't you solve this by
>>>>> bumping
>>>>> the minimal requirement of C++ version?
>>>>
>>>> We're already using C++11 when available because of atomics on
>>>> mediacodec.
>>>> Also, just tried and it seems to fail even with C++14, so it just doesn't
>>>> work with C++.
>>>>
>>>> We could instead just make these strict assert wrappers work only on C
>>>> code by for example checking for defined(__cplusplus).
>>>
>>> I'd say let's apply the patch as is, that is the simplest solution.
>>>
>>> Regards,
>>> Marton
>>
>> I don't think most people care one way or the other--perhaps someone 
>> with push privileges could apply the patch?  I assume that the Coverity 
>> builds are continuing to fail as a result of the issue that this patch 
>> addresses.
>
> Ok, I will apply this tomorrow.
>

Applied.

Regards,
Marton