| Message ID | AS8P250MB0744D5687E6077103AFA02268F319@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | 894191e7e10520109db983032d1cd8d45c85af6d |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/svq1enc: Workaround GCC bug 102513 | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
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.
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
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(+)