Re: [PATCH v3 01/10] capstone: Convert Makefile bits to meson bits

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Richard Henderson <richard.henderson@linaro.org> writes:
>
>> There are better ways to do this, e.g. meson cmake subproject,
>> but that requires cmake 3.7 and some of our CI environments
>> only provide cmake 3.5.
>>
>> Nor can we add a meson.build file to capstone/, because the git
>> submodule would then always report "untracked files".  Fixing that
>> would require creating our own branch on the qemu git mirror, at
>> which point we could just as easily create a native meson subproject.
>>
>> Instead, build the library via the main meson.build.
>>
>> This improves the current state of affairs in that we will re-link
>> the qemu executables against a changed libcapstone.a, which we wouldn't
>> do before-hand.  In addition, the use of the configuration header file
>> instead of command-line -DEFINES means that we will rebuild the
>> capstone objects with changes to meson.build.
>
> Something is breaking when switching to a branch with this on from
> current master:
>
>   Linking target qemu-hppa
>   /usr/bin/ld: libcommon.fa.p/disas_alpha.c.o: in function `print_insn_alpha':
>   /home/alex/lsrc/qemu.git/builds/all/../../disas/alpha.c:1818: undefined 
> reference to `bfd_getl32'
>   collect2: error: ld returned 1 exit status
>   make: *** [Makefile.ninja:5965: qemu-alpha] Error 1
>   make: *** Waiting for unfinished jobs....
>   /usr/bin/ld: libcommon.fa.p/disas_hppa.c.o: in function `print_insn_hppa':
>   /home/alex/lsrc/qemu.git/builds/all/../../disas/hppa.c:1969: undefined 
> reference to `bfd_getb32'
>   collect2: error: ld returned 1 exit status
>   make: *** [Makefile.ninja:6259: qemu-hppa] Error 1
>
> Aggressively wiping out the submodule and doing a fresh configure in a
> empty build directory and I still see a failure:
>
>   ../../disas/capstone.c:25:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or 
> ‘__attribute__’ before ‘cap_skipdata_s390x_cb’
>    cap_skipdata_s390x_cb(const uint8_t *code, size_t code_size,
>    ^~~~~~~~~~~~~~~~~~~~~
>   ../../disas/capstone.c:49:17: error: ‘cap_skipdata_s390x_cb’ undeclared 
> here (not in a function); did you mean ‘cap_skipdata_s390x’?
>        .callback = cap_skipdata_s390x_cb
>                    ^~~~~~~~~~~~~~~~~~~~~
>                    cap_skipdata_s390x
>   Makefile.ninja:1424: recipe for target 'libcommon.fa.p/disas_capstone.c.o' 
> failed
>   make: *** [libcommon.fa.p/disas_capstone.c.o] Error 1
>   make: *** Waiting for unfinished jobs....
>
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> v2: Further reduce probing in configure (paolo),
>>     Drop state 'internal' and use 'git' even when it isn't git.
>>     Move CONFIG_CAPSTONE to config_host_data.
>> v3: Add Submodules separator; fix default in meson_options.txt.
>> ---
>>  configure         |  61 +++----------------------
>>  Makefile          |  16 -------
>>  meson.build       | 111 +++++++++++++++++++++++++++++++++++++++++++---
>>  meson_options.txt |   4 ++
>>  4 files changed, 115 insertions(+), 77 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 7564479008..76636c430d 100755
>> --- a/configure
>> +++ b/configure
>> @@ -478,7 +478,7 @@ opengl=""
>>  opengl_dmabuf="no"
>>  cpuid_h="no"
>>  avx2_opt=""
>> -capstone=""
>> +capstone="auto"
>>  lzo=""
>>  snappy=""
>>  bzip2=""
>> @@ -1580,7 +1580,7 @@ for opt do
>>    ;;
>>    --enable-capstone) capstone="yes"
>>    ;;
>> -  --enable-capstone=git) capstone="git"
>> +  --enable-capstone=git) capstone="internal"
>>    ;;
>>    --enable-capstone=system) capstone="system"
>>    ;;
>> @@ -5128,51 +5128,11 @@ fi
>>  # capstone
>>  
>>  case "$capstone" in
>> -  "" | yes)
>> -    if $pkg_config capstone; then
>> -      capstone=system
>> -    elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then
>> -      capstone=git
>> -    elif test -e "${source_path}/capstone/Makefile" ; then
>> -      capstone=internal
>> -    elif test -z "$capstone" ; then
>> -      capstone=no
>> -    else
>> -      feature_not_found "capstone" "Install capstone devel or git submodule"
>> -    fi
>> -    ;;
>> -
>> -  system)
>> -    if ! $pkg_config capstone; then
>> -      feature_not_found "capstone" "Install capstone devel"
>> -    fi
>> -    ;;
>> -esac
>> -
>> -case "$capstone" in
>> -  git | internal)
>> -    if test "$capstone" = git; then
>> +  auto | yes | internal)
>> +    # Simpler to always update submodule, even if not needed.
>> +    if test -e "${source_path}/.git" && test $git_update = 'yes' ; then
>>        git_submodules="${git_submodules} capstone"
>>      fi
>> -    mkdir -p capstone
>> -    if test "$mingw32" = "yes"; then
>> -      LIBCAPSTONE=capstone.lib
>> -    else
>> -      LIBCAPSTONE=libcapstone.a
>> -    fi
>> -    capstone_libs="-Lcapstone -lcapstone"
>> -    capstone_cflags="-I${source_path}/capstone/include"
>> -    ;;
>> -
>> -  system)
>> -    capstone_libs="$($pkg_config --libs capstone)"
>> -    capstone_cflags="$($pkg_config --cflags capstone)"
>> -    ;;
>> -
>> -  no)
>> -    ;;
>> -  *)
>> -    error_exit "Unknown state for capstone: $capstone"
>>      ;;
>>  esac
>>  
>> @@ -7292,11 +7252,6 @@ fi
>>  if test "$ivshmem" = "yes" ; then
>>    echo "CONFIG_IVSHMEM=y" >> $config_host_mak
>>  fi
>> -if test "$capstone" != "no" ; then
>> -  echo "CONFIG_CAPSTONE=y" >> $config_host_mak
>> -  echo "CAPSTONE_CFLAGS=$capstone_cflags" >> $config_host_mak
>> -  echo "CAPSTONE_LIBS=$capstone_libs" >> $config_host_mak
>> -fi
>>  if test "$debug_mutex" = "yes" ; then
>>    echo "CONFIG_DEBUG_MUTEX=y" >> $config_host_mak
>>  fi
>> @@ -7819,9 +7774,6 @@ done # for target in $targets
>>  if [ "$fdt" = "git" ]; then
>>    subdirs="$subdirs dtc"
>>  fi
>> -if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then
>> -  subdirs="$subdirs capstone"
>> -fi
>>  echo "SUBDIRS=$subdirs" >> $config_host_mak
>>  if test -n "$LIBCAPSTONE"; then
>>    echo "LIBCAPSTONE=$LIBCAPSTONE" >> $config_host_mak
>> @@ -8008,7 +7960,8 @@ NINJA=${ninja:-$PWD/ninjatool} $meson setup \
>>          -Db_coverage=$(if test "$gcov" = yes; then echo true; else echo 
>> false; fi) \
>>      -Dsdl=$sdl -Dsdl_image=$sdl_image \
>>      -Dvnc=$vnc -Dvnc_sasl=$vnc_sasl -Dvnc_jpeg=$vnc_jpeg -Dvnc_png=$vnc_png 
>> \
>> -    -Dgettext=$gettext -Dxkbcommon=$xkbcommon -Du2f=$u2f\
>> +    -Dgettext=$gettext -Dxkbcommon=$xkbcommon -Du2f=$u2f \
>> +    -Dcapstone=$capstone \
>>          $cross_arg \
>>          "$PWD" "$source_path"
>>  
>> diff --git a/Makefile b/Makefile
>> index 7c60b9dcb8..f3da1760ad 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -156,22 +156,6 @@ dtc/all: .git-submodule-status dtc/libfdt
>>  dtc/%: .git-submodule-status
>>      @mkdir -p $@
>>  
>> -# Overriding CFLAGS causes us to lose defines added in the sub-makefile.
>> -# Not overriding CFLAGS leads to mis-matches between compilation modes.
>> -# Therefore we replicate some of the logic in the sub-makefile.
>> -# Remove all the extra -Warning flags that QEMU uses that Capstone doesn't;
>> -# no need to annoy QEMU developers with such things.
>> -CAP_CFLAGS = $(patsubst -W%,,$(CFLAGS) $(QEMU_CFLAGS)) $(CAPSTONE_CFLAGS)
>> -CAP_CFLAGS += -DCAPSTONE_USE_SYS_DYN_MEM
>> -CAP_CFLAGS += -DCAPSTONE_HAS_ARM
>> -CAP_CFLAGS += -DCAPSTONE_HAS_ARM64
>> -CAP_CFLAGS += -DCAPSTONE_HAS_POWERPC
>> -CAP_CFLAGS += -DCAPSTONE_HAS_X86
>> -
>> -.PHONY: capstone/all
>> -capstone/all: .git-submodule-status
>> -    $(call quiet-command,$(MAKE) -C $(SRC_PATH)/capstone CAPSTONE_SHARED=no 
>> BUILDDIR="$(BUILD_DIR)/capstone" CC="$(CC)" AR="$(AR)" LD="$(LD)" 
>> RANLIB="$(RANLIB)" CFLAGS="$(CAP_CFLAGS)" $(SUBDIR_MAKEFLAGS) 
>> $(BUILD_DIR)/capstone/$(LIBCAPSTONE))
>> -
>>  .PHONY: slirp/all
>>  slirp/all: .git-submodule-status
>>      $(call quiet-command,$(MAKE) -C $(SRC_PATH)/slirp               \
>> diff --git a/meson.build b/meson.build
>> index f4d1ab1096..f23273693d 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -10,6 +10,7 @@ else
>>    keyval = import('unstable-keyval')
>>  endif
>>  ss = import('sourceset')
>> +fs = import('fs')
>>  
>>  sh = find_program('sh')
>>  cc = meson.get_compiler('c')
>> @@ -409,11 +410,6 @@ if 'CONFIG_USB_LIBUSB' in config_host
>>    libusb = declare_dependency(compile_args: 
>> config_host['LIBUSB_CFLAGS'].split(),
>>                                link_args: config_host['LIBUSB_LIBS'].split())
>>  endif
>> -capstone = not_found
>> -if 'CONFIG_CAPSTONE' in config_host
>> -  capstone = declare_dependency(compile_args: 
>> config_host['CAPSTONE_CFLAGS'].split(),
>> -                                link_args: 
>> config_host['CAPSTONE_LIBS'].split())
>> -endif
>>  libpmem = not_found
>>  if 'CONFIG_LIBPMEM' in config_host
>>    libpmem = declare_dependency(compile_args: 
>> config_host['LIBPMEM_CFLAGS'].split(),
>> @@ -470,7 +466,6 @@ foreach k, v: config_host
>>      config_host_data.set(k, v == 'y' ? 1 : v)
>>    endif
>>  endforeach
>> -genh += configure_file(output: 'config-host.h', configuration: 
>> config_host_data)
>>  
>>  minikconf = find_program('scripts/minikconf.py')
>>  config_all_devices = {}
>> @@ -610,6 +605,108 @@ config_all += {
>>    'CONFIG_ALL': true,
>>  }
>>  
>> +# Submodules
>> +
>> +capstone = not_found
>> +capstone_opt = get_option('capstone')
>> +if capstone_opt == 'no'
>> +  capstone_opt = false
>> +elif capstone_opt in ['yes', 'auto', 'system']
>> +  have_internal = fs.exists('capstone/Makefile')
>> +  capstone = dependency('capstone', static: enable_static,
>> +                        required: capstone_opt == 'system' or
>> +                                  capstone_opt == 'yes' and not 
>> have_internal)
>> +  if capstone.found()
>> +    capstone_opt = 'system'
>> +  elif have_internal
>> +    capstone_opt = 'internal'
>> +  else
>> +    capstone_opt = false
>> +  endif
>> +endif
>> +if capstone_opt == 'internal'
>> +  capstone_data = configuration_data()
>> +  capstone_data.set('CAPSTONE_USE_SYS_DYN_MEM', '1')
>> +
>> +  capstone_files = files(
>> +    'capstone/cs.c',
>> +    'capstone/MCInst.c',
>> +    'capstone/MCInstrDesc.c',
>> +    'capstone/MCRegisterInfo.c',
>> +    'capstone/SStream.c',
>> +    'capstone/utils.c'
>> +  )
>> +
>> +  if 'CONFIG_ARM_DIS' in config_all_disas
>> +    capstone_data.set('CAPSTONE_HAS_ARM', '1')
>> +    capstone_files += files(
>> +      'capstone/arch/ARM/ARMDisassembler.c',
>> +      'capstone/arch/ARM/ARMInstPrinter.c',
>> +      'capstone/arch/ARM/ARMMapping.c',
>> +      'capstone/arch/ARM/ARMModule.c'
>> +    )
>> +  endif
>> +
>> +  # FIXME: This config entry currently depends on a c++ compiler.
>> +  # Which is needed for building libvixl, but not for capstone.
>> +  if 'CONFIG_ARM_A64_DIS' in config_all_disas
>> +    capstone_data.set('CAPSTONE_HAS_ARM64', '1')
>> +    capstone_files += files(
>> +      'capstone/arch/AArch64/AArch64BaseInfo.c',
>> +      'capstone/arch/AArch64/AArch64Disassembler.c',
>> +      'capstone/arch/AArch64/AArch64InstPrinter.c',
>> +      'capstone/arch/AArch64/AArch64Mapping.c',
>> +      'capstone/arch/AArch64/AArch64Module.c'
>> +    )
>> +  endif
>> +
>> +  if 'CONFIG_PPC_DIS' in config_all_disas
>> +    capstone_data.set('CAPSTONE_HAS_POWERPC', '1')
>> +    capstone_files += files(
>> +      'capstone/arch/PowerPC/PPCDisassembler.c',
>> +      'capstone/arch/PowerPC/PPCInstPrinter.c',
>> +      'capstone/arch/PowerPC/PPCMapping.c',
>> +      'capstone/arch/PowerPC/PPCModule.c'
>> +    )
>> +  endif
>> +
>> +  if 'CONFIG_I386_DIS' in config_all_disas
>> +    capstone_data.set('CAPSTONE_HAS_X86', 1)
>> +    capstone_files += files(
>> +      'capstone/arch/X86/X86Disassembler.c',
>> +      'capstone/arch/X86/X86DisassemblerDecoder.c',
>> +      'capstone/arch/X86/X86ATTInstPrinter.c',
>> +      'capstone/arch/X86/X86IntelInstPrinter.c',
>> +      'capstone/arch/X86/X86Mapping.c',
>> +      'capstone/arch/X86/X86Module.c'
>> +    )
>> +  endif
>> +
>> +  configure_file(output: 'capstone-defs.h', configuration: capstone_data)
>> +
>> +  capstone_cargs = [
>> +    # FIXME: There does not seem to be a way to completely replace the 
>> c_args
>> +    # that come from add_project_arguments() -- we can only add to them.
>> +    # So: disable all warnings with a big hammer.
>> +    '-Wno-error', '-w',
>> +
>> +    # Include all configuration defines via a header file, which will wind 
>> up
>> +    # as a dependency on the object file, and thus changes here will result
>> +    # in a rebuild.
>> +    '-include', 'capstone-defs.h'
>> +  ]
>> +
>> +  libcapstone = static_library('capstone',
>> +                               sources: capstone_files,
>> +                               c_args: capstone_cargs,
>> +                               include_directories: 'capstone/include')
>> +  capstone = declare_dependency(link_with: libcapstone,
>> +                                include_directories: 'capstone/include')
>> +endif
>> +config_host_data.set('CONFIG_CAPSTONE', capstone.found())
>> +
>> +genh += configure_file(output: 'config-host.h', configuration: 
>> config_host_data)
>> +
>>  # Generators
>>  
>>  hxtool = find_program('scripts/hxtool')
>> @@ -1512,7 +1609,7 @@ summary_info += {'vvfat support':     
>> config_host.has_key('CONFIG_VVFAT')}
>>  summary_info += {'qed support':       config_host.has_key('CONFIG_QED')}
>>  summary_info += {'parallels support': 
>> config_host.has_key('CONFIG_PARALLELS')}
>>  summary_info += {'sheepdog support':  
>> config_host.has_key('CONFIG_SHEEPDOG')}
>> -summary_info += {'capstone':          
>> config_host.has_key('CONFIG_CAPSTONE')}
>> +summary_info += {'capstone':          capstone_opt}
>>  summary_info += {'libpmem support':   config_host.has_key('CONFIG_LIBPMEM')}
>>  summary_info += {'libdaxctl support': 
>> config_host.has_key('CONFIG_LIBDAXCTL')}
>>  summary_info += {'libudev':           config_host.has_key('CONFIG_LIBUDEV')}
>> diff --git a/meson_options.txt b/meson_options.txt
>> index 543cf70043..e650a937e7 100644
>> --- a/meson_options.txt
>> +++ b/meson_options.txt
>> @@ -22,3 +22,7 @@ option('vnc_sasl', type : 'feature', value : 'auto',
>>         description: 'SASL authentication for VNC server')
>>  option('xkbcommon', type : 'feature', value : 'auto',
>>         description: 'xkbcommon support')
>> +
>> +option('capstone', type: 'combo', value: 'auto',
>> +       choices: ['no', 'yes', 'auto', 'system', 'internal'],
>> +       description: 'Whether and how to find the capstone library')


Hmm even more confusing is configure does:

  GIT submodules: ui/keycodemapdb tests/fp/berkeley-testfloat-3 
tests/fp/berkeley-softfloat-3 meson dtc capstone slirp

but also:

  capstone: system

which I can't quite help but feel is going to be confusing.

-- 
Alex Bennée