diff mbox series

[v2,01/10] capstone: Convert Makefile bits to meson bits

Message ID 20200914230210.2185860-2-richard.henderson@linaro.org
State New
Headers show
Series capstone + disassembler patches | expand

Commit Message

Richard Henderson Sept. 14, 2020, 11:02 p.m. UTC 
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 confuration header file
instead of command-line -DEFINES means that we will rebuild the
capstone objects with changes to meson.build.

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.
---
 configure         |  61 +++-----------------------
 Makefile          |  16 -------
 meson.build       | 109 +++++++++++++++++++++++++++++++++++++++++++---
 meson_options.txt |   4 ++
 4 files changed, 113 insertions(+), 77 deletions(-)

Comments

Richard Henderson Sept. 14, 2020, 11:06 p.m. UTC | #1 
On 9/14/20 4:02 PM, Richard Henderson wrote:
> +option('capstone', type: 'combo', value: 'no',
> +       choices: ['no', 'yes', 'auto', 'system', 'internal'],
> +       description: 'Whether and how to find the capstone library')

Dangit, meant to change value: back to 'auto'.


r~
Thomas Huth Sept. 15, 2020, 6:10 a.m. UTC | #2 
On 15/09/2020 01.02, Richard Henderson wrote:
> 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 confuration header file

s/confuration/configuration/

 Thomas
Paolo Bonzini Sept. 15, 2020, 6:27 a.m. UTC | #3 
Looks good. Can you just add a "# Submodules" heading above the test?

I would also like to remove the "yes" value (that is, the default fails if
the internal copy is not there) but it can be done later for all submodules.

Paolo

Il mar 15 set 2020, 01:06 Richard Henderson <richard.henderson@linaro.org>
ha scritto:

> On 9/14/20 4:02 PM, Richard Henderson wrote:
> > +option('capstone', type: 'combo', value: 'no',
> > +       choices: ['no', 'yes', 'auto', 'system', 'internal'],
> > +       description: 'Whether and how to find the capstone library')
>
> Dangit, meant to change value: back to 'auto'.
>
>
> r~
>
>
Richard Henderson Sept. 15, 2020, 2:27 p.m. UTC | #4 
On 9/14/20 11:27 PM, Paolo Bonzini wrote:
> Looks good. Can you just add a "# Submodules" heading above the test?
> 
> I would also like to remove the "yes" value (that is, the default fails if the
> internal copy is not there) but it can be done later for all submodules.

Unless you simply plan to rename {no, yes} to {disabled, enabled}, as for the
Feature objects, why?

That seems to be the only sensible value for --enable-foo, without the =system
or =git specifiers.  We *should* fail if no system library nor internal copy is
present.


r~
Yonggang Luo Sept. 15, 2020, 4:12 p.m. UTC | #5 
On Tue, Sep 15, 2020 at 10:27 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/14/20 11:27 PM, Paolo Bonzini wrote:
> > Looks good. Can you just add a "# Submodules" heading above the test?
> >
> > I would also like to remove the "yes" value (that is, the default fails
> if the
> > internal copy is not there) but it can be done later for all submodules.
>
> Unless you simply plan to rename {no, yes} to {disabled, enabled}, as for
> the
> Feature objects, why?
>
> That seems to be the only sensible value for --enable-foo, without the
> =system
> or =git specifiers.  We *should* fail if no system library nor internal
> copy is
> present.
>
I suggest remove the capstone=system option cause the system library may
not satisfy the requirements of qemu
and create in-consistence expereince when bug or error happens about
capstone. We either have git submodule capstone
or nothing at all

>
>
> r~
>
Paolo Bonzini Sept. 15, 2020, 4:58 p.m. UTC | #6 
On 15/09/20 16:27, Richard Henderson wrote:
> On 9/14/20 11:27 PM, Paolo Bonzini wrote:
>> Looks good. Can you just add a "# Submodules" heading above the test?
>>
>> I would also like to remove the "yes" value (that is, the default fails if the
>> internal copy is not there) but it can be done later for all submodules.
> 
> Unless you simply plan to rename {no, yes} to {disabled, enabled}, as for the
> Feature objects, why?
> 
> That seems to be the only sensible value for --enable-foo, without the =system
> or =git specifiers.  We *should* fail if no system library nor internal copy is
> present.

Yes, that was a bit concise.  I would like "auto" to take the meaning
that "yes" currently as.  Right now we have

no -> Easy :)
system -> System capstone if found, fail otherwise
internal/git -> Compile capstone if found, fail otherwise
auto -> System capstone, then internal, then disable
yes -> System capstone, then internal, then fail

I'm not sure of the usefulness of disabling a dependency because we
don't have it checked out, since we have the machinery to do the
checkout automatically.  So that would become:

