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

["罗勇刚(Yonggang Luo)" <luoyonggang@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 16043 bytes --]

On Mon, Sep 21, 2020 at 7:06 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> 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.
maybe remove the capstone system support is a good idea after-all.
>
> --
> Alex Bennée
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 22581 bytes --]