[FFmpeg-devel] [PATCH] avcodec/svq1enc: Workaround GCC bug 102513

[FFmpeg-devel] [PATCH] avcodec/svq1enc: Workaround GCC bug 102513

[Andreas Rheinhardt]

GCC 11 has a bug: When it creates clones of recursive functions
(to inline some parameters), it clones a recursive function
eight times by default, even when this exceeds the recursion
depth. This happens with encode_block() in libavcodec/svq1enc.c
where a parameter level is always in the range 0..5;
but GCC 11 also creates functions corresponding to level UINT_MAX
and UINT_MAX - 1 (on -O3; -O2 is fine).

Using such levels would produce undefined behaviour and because
of this GCC emits bogus -Warray-bounds warnings for these clones.

Since commit d08b2900a9f0935959303da668cb00a8a7245228, certain
symbols that are accessed like ff_svq1_inter_multistage_vlc[level]
are declared with hidden visibility, which allows compilers
to bake the offset implied by level into the instructions
if level is a compile-time constant as it is in the clones.
Yet this leads to insane offsets for level == UINT_MAX which
can be incompatible with the supported offset ranges of relocations.
This happens in the small code model (the default code model for
AArch64).

This commit therefore works around this bug by disabling cloning
recursive functions for GCC 10 and 11. GCC 10 is affected by the
underlying bug (see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102513), so the workaround
also targets it, although it only produces three versions of
encode_block(), so it does not seem to trigger the actual issue here.

The issue has been mitigated in GCC 12.1 (it no longer creates clones
for impossible values; see also commit
1cb7fd317c84117bbb13b14851d62f77f57bb9ce), so the workaround
does not target it.

Reported-by: Josh Dekker <josh@itanimul.li>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/svq1enc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libavcodec/svq1enc.c b/libavcodec/svq1enc.c
index 75adbe7ea0..7c9430a137 100644
--- a/libavcodec/svq1enc.c
+++ b/libavcodec/svq1enc.c
@@ -46,6 +46,12 @@
 #include "libavutil/frame.h"
 #include "libavutil/mem_internal.h"
 
+// Workaround for GCC bug 102513
+#if AV_GCC_VERSION_AT_LEAST(10, 0) && AV_GCC_VERSION_AT_MOST(12, 0) \
+    && !defined(__clang__) && !defined(__INTEL_COMPILER)
+#pragma GCC optimize ("no-ipa-cp-clone")
+#endif
+
 typedef struct SVQ1EncContext {
     /* FIXME: Needed for motion estimation, should not be used for anything
      * else, the idea is to make the motion estimation eventually independent
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/svq1enc: Workaround GCC bug 102513

[J. Dekker]

On 25 Oct 2022, at 14:48, Andreas Rheinhardt wrote:

> GCC 11 has a bug: When it creates clones of recursive functions
> (to inline some parameters), it clones a recursive function
> eight times by default, even when this exceeds the recursion
> depth. This happens with encode_block() in libavcodec/svq1enc.c
> where a parameter level is always in the range 0..5;
> but GCC 11 also creates functions corresponding to level UINT_MAX
> and UINT_MAX - 1 (on -O3; -O2 is fine).
>
> Using such levels would produce undefined behaviour and because
> of this GCC emits bogus -Warray-bounds warnings for these clones.
>
> Since commit d08b2900a9f0935959303da668cb00a8a7245228, certain
> symbols that are accessed like ff_svq1_inter_multistage_vlc[level]
> are declared with hidden visibility, which allows compilers
> to bake the offset implied by level into the instructions
> if level is a compile-time constant as it is in the clones.
> Yet this leads to insane offsets for level == UINT_MAX which
> can be incompatible with the supported offset ranges of relocations.
> This happens in the small code model (the default code model for
> AArch64).
>
> This commit therefore works around this bug by disabling cloning
> recursive functions for GCC 10 and 11. GCC 10 is affected by the
> underlying bug (see
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102513), so the workaround
> also targets it, although it only produces three versions of
> encode_block(), so it does not seem to trigger the actual issue here.
>
> The issue has been mitigated in GCC 12.1 (it no longer creates clones
> for impossible values; see also commit
> 1cb7fd317c84117bbb13b14851d62f77f57bb9ce), so the workaround
> does not target it.
>
> Reported-by: Josh Dekker <josh@itanimul.li>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>  libavcodec/svq1enc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/libavcodec/svq1enc.c b/libavcodec/svq1enc.c
> index 75adbe7ea0..7c9430a137 100644
> --- a/libavcodec/svq1enc.c
> +++ b/libavcodec/svq1enc.c
> @@ -46,6 +46,12 @@
>  #include "libavutil/frame.h"
>  #include "libavutil/mem_internal.h"
>
> +// Workaround for GCC bug 102513
> +#if AV_GCC_VERSION_AT_LEAST(10, 0) && AV_GCC_VERSION_AT_MOST(12, 0) \
> +    && !defined(__clang__) && !defined(__INTEL_COMPILER)
> +#pragma GCC optimize ("no-ipa-cp-clone")
> +#endif
> +
>  typedef struct SVQ1EncContext {
>      /* FIXME: Needed for motion estimation, should not be used for anything
>       * else, the idea is to make the motion estimation eventually independent

Discussed on IRC, LGTM & pushed.

-- 
jd
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".