[FFmpeg-devel] [PATCH 1/2] Fix miscompilation for i586.

[Mikulas Patocka]

> > Hi
> >
> > Here I'm sending two patches to fix portability for 586-class machines
> > (Pentium, K6, etc.)
> >
> >
> > If the CPU is generic, 386, 486 or pentium, we must not use cmov in inline
> > assembler.
> >
> > Note that some Linux distributions are compiled for i686, and for them it is
> > possible to use cmov in the assembler (because gcc uses it anyway). To test for
> > this case, we test for defined(__i686__) || defined(__athlon__) ||
> > defined(__SSE__) (these macros are driven by -march flag to gcc) and use cmov if
> > one of these conditions is true.
> >
> > ---
> >  configure                |    4 ++++
> >  libavcodec/x86/mathops.h |    2 +-
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > Index: ffmpeg/configure
> > ===================================================================
> > --- ffmpeg.orig/configure       2014-09-12 21:46:25.710512294 +0200
> > +++ ffmpeg/configure    2014-09-12 21:46:32.099587711 +0200
> > @@ -3840,8 +3840,12 @@ elif enabled sparc; then
> >  elif enabled x86; then
> >
> >      case $cpu in
> > +        generic)
> > +            disable i686
> > +        ;;
> i686 extensions were specifically enabled some time ago by default,
> since we live in a modern world.
> If you're building for a older system, its your responsibility to pass
> the appropriate option.
> 
> >          i[345]86|pentium)
> >              cpuflags="-march=$cpu"
> > +            disable i686
> >              disable mmx
> >          ;;
> >          # targets that do NOT support nopl and conditional mov (cmov)
> > Index: ffmpeg/libavcodec/x86/mathops.h
> > ===================================================================
> > --- ffmpeg.orig/libavcodec/x86/mathops.h        2014-09-12 21:46:05.823390224 +0200
> > +++ ffmpeg/libavcodec/x86/mathops.h     2014-09-12 21:46:32.116251966 +0200
> > @@ -69,7 +69,7 @@ static av_always_inline av_const int64_t
> >
> >  #endif /* ARCH_X86_32 */
> >
> > -#if HAVE_I686
> > +#if HAVE_I686 || defined(__i686__) || defined(__athlon__) || defined(__SSE__)
> >  /* median of 3 */
> >  #define mid_pred mid_pred
> >  static inline av_const int mid_pred(int a, int b, int c)
> 
> I don't like this part, configure should control if the code is used,
> and if the i686 extensions are not enabled in configure, then they
> should not be built here.

In my opinion you could remove HAVE_I686 and disable/enable i686 at all.

The user sets CFLAGS="-march=xxx" variable when running ./configure and it 
selects the instruction set that is being generated.

In the program, you can easily determine what -march was passed to the 
compiler and react to it - if one of __i686__, __athlon__, __SSE__ is 
defined, you can use cmov, because the compiler is already generating 
cmov.

The problem is this: Debian or Slackware is compiled with -march=i586. So 
you don't want cmov to be generated when building binary for them. Fedora 
is compiled with -march=i686, so you can use cmov. But your configure 
script doesn't know if it is building a binary for Debian or Fedora, so it 
can't magically detect if generating cmov is appropriate or not. If you 
make decision based on -march, it will be right on all distributions.

Mikulas