no -> Easy :)
system -> System capstone if found, fail otherwise
internal/git -> Compile capstone if found, fail otherwise
auto -> System capstone, then internal, then fail

The disadvantage is that it would be different from other "auto"
symbols, which never fail.  But then those other "auto" symbols do not
have a built-in fallback, so the question is whether the combination of

1) building from a fresh git checkout
2) --disable-git-update
3) not having a system capstone/libfdt/slirp
4) not having --disable-{capstone,libfdt,slirp} on the command line

is more likely to be intentional or operator error.

Paolo
Paolo Bonzini Sept. 15, 2020, 4:59 p.m. UTC | #7 
On 15/09/20 18:12, 罗勇刚(Yonggang Luo) wrote:
> 
> I suggest remove the capstone=system option cause the system library
> may not satisfy the requirements of qemu and create in-consistence
> expereince when bug or error happens about capstone. We either have
> git submodule capstone or nothing at all

Linux distributions generally do not want to have bundled libraries, so
the fallback to the system library is the default.  We single out
capstone, libfdt and slirp because they are slightly less common
dependencies and are missing on some distros; however, in general we
strive to _only_ use system libraries and not package any of QEMU's
dependencies.

Paolo
Yonggang Luo Sept. 15, 2020, 5:07 p.m. UTC | #8 
On Wed, Sep 16, 2020 at 1:00 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/09/20 18:12, 罗勇刚(Yonggang Luo) wrote:
> >
> > I suggest remove the capstone=system option cause the system library
> > may not satisfy the requirements of qemu and create in-consistence
> > expereince when bug or error happens about capstone. We either have
> > git submodule capstone or nothing at all
>
> Linux distributions generally do not want to have bundled libraries, so
>
Yes, bundled libraries is a bad idea, but static linked library is another
case, that won't affect
the Linux distributions.

> the fallback to the system library is the default.  We single out
> capstone, libfdt and slirp because they are slightly less common
>
Ineed, I would like suggest these three libraries always to be static
linked or not use it at all.

> dependencies and are missing on some distros; however, in general we
> strive to _only_ use system libraries and not package any of QEMU's
> dependencies.
>
> Paolo
>
>
Paolo Bonzini Sept. 15, 2020, 5:14 p.m. UTC | #9 
On 15/09/20 19:07, 罗勇刚(Yonggang Luo) wrote:
> 
>     Linux distributions generally do not want to have bundled libraries, so
> 
> Yes, bundled libraries is a bad idea, but static linked library is
> another case, that won't affect
> the Linux distributions. 

As far as Linux distributions are concerned, static linking is a case of
bundling.  Bundling means that, for example, any security issue will
require rebuilding the package that does the bundling (this applies
especially to slirp among the three that QEMU we bundles).

Paolo
Yonggang Luo Sept. 15, 2020, 5:20 p.m. UTC | #10 
On Wed, Sep 16, 2020 at 1:14 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 15/09/20 19:07, 罗勇刚(Yonggang Luo) wrote:
> >
> >     Linux distributions generally do not want to have bundled
libraries, so
> >
> > Yes, bundled libraries is a bad idea, but static linked library is
> > another case, that won't affect
> > the Linux distributions.
>
> As far as Linux distributions are concerned, static linking is a case of
> bundling.  Bundling means that, for example, any security issue will
> require rebuilding the package that does the bundling (this applies
> especially to slirp among the three that QEMU we bundles).

Oh, see that, so capstone and  libfdt doesn't have the security concern?
>
>
> Paolo
>


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

Patch

diff --git a/configure b/configure
index ce27eafb0a..1fd80ed699 100755
--- a/configure
+++ b/configure
@@ -469,7 +469,7 @@  opengl=""
 opengl_dmabuf="no"
 cpuid_h="no"
 avx2_opt=""
-capstone=""
+capstone="auto"
 lzo=""
 snappy=""
 bzip2=""
@@ -1573,7 +1573,7 @@  for opt do
   ;;
   --enable-capstone) capstone="yes"
   ;;
-  --enable-capstone=git) capstone="git"
+  --enable-capstone=git) capstone="internal"
   ;;
   --enable-capstone=system) capstone="system"
   ;;
@@ -5124,51 +5124,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
 
@@ -7288,11 +7248,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
@@ -7816,9 +7771,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
@@ -8005,7 +7957,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 690723b470..df7d2eb52f 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')
@@ -415,11 +416,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(),
@@ -476,7 +472,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 = {}
@@ -616,6 +611,106 @@  config_all += {
   'CONFIG_ALL': true,
 }
 
+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')
@@ -1518,7 +1613,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..f6a1b8ad21 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: 'no',
+       choices: ['no', 'yes', 'auto', 'system', 'internal'],
+       description: 'Whether and how to find the capstone library')