| Message ID | AM7PR03MB66600E7AD758E952312EB0F68FA69@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | ff800903747325f15056444c11053279e391d60b |
| Headers | show |
| Series | [FFmpeg-devel] avutil/utils: Remove racy check from avutil_version() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
lgtm
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@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. > > 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; > } > >
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@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; >> } >>
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; }
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@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.) libavutil/utils.c | 5 ----- 1 file changed, 5 deletions(-)