FDH2 / UxPlay

AirPlay Unix mirroring server
GNU General Public License v3.0
1.35k stars 72 forks source link

Remove -march=native flag to make packaging friendly #69

Closed WUGqnwvMQPzl closed 2 years ago

WUGqnwvMQPzl commented 2 years ago

I'm using Arch Linux and archlinuxcn repository have pre-built UxPlay package (uxplay-git), but when I try to use it, program just throws "Illegal instruction" error and exits.

After asking people on Arch Linux CN community, someone pointed out it may be compiled with -march=native flag, and I found that flag on cmakelists file.

So I removed the flag and recompile it, things still works and hopefully it can fix "Illegal instruction" error on pre-built package on software repositories.

fduncanh commented 2 years ago

Sounds fine. These CFLAGS are inherited from initial versions of UxPlay, and I don't really understand them. Can you review all of them and see if anything else should be changed.

fduncanh commented 2 years ago

Looking at this its clear that -march=native is not wanted for packaging, but might be wanted when compiling for one self. Does the AUR package have a script? I could make a cmake optiion line '-DNO_MARCH_NATIVE=ON" to omit it when packaging?

Optimized CFLAGS for modern processors

CFLAGS="-O2 -pipe -march=native" makes gcc enable all the features of the CPU it is running on. That's great as long as you do not plan on running the resulting binaries on another computer ever. You may want to do that and optimize for modern processors.. Having your cake and eating it is tricky so some compromises are required. One you may accept is to optimize for desktop computers made after 2015. That gives you:

    -mcx16 # CMPXCHG16B, AMD64 2005
    -mmmx -msse -msse2 # Intel 2001, AMD 2003
    -msse3 # Intel 2004, AMD 2007
    -mssse3 # Intel 2006, AMD 2011
    -msse4.1 -msse4.2 # Intel 2008, AMD 2011
    -mavx # Intel 2011, AMD 2011
    -mavx2 # Intel 2013, AMD 2015
WUGqnwvMQPzl commented 2 years ago

These CFLAGS are inherited from initial versions of UxPlay, and I don't really understand them. Can you review all of them and see if anything else should be changed.

I'm not good at C programming either, so I'm afraid I cannot reviewing all flags, sorry.

Does the AUR package have a script? I could make a cmake optiion line '-DNO_MARCH_NATIVE=ON" to omit it when packaging?

You can view the AUR packaging script from their webpage like this: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=uxplay-git It just compiles from cmake and package it.

Anyway, add the -DNO_MARCH_NATIVE option sounds a good idea.

fduncanh commented 2 years ago

I checked how RPiPlay does it now, and I see that now it has

# Common x86/x86_64 cflags
if( CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)" )
    set( CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Ofast -march=native" )
endif()

so I did the same in UxPlay. (now in github) I am guessing that the "illegal instruction" is in builds on Arm (Raspberry Pi?) not intel x8/x86_64 ?

is that correct?

If so, this should fix the problem with future builds.

WUGqnwvMQPzl commented 2 years ago

I checked how RPiPlay does it now, and I see that now it has

# Common x86/x86_64 cflags
if( CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)" )
    set( CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Ofast -march=native" )
endif()

so I did the same in UxPlay. (now in github) I am guessing that the "illegal instruction" is in builds on Arm (Raspberry Pi?) not intel x8/x86_64 ?

is that correct?

If so, this should fix the problem with future builds.

Nope, I tried the latest git version of UxPlay on archlinuxcn repository, still gets the "illegal hardware instruction" error.

[username and hostname] ~ % paru -Qi uxplay-git 
名字           : uxplay-git
版本           : r548.8759819-1
描述           : AirPlay Unix mirroring server
架构           : any
URL            : https://github.com/FDH2/UxPlay
软件许可       : GPL3
组             : 无
提供           : uxplay
依赖于         : gstreamer  gst-plugins-base  gst-plugins-base-libs  gst-libav
                 gstreamer-vaapi  avahi  libplist
可选依赖       : 无
依赖它         : 无
被可选依赖     : 无
与它冲突       : uxplay
取代           : 无
安装后大小     : 488.37 KiB
打包者         : lilac (on behalf of SiHuan) <sihuan@sakuya.love>
编译日期       : 2022年02月28日 星期一 04时43分20秒
安装日期       : 2022年02月28日 星期一 08时57分55秒
安装原因       : 单独指定安装
安装脚本       : 否
验证者         : 数字签名

[username and hostname] ~ % uxplay 
using system MAC address 7c:8a:e1:b6:d1:c8
Initialized server socket(s)
[1]    3106 illegal hardware instruction (core dumped)  uxplay
132 [username and hostname] ~ % file $(which uxplay)
/usr/bin/uxplay: ELF 64-bit LSB pie executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=44423532b5cc730ffa271aa4fbe350981b98bdae, for GNU/Linux 4.4.0, stripped

Both my desktop and laptop are using AMD Ryzen CPU, but I don't really know what CPU archlinuxcn's compiling server using, I'm guessing the Intel family ones?

fduncanh commented 2 years ago

You are correct: -march=native should only be used for compiling something to run on the same machine it was compiled on.

I added a cmake option cmake -DNO_MARCH_NATIVE=ON to switch it off, and a cmake warning if the -march=native option is used. The -DNO_MARCH_NATIVE=ON cmake option should be added to the AUR build scripts.

The change is:

# Common x86/x86_64 cflags
if( NOT NO_MARCH_NATIVE AND CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)" )
  set( CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -march=native" )
  message( STATUS "Using CFLAGS with  -march=native" )
  message( STATUS "*** ONLY USE THIS WHEN COMPILING ON THE MACHINE THAT WLL RUN UXPLAY" )
  message( STATUS "  run \"cmake -DNO_MARCH_NATIVE=ON\" to switch off this compiler option" )
endif()

I also replaced -Ofast by -O2

WUGqnwvMQPzl commented 2 years ago

You are correct: -march=native should only be used for compiling something to run on the same machine it was compiled on.

I added a cmake option cmake -DNO_MARCH_NATIVE=ON to switch it off, and a cmake warning if the -march=native option is used. The -DNO_MARCH_NATIVE=ON cmake option should be added to the AUR build scripts.

The change is:

# Common x86/x86_64 cflags
if( NOT NO_MARCH_NATIVE AND CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)" )
  set( CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -march=native" )
  message( STATUS "Using CFLAGS with  -march=native" )
  message( STATUS "*** ONLY USE THIS WHEN COMPILING ON THE MACHINE THAT WLL RUN UXPLAY" )
  message( STATUS "  run \"cmake -DNO_MARCH_NATIVE=ON\" to switch off this compiler option" )
endif()

I also replaced -Ofast by -O2

Tried compiling on my VPS and install it on my main AMD machine with that option, works like charm, thanks!