| Message ID | 9faab0ce-29d5-0f04-59e1-bcaea04abeb4@aracnet.com |
|---|---|
| State | Accepted |
| Commit | 5b281b476b32c35527c0eea5f42161c4acad83f9 |
| Headers | show |
I'll try that again with [PATCH] in the subject line.
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@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?
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@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).
On Mon, 17 Apr 2017 12:06:59 -0300 James Almer <jamrial@gmail.com> 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@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). Better solution: move all the code to a .c file.
On 4/17/2017 8:28 AM, wm4 wrote: > On Mon, 17 Apr 2017 12:06:59 -0300 > James Almer <jamrial@gmail.com> 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@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). > > Better solution: move all the code to a .c file. I've spent some time considering what would be involved in moving the relevant code into a .c file. thread.h is a header file that needs to be included to provide implementations of various pthread_ APIs on Windows and OS/2 without needing to link to pthread on those OS's. If pthread is available, it just includes pthread.h. So, it sort of acts like a portability layer. Providing it in the form of a .h file acts as a convenience. If the implementation were moved into a .c file, that tends to imply that it will reside in one of the ffmpeg libraries, likely libavutil. And that also implies that functions with the name pthread_create, etc, would be exported by libavutil, which is a bad idea. Instead, the right way to go is to provide a true threading portability layer with exported functions that start with, say, av_thread_. But, that's a decent project, and while I'm willing to undertake it, I would like to see some support for this endeavor first. However, there also seems to be some resistance to supporting C++ in ffmpeg. The DeckLink C++ files were contributed to ffmpeg in February 2014, over three years ago. While there is certainly no issue with using C-specific functionality in .c files, there is certainly an issue with doing so in header files that are intended to be used by any aspect of the project, whether in .c or .cpp files. thread.h is an example of a header file that should be suitable for use in either .c or .cpp files. The patch that I submitted accomplishes exactly that. Aaron Levinson
On Mon, 17 Apr 2017 18:14:45 -0700 Aaron Levinson <alevinsn@aracnet.com> wrote: > On 4/17/2017 8:28 AM, wm4 wrote: > > On Mon, 17 Apr 2017 12:06:59 -0300 > > James Almer <jamrial@gmail.com> 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@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). > > > > Better solution: move all the code to a .c file. > > I've spent some time considering what would be involved in moving the > relevant code into a .c file. thread.h is a header file that needs to > be included to provide implementations of various pthread_ APIs on > Windows and OS/2 without needing to link to pthread on those OS's. If > pthread is available, it just includes pthread.h. So, it sort of acts > like a portability layer. Providing it in the form of a .h file acts as > a convenience. If the implementation were moved into a .c file, that > tends to imply that it will reside in one of the ffmpeg libraries, > likely libavutil. And that also implies that functions with the name > pthread_create, etc, would be exported by libavutil, which is a bad > idea. Instead, the right way to go is to provide a true threading > portability layer with exported functions that start with, say, > av_thread_. But, that's a decent project, and while I'm willing to > undertake it, I would like to see some support for this endeavor first. Good point, I forgot about that headache. There are various tricks to get around it, but since we actually need to have visible symbols to use it from C++, it all becomes a step harder. > However, there also seems to be some resistance to supporting C++ in > ffmpeg. The DeckLink C++ files were contributed to ffmpeg in February > 2014, over three years ago. While there is certainly no issue with > using C-specific functionality in .c files, there is certainly an issue > with doing so in header files that are intended to be used by any aspect > of the project, whether in .c or .cpp files. thread.h is an example of > a header file that should be suitable for use in either .c or .cpp > files. The patch that I submitted accomplishes exactly that.
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@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
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@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. Thanks, Aaron Levinson
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@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. Regards, Marton
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@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
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)
From 9e6a9e2b8d58f17c661a3f455e03c95587ec7b18 Mon Sep 17 00:00:00 2001 From: Aaron Levinson <alevinsn@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(-)