| Message ID | 20200914000153.1725632-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | capstone + disassembler patches | expand |
On 9/13/20 5:01 PM, Richard Henderson wrote: > case "$capstone" in > - git | internal) > + git) > if test "$capstone" = git; then > git_submodules="${git_submodules} capstone" > fi The if here can be removed now. Alternately... > - 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)" > + internal | system | no) > ;; > > - no) > - ;; > *) > error_exit "Unknown state for capstone: $capstone" > ;; ... stop trying to validate the set of states and instead remove the outer case, leaving only the inner if. Thoughts? r~
On 14/09/20 02:01, Richard Henderson wrote: > > case "$capstone" in > - git | internal) > + git) > if test "$capstone" = git; 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)" > + internal | system | no) > ;; > > - no) > - ;; > *) > error_exit "Unknown state for capstone: $capstone" > ;; We can simplify it further if we move the selection logic to meson.build. Here in configure the whole capstone stanza is replaced by capstone=auto ... case "$capstone" in auto|git) # 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 test "$capstone" = git && capstone=internal ;; esac and in meson.build: capstone = not_found build_internal_capstone = false if get_option('capstone') != 'no' if get_option('capstone') != 'internal' capstone = dependency('capstone', required: get_option('capstone') == 'system', method: 'pkg-config', static: enable_static) endif build_internal_capstone = not capstone.found() endif ... if build_internal_capstone ... capstone = declare_dependency(...) endif > +option('capstone', type: 'string', value: 'no', > + description: 'capstone support') That would become: +option('capstone', type: 'combo', value: 'auto', + choices: ['auto', 'system', 'internal', 'no'], + description: 'How to find the capstone library') Thanks, Paolo
On Mon, 14 Sep 2020 at 01:04, Richard Henderson <richard.henderson@linaro.org> 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. > > In leiu, build the library via the main meson.build. "in lieu" (or you could just say "instead" :-)) thanks -- PMM
On 9/14/20 6:28 AM, Paolo Bonzini wrote: > We can simplify it further if we move the selection logic to > meson.build. Here in configure the whole capstone stanza > is replaced by > > capstone=auto > ... > case "$capstone" in > auto|git) > # 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 > test "$capstone" = git && capstone=internal > ;; > esac Do we really need to keep testing $source/.git and $git_update? Surely we can accumulate git_submodules and then do (or not do) something with that at the end without all of the tests? > and in meson.build: > > capstone = not_found > build_internal_capstone = false > if get_option('capstone') != 'no' > if get_option('capstone') != 'internal' > capstone = dependency('capstone', > required: get_option('capstone') == 'system', > method: 'pkg-config', > static: enable_static) > endif > build_internal_capstone = not capstone.found() > endif > ... > if build_internal_capstone > ... > capstone = declare_dependency(...) > endif This doesn't seem like it would do the right thing for capstone=auto, --disable-git-update, and no system library. In that case auto should resolve to no. I don't think we can move this detection to meson until the definition of CONFIG_CAPSTONE is under control of meson. > +option('capstone', type: 'combo', value: 'auto', > + choices: ['auto', 'system', 'internal', 'no'], > + description: 'How to find the capstone library') I can certainly change this. I presume this validates that the -Dcapstone=foo value passed to meson is correct? r~
Il lun 14 set 2020, 18:23 Richard Henderson <richard.henderson@linaro.org> ha scritto: > Do we really need to keep testing $source/.git and $git_update? > Surely we can accumulate git_submodules and then do (or not do) something > with > that at the end without all of the tests? > Possibly, but I don't mind going through that separately. > and in meson.build: > > > > capstone = not_found > > build_internal_capstone = false > > if get_option('capstone') != 'no' > > if get_option('capstone') != 'internal' > > capstone = dependency('capstone', > > required: get_option('capstone') == 'system', > > method: 'pkg-config', > > static: enable_static) > > endif > > build_internal_capstone = not capstone.found() > > endif > > ... > > if build_internal_capstone > > ... > > capstone = declare_dependency(...) > > endif > > This doesn't seem like it would do the right thing for capstone=auto, > --disable-git-update, and no system library. In that case auto should > resolve to no. > Indeed, that would require some filesystem check like fs = import('fs') build_internal_capstone = not capstone.found() and \ (get_option('capstone') == 'internal' or \ fs.exists('capstone/Makefile')) I don't think we can move this detection to meson until the definition of > CONFIG_CAPSTONE is under control of meson. > Yep, that's another part that needs to be moved to meson.build in this patch with config_host_data.set. But this patch is the right one to do this. Paolo > > +option('capstone', type: 'combo', value: 'auto', > > + choices: ['auto', 'system', 'internal', 'no'], > > + description: 'How to find the capstone library') > > I can certainly change this. I presume this validates that the > -Dcapstone=foo > value passed to meson is correct? > > > r~ > > <div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Il lun 14 set 2020, 18:23 Richard Henderson <<a href="mailto:richard.henderson@linaro.org" target="_blank" rel="noreferrer">richard.henderson@linaro.org</a>> ha scritto:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Do we really need to keep testing $source/.git and $git_update?<br> Surely we can accumulate git_submodules and then do (or not do) something with<br> that at the end without all of the tests?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Possibly, but I don't mind going through that separately.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> and in meson.build:<br> > <br> > capstone = not_found<br> > build_internal_capstone = false<br> > if get_option('capstone') != 'no'<br> > if get_option('capstone') != 'internal'<br> > capstone = dependency('capstone',<br> > required: get_option('capstone') == 'system',<br> > method: 'pkg-config',<br> > static: enable_static)<br> > endif<br> > build_internal_capstone = not capstone.found()<br> > endif<br> > ...<br> > if build_internal_capstone<br> > ...<br> > capstone = declare_dependency(...)<br> > endif<br> <br> This doesn't seem like it would do the right thing for capstone=auto,<br> --disable-git-update, and no system library. In that case auto should resolve to no.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Indeed, that would require some filesystem check like</div><div dir="auto"><br></div><div dir="auto">fs = import('fs')<br></div><div dir="auto">build_internal_capstone = not capstone.found() and \</div><div dir="auto"> (get_option('capstone') == 'internal' or \</div><div dir="auto"> fs.exists('capstone/Makefile'))</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I don't think we can move this detection to meson until the definition of<br> CONFIG_CAPSTONE is under control of meson.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Yep, that's another part that needs to be moved to meson.build in this patch with config_host_data.set. But this patch is the right one to do this.</div><div dir="auto"><br></div><div dir="auto">Paolo</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> > +option('capstone', type: 'combo', value: 'auto',<br> > + choices: ['auto', 'system', 'internal', 'no'],<br> > + description: 'How to find the capstone library')<br> <br> I can certainly change this. I presume this validates that the -Dcapstone=foo<br> value passed to meson is correct?<br> <br> <br> r~<br> <br> </blockquote></div></div></div>
diff --git a/configure b/configure index 2b6a1196da..4fc5c15283 100755 --- a/configure +++ b/configure @@ -5146,27 +5146,15 @@ case "$capstone" in esac case "$capstone" in - git | internal) + git) if test "$capstone" = git; 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)" + internal | system | no) ;; - no) - ;; *) error_exit "Unknown state for capstone: $capstone" ;; @@ -7290,8 +7278,6 @@ if test "$ivshmem" = "yes" ; then 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 @@ -7816,9 +7802,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 +7988,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 57f72f56c6..0746aa83e3 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..4417de1e14 100644 --- a/meson.build +++ b/meson.build @@ -415,11 +415,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(), @@ -616,6 +611,90 @@ config_all += { 'CONFIG_ALL': true, } +if get_option('capstone') == 'no' + capstone = not_found +elif get_option('capstone') == 'system' + capstone = dependency('capstone', static: enable_static, required: true) +else + 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 + # Generators hxtool = find_program('scripts/hxtool') diff --git a/meson_options.txt b/meson_options.txt index 543cf70043..99ecd44aca 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -22,3 +22,6 @@ 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: 'string', value: 'no', + description: 'capstone support')
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. In leiu, 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. Cc: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- configure | 24 +++---------- Makefile | 16 --------- meson.build | 89 ++++++++++++++++++++++++++++++++++++++++++++--- meson_options.txt | 3 ++ 4 files changed, 91 insertions(+), 41 deletions(-) -- 2.25.1