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

[Michael Niedermayer]

On Sun, Sep 14, 2014 at 01:01:45AM +0200, Mikulas Patocka wrote:
> > > 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.

This is problematic,
it would work with gcc and clang i assume but we support many more
compilers and i suspect it wont work for all. Though maybe that
wouldnt matter as they probably dont do gcc inline asm either

But it would also be a special case as all other "what to built" cpu
extensions are controled by configure --enable/disable- flags

also having to use -march to disable could be annoying for debuging
as -march affects alot more


> 
> 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.

thats true,
but maybe its better if you put this check in configure so the
--enable/disable flags can be used to override it

but iam not really opposed to this hunk either if others are ok with
it


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140917/d2538dad/attachment.asc>