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

[Hendrik Leppkes]

On Fri, Sep 12, 2014 at 9:49 PM, Mikulas Patocka
<mikulas at artax.karlin.mff.cuni.cz> 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.