| Message ID | 20230102230128.972907-1-ccom@randomderp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] get_cabac_inline_x86: Don't inline if 32-bit Windows | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Tue, Jan 3, 2023 at 12:01 AM Christopher Degawa <ccom@randomderp.com> wrote: > > previouslly, it only was an issue with 32-bit clang from msys2's > mingw32 repo, however, at some point with an update to gcc 12.2.0, > the same issue popped up. Tested with a clean clone of ffmpeg, and even > tested with n5.0, but the issue persists, so I presume it's a compiler > issue. > > Related: https://trac.ffmpeg.org/ticket/8903 > I regularly build with 12.2 on win32 and its fine. In fact, there is a fate station for that: https://fate.ffmpeg.org/report.cgi?slot=x86_32-mingw-w64-dll-windows-native&time=20230102232810 So if you are seeing this issue, more details that trigger it will be required, and maybe a more targeted fix. - Hendrik
Christopher Degawa: > previouslly, it only was an issue with 32-bit clang from msys2's > mingw32 repo, however, at some point with an update to gcc 12.2.0, > the same issue popped up. Tested with a clean clone of ffmpeg, and even > tested with n5.0, but the issue persists, so I presume it's a compiler > issue. > > Related: https://trac.ffmpeg.org/ticket/8903 > > Signed-off-by: Christopher Degawa <ccom@randomderp.com> > --- > libavcodec/x86/cabac.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/x86/cabac.h b/libavcodec/x86/cabac.h > index b046a56a6b..70f990db8d 100644 > --- a/libavcodec/x86/cabac.h > +++ b/libavcodec/x86/cabac.h > @@ -178,7 +178,7 @@ > #if HAVE_7REGS && !BROKEN_COMPILER > #define get_cabac_inline get_cabac_inline_x86 > static > -#if defined(_WIN32) && !defined(_WIN64) && defined(__clang__) > +#if defined(_WIN32) && !defined(_WIN64) > av_noinline > #else > av_always_inline Have these presumed compiler bugs ever been reported upstream? - Andreas
On Mon, Jan 2, 2023 at 5:36 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Jan 3, 2023 at 12:01 AM Christopher Degawa <ccom@randomderp.com> > wrote: > > > > I regularly build with 12.2 on win32 and its fine. > > In fact, there is a fate station for that: > > https://fate.ffmpeg.org/report.cgi?slot=x86_32-mingw-w64-dll-windows-native&time=20230102232810 > > So if you are seeing this issue, more details that trigger it will be > required, and maybe a more targeted fix. > > - Hendrik I can try to see if I can narrow down the configuration more, and try the one used in fate, but for now, I was reproducing it by using ../configure && make -j 12 As additional information for now, I'm using Target: i686-w64-mingw32 gcc version 12.2.0 (Rev7, Built by MSYS2 project) with no notable environment variables like CFLAGS etc. I did make sure to update my packages from msys2. Interestingly, when I ran "../configure --enable-gpl --enable-memory-poisoning --arch=x86 --cpu=i686 --enable-shared" there were no errors, but did confirm that with just "../configure && make libavcodec/h264_cabac.o" the error reappeared. CC libavcodec/h264_cabac.o In file included from C:/Users/cddeg/FFmpeg/libavcodec/cabac_functions.h:49, from C:/Users/cddeg/FFmpeg/libavcodec/h264_cabac.c:36: In function 'get_cabac_inline_x86', inlined from 'get_cabac' at C:/Users/cddeg/FFmpeg/libavcodec/cabac_functions.h:145:12, inlined from 'decode_cabac_mb_intra4x4_pred_mode' at C:/Users/cddeg/FFmpeg/libavcodec/h264_cabac.c:1377:9, inlined from 'ff_h264_decode_mb_cabac' at C:/Users/cddeg/FFmpeg/libavcodec/h264_cabac.c:2081:32: C:/Users/cddeg/FFmpeg/libavcodec/x86/cabac.h:199:5: error: 'asm' operand has impossible constraints 199 | __asm__ volatile( | ^~~~~~~ C:/Users/cddeg/FFmpeg/libavcodec/x86/cabac.h:199:5: error: 'asm' operand has impossible constraints C:/Users/cddeg/FFmpeg/libavcodec/x86/cabac.h:199:5: error: 'asm' operand has impossible constraints C:/Users/cddeg/FFmpeg/libavcodec/x86/cabac.h:199:5: error: 'asm' operand has impossible constraints make: *** [/c/Users/cddeg/FFmpeg/ffbuild/common.mak:81: libavcodec/h264_cabac.o] Error 1 I will try to see which flag in that configure line causes the issues to disappear
On Tue, Jan 3, 2023 at 12:32 PM Christopher Degawa <ccom@randomderp.com> wrote: > > > On Mon, Jan 2, 2023 at 5:36 PM Hendrik Leppkes <h.leppkes@gmail.com> > wrote: > >> On Tue, Jan 3, 2023 at 12:01 AM Christopher Degawa <ccom@randomderp.com> >> wrote: >> > >> >> I regularly build with 12.2 on win32 and its fine. >> >> In fact, there is a fate station for that: >> >> https://fate.ffmpeg.org/report.cgi?slot=x86_32-mingw-w64-dll-windows-native&time=20230102232810 >> >> So if you are seeing this issue, more details that trigger it will be >> required, and maybe a more targeted fix. >> >> - Hendrik > > I will try to see which flag in that configure line causes the issues to > disappear > Seems I got it to disappear with just "--cpu=i686", so "ARCH x86 (generic)" fails but "ARCH x86 (i686)" passes
On Tue, Jan 3, 2023 at 4:16 AM Andreas Rheinhardt < andreas.rheinhardt@outlook.com> wrote: > Christopher Degawa: > > previouslly, it only was an issue with 32-bit clang from msys2's > > mingw32 repo, however, at some point with an update to gcc 12.2.0, > > the same issue popped up. Tested with a clean clone of ffmpeg, and even > > tested with n5.0, but the issue persists, so I presume it's a compiler > > issue. > > Have these presumed compiler bugs ever been reported upstream? > > - Andreas > I'm not sure if it's necessarily a compiler bug, rather I'm suspecting something was changed in msys2's i686 gcc's. Based on my small findings, it might be related to what “generic” means for -mtune for the 32-bit compiler. I was asking around msys2's channels to see if anyone else could reproduce it, but nobody has responded yet.
On Tue, Jan 3, 2023 at 12:51 PM Christopher Degawa <ccom@randomderp.com> wrote: > > > On Tue, Jan 3, 2023 at 4:16 AM Andreas Rheinhardt < > andreas.rheinhardt@outlook.com> wrote: > >> Christopher Degawa: >> > previouslly, it only was an issue with 32-bit clang from msys2's >> > mingw32 repo, however, at some point with an update to gcc 12.2.0, >> > the same issue popped up. Tested with a clean clone of ffmpeg, and even >> > tested with n5.0, but the issue persists, so I presume it's a compiler >> > issue. >> >> Have these presumed compiler bugs ever been reported upstream? >> >> - Andreas >> > > I'm not sure if it's necessarily a compiler bug, rather I'm suspecting > something was changed in msys2's i686 gcc's. Based on my small findings, it > might be related to what “generic” means for -mtune for the 32-bit > compiler. I was asking around msys2's channels to see if anyone else could > reproduce it, but nobody has responded yet. > Seems some of them have encountered it https://github.com/msys2/MINGW-packages/pull/11683
On Mon, 2 Jan 2023, Christopher Degawa wrote: > previouslly, it only was an issue with 32-bit clang from msys2's > mingw32 repo, however, at some point with an update to gcc 12.2.0, > the same issue popped up. Tested with a clean clone of ffmpeg, and even > tested with n5.0, but the issue persists, so I presume it's a compiler > issue. > > Related: https://trac.ffmpeg.org/ticket/8903 > > Signed-off-by: Christopher Degawa <ccom@randomderp.com> > --- > libavcodec/x86/cabac.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/x86/cabac.h b/libavcodec/x86/cabac.h > index b046a56a6b..70f990db8d 100644 > --- a/libavcodec/x86/cabac.h > +++ b/libavcodec/x86/cabac.h > @@ -178,7 +178,7 @@ > #if HAVE_7REGS && !BROKEN_COMPILER > #define get_cabac_inline get_cabac_inline_x86 > static > -#if defined(_WIN32) && !defined(_WIN64) && defined(__clang__) > +#if defined(_WIN32) && !defined(_WIN64) > av_noinline > #else > av_always_inline > -- > 2.39.0 I'm ok with this change (although I'd reword the commit message a bit); the inline assembly here is brittle and it's easy to trigger this failure. It's very hard to say whether this is a compiler bug, or just our assembly crossing the line for what we can demand that the compiler accommodates, in combination with deep inlineing - the patch avoids inlineing the function which makes it much more possible to handle the inline assembly constraints. I and Hendrik discussed the issue on irc; I reproduced the issue with MSYS2's 32 bit GCC, and Hendrik also separately reproduced it with a separate build of GCC, by adding the options "-march=pentium4 -mtune=generic", which are MSYS2's current defaults, which trigger the failure (at least in mingw builds, with GCC 12.2.0). So if there's no reasonable opposition, I'd go ahead and push this patch with a slightly reworded commit message. // Martin
On Mon, Mar 27, 2023 at 7:35 AM Martin Storsjö <martin@martin.st> wrote: > On Mon, 2 Jan 2023, Christopher Degawa wrote: > > > previouslly, it only was an issue with 32-bit clang from msys2's > > mingw32 repo, however, at some point with an update to gcc 12.2.0, > > the same issue popped up. Tested with a clean clone of ffmpeg, and even > > tested with n5.0, but the issue persists, so I presume it's a compiler > > issue. > > > > Related: https://trac.ffmpeg.org/ticket/8903 > > > > Signed-off-by: Christopher Degawa <ccom@randomderp.com> > > --- > > libavcodec/x86/cabac.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/x86/cabac.h b/libavcodec/x86/cabac.h > > index b046a56a6b..70f990db8d 100644 > > --- a/libavcodec/x86/cabac.h > > +++ b/libavcodec/x86/cabac.h > > @@ -178,7 +178,7 @@ > > #if HAVE_7REGS && !BROKEN_COMPILER > > #define get_cabac_inline get_cabac_inline_x86 > > static > > -#if defined(_WIN32) && !defined(_WIN64) && defined(__clang__) > > +#if defined(_WIN32) && !defined(_WIN64) > > av_noinline > > #else > > av_always_inline > > -- > > 2.39.0 > > I'm ok with this change (although I'd reword the commit message a bit); > the inline assembly here is brittle and it's easy to trigger this failure. > > It's very hard to say whether this is a compiler bug, or just our assembly > crossing the line for what we can demand that the compiler accommodates, > in combination with deep inlineing - the patch avoids inlineing the > function which makes it much more possible to handle the inline assembly > constraints. > > I and Hendrik discussed the issue on irc; I reproduced the issue with > MSYS2's 32 bit GCC, and Hendrik also separately reproduced it with a > separate build of GCC, by adding the options "-march=pentium4 > -mtune=generic", which are MSYS2's current defaults, which trigger the > failure (at least in mingw builds, with GCC 12.2.0). > > So if there's no reasonable opposition, I'd go ahead and push this patch > with a slightly reworded commit message. > > // Martin > This may need to be re-done since according to https://trac.ffmpeg.org/ticket/8903, it seems to be affecting linux 32-bit as well with the latest compilers.
On Mon, 27 Mar 2023, Christopher Degawa wrote: > This may need to be re-done since according to > https://trac.ffmpeg.org/ticket/8903, > it seems to be affecting linux 32-bit as well with the latest compilers. Oh, interesting - both me and Hendrik tried to reproduce it with 32 bit compilers for Linux today, but the combination that triggered it on Windows (-march=pentium4 -mtune=generic) didn't trigger it there. With that in mind, I think it's rather clear that we should make this function noinline for all i386 combinations; yes it used to work back in the day, but compilers these days do way more things than they used to do, and inlineing this everywhere might not be feasible on i386. // Martin
diff --git a/libavcodec/x86/cabac.h b/libavcodec/x86/cabac.h index b046a56a6b..70f990db8d 100644 --- a/libavcodec/x86/cabac.h +++ b/libavcodec/x86/cabac.h @@ -178,7 +178,7 @@ #if HAVE_7REGS && !BROKEN_COMPILER #define get_cabac_inline get_cabac_inline_x86 static -#if defined(_WIN32) && !defined(_WIN64) && defined(__clang__) +#if defined(_WIN32) && !defined(_WIN64) av_noinline #else av_always_inline
previouslly, it only was an issue with 32-bit clang from msys2's mingw32 repo, however, at some point with an update to gcc 12.2.0, the same issue popped up. Tested with a clean clone of ffmpeg, and even tested with n5.0, but the issue persists, so I presume it's a compiler issue. Related: https://trac.ffmpeg.org/ticket/8903 Signed-off-by: Christopher Degawa <ccom@randomderp.com> --- libavcodec/x86/cabac.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)