| Message ID | 20171022004621.28372-12-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Richard, On 10/21/2017 09:46 PM, Richard Henderson wrote: > Do not require the submodule, but use it if present. Allow the > command-line to override system or git submodule either way. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > Makefile | 13 +++++++++++++ > .gitmodules | 3 +++ > capstone | 1 + > configure | 60 +++++++++++++++++++++++++++++++++++++++++++++++++----------- > 4 files changed, 66 insertions(+), 11 deletions(-) > create mode 160000 capstone > > diff --git a/Makefile b/Makefile > index 9372742f86..beecc85bee 100644 > --- a/Makefile > +++ b/Makefile > @@ -383,6 +383,19 @@ subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests > 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. I'm having plenty of "missing-prototypes" warnings: arch/X86/X86IntelInstPrinter.c:354:6: warning: no previous prototype for ‘printSrcIdx8’ [-Wmissing-prototypes] void printSrcIdx8(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:361:6: warning: no previous prototype for ‘printSrcIdx16’ [-Wmissing-prototypes] void printSrcIdx16(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:368:6: warning: no previous prototype for ‘printSrcIdx32’ [-Wmissing-prototypes] void printSrcIdx32(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:375:6: warning: no previous prototype for ‘printSrcIdx64’ [-Wmissing-prototypes] void printSrcIdx64(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:382:6: warning: no previous prototype for ‘printDstIdx8’ [-Wmissing-prototypes] void printDstIdx8(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:389:6: warning: no previous prototype for ‘printDstIdx16’ [-Wmissing-prototypes] void printDstIdx16(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:396:6: warning: no previous prototype for ‘printDstIdx32’ [-Wmissing-prototypes] void printDstIdx32(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:403:6: warning: no previous prototype for ‘printDstIdx64’ [-Wmissing-prototypes] void printDstIdx64(MCInst *MI, unsigned OpNo, SStream *O) ^~~~~~~~~~~~~ arch/X86/X86IntelInstPrinter.c:494:6: warning: no previous prototype for ‘X86_Intel_printInst’ [-Wmissing-prototypes] void X86_Intel_printInst(MCInst *MI, SStream *O, void *Info) ^~~~~~~~~~~~~~~~~~~ arch/ARM/ARMModule.c:63:6: warning: no previous prototype for ‘ARM_enable’ [-Wmissing-prototypes] void ARM_enable(void) ^~~~~~~~~~ arch/AArch64/AArch64Disassembler.c:260:6: warning: no previous prototype for ‘AArch64_getInstruction’ [-Wmissing-prototypes] bool AArch64_getInstruction(csh ud, const uint8_t *code, size_t code_len, ^~~~~~~~~~~~~~~~~~~~~~ arch/AArch64/AArch64Disassembler.c:1658:6: warning: no previous prototype for ‘AArch64_init’ [-Wmissing-prototypes] void AArch64_init(MCRegisterInfo *MRI) ^~~~~~~~~~~~ arch/AArch64/AArch64Module.c:44:6: warning: no previous prototype for ‘AArch64_enable’ [-Wmissing-prototypes] void AArch64_enable(void) ^~~~~~~~~~~~~~ arch/PowerPC/PPCDisassembler.c:368:6: warning: no previous prototype for ‘PPC_getInstruction’ [-Wmissing-prototypes] bool PPC_getInstruction(csh ud, const uint8_t *code, size_t code_len, ^~~~~~~~~~~~~~~~~~ arch/PowerPC/PPCDisassembler.c:381:6: warning: no previous prototype for ‘PPC_init’ [-Wmissing-prototypes] void PPC_init(MCRegisterInfo *MRI) ^~~~~~~~ arch/PowerPC/PPCModule.c:48:6: warning: no previous prototype for ‘PPC_enable’ [-Wmissing-prototypes] void PPC_enable(void) ^~~~~~~~~~ ... > +CAP_CFLAGS = $(subst -Werror,,$(CFLAGS) $(QEMU_CFLAGS)) missing-prototypes is the only problem I currently see with capstone (not having header declaring prototypes and accessing them declared extern...) If this is enough we could use: CAP_CFLAGS = $(CFLAGS) $(QEMU_CFLAGS) -Wno-error -Wno-missing-prototypes Or if we believe upstream capstone is perfect :) CAP_CFLAGS = $(CFLAGS) $(QEMU_CFLAGS) -w Whichever you prefer: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > +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 > + > +subdir-capstone: .git-submodule-status > + $(call quiet-command,$(MAKE) -C $(SRC_PATH)/capstone CAPSTONE_SHARED=no BUILDDIR="$(BUILD_DIR)/capstone" CC="$(CC)" AR="$(AR)" LD="$(LD)" CFLAGS="$(CAP_CFLAGS)" $(SUBDIR_MAKEFLAGS) $(BUILD_DIR)/capstone/libcapstone.a) > + > $(SUBDIR_RULES): libqemuutil.a $(common-obj-y) $(chardev-obj-y) \ > $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY)) > > diff --git a/.gitmodules b/.gitmodules > index 7c981a42b6..1500579638 100644 > --- a/.gitmodules > +++ b/.gitmodules > @@ -37,3 +37,6 @@ > [submodule "ui/keycodemapdb"] > path = ui/keycodemapdb > url = git://git.qemu.org/keycodemapdb.git > +[submodule "capstone"] > + path = capstone > + url = git://git.qemu.org/capstone.git > diff --git a/capstone b/capstone > new file mode 160000 > index 0000000000..a279481dbf > --- /dev/null > +++ b/capstone > @@ -0,0 +1 @@ > +Subproject commit a279481dbfd54bb1e2336d771e89978cc6d43176 > diff --git a/configure b/configure > index 26e5ce7787..2807569f9f 100755 > --- a/configure > +++ b/configure > @@ -1299,6 +1299,10 @@ for opt do > ;; > --enable-capstone) capstone="yes" > ;; > + --enable-capstone=git) capstone="git" > + ;; > + --enable-capstone=system) capstone="system" > + ;; > *) > echo "ERROR: unknown option $opt" > echo "Try '$0 --help' for more information" > @@ -4419,18 +4423,49 @@ fi > ########################################## > # capstone > > -if test "$capstone" != no; then > - if $pkg_config capstone; then > - capstone=yes > +case "$capstone" in > + "" | yes) > + if $pkg_config capstone; then > + capstone=system > + elif test -e "${source_path}/.git" ; 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 > + git_submodules="${git_submodules} capstone" > + fi > + mkdir -p capstone > + QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include" > + LIBS="\$(BUILD_DIR)/capstone/libcapstone.a $LIBS" > + ;; > + > + system) > QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags capstone)" > LIBS="$($pkg_config --libs capstone) $LIBS" > - else > - if test "$capstone" = yes; then > - feature_not_found capstone > - fi > - capstone=no > - fi > -fi > + ;; > + > + no) > + ;; > + *) > + error_exit "Unknown state for capstone: $capstone" > + ;; > +esac > > ########################################## > # check if we have fdatasync > @@ -6165,7 +6200,7 @@ fi > if test "$ivshmem" = "yes" ; then > echo "CONFIG_IVSHMEM=y" >> $config_host_mak > fi > -if test "$capstone" = "yes" ; then > +if test "$capstone" != "no" ; then > echo "CONFIG_CAPSTONE=y" >> $config_host_mak > fi > > @@ -6650,6 +6685,9 @@ done # for target in $targets > if [ "$dtc_internal" = "yes" ]; then > echo "config-host.h: subdir-dtc" >> $config_host_mak > fi > +if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then > + echo "config-host.h: subdir-capstone" >> $config_host_mak > +fi > > if test "$numa" = "yes"; then > echo "CONFIG_NUMA=y" >> $config_host_mak >
On 10/24/2017 06:45 PM, Philippe Mathieu-Daudé wrote: > Hi Richard, > > On 10/21/2017 09:46 PM, Richard Henderson wrote: >> Do not require the submodule, but use it if present. Allow the >> command-line to override system or git submodule either way. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> Makefile | 13 +++++++++++++ >> .gitmodules | 3 +++ >> capstone | 1 + >> configure | 60 +++++++++++++++++++++++++++++++++++++++++++++++++----------- >> 4 files changed, 66 insertions(+), 11 deletions(-) >> create mode 160000 capstone >> >> diff --git a/Makefile b/Makefile >> index 9372742f86..beecc85bee 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -383,6 +383,19 @@ subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests >> 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. > > I'm having plenty of "missing-prototypes" warnings: Yes, we use lots of -Wfoo that upstream Capstone does not. I do strip -Werror, so at least it builds. Are you suggesting that I drop most of our extra -Wfoo? I suppose that's reasonable. We don't want our developers worrying about warnings coming from upstream code. If in fact you believe that most of our developers won't just install libcapstone-dev and be done? r~
Hi Richard, >> On 10/21/2017 09:46 PM, Richard Henderson wrote: >>> Do not require the submodule, but use it if present. Allow the >>> command-line to override system or git submodule either way. >>> >>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >>> --- >>> Makefile | 13 +++++++++++++ >>> .gitmodules | 3 +++ >>> capstone | 1 + >>> configure | 60 +++++++++++++++++++++++++++++++++++++++++++++++++----------- >>> 4 files changed, 66 insertions(+), 11 deletions(-) >>> create mode 160000 capstone >>> >>> diff --git a/Makefile b/Makefile >>> index 9372742f86..beecc85bee 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -383,6 +383,19 @@ subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests >>> 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. >> >> I'm having plenty of "missing-prototypes" warnings: > > Yes, we use lots of -Wfoo that upstream Capstone does not. I do strip -Werror, > so at least it builds. Are you suggesting that I drop most of our extra -Wfoo? Exactly. It's unlikely we try to modify capstone code in the submodule to silent the warnings, there is enough QEMU work to do :P I'd personally go with: CAP_CFLAGS = $(CFLAGS) $(QEMU_CFLAGS) -w At worst if there is an error while building capstone, it will still get displayed. > I suppose that's reasonable. We don't want our developers worrying about > warnings coming from upstream code. If in fact you believe that most of our > developers won't just install libcapstone-dev and be done? This was my first reflex, but then you added the git feature and I just wanted to test it. Usually if I don't need a cutting edge feature, I try to use distrib packages, to keep my environment closer to other developers. A big part of QEMU developers uses Redhat/Fedora or Debian/Ubuntu and there is an effort to verify QEMU still builds with those distribs using the Travis CI. Now there even are VMs for BSD folks :) So I'd not worry about distrib packages and these upstream warnings. Regards, Phil.
diff --git a/Makefile b/Makefile index 9372742f86..beecc85bee 100644 --- a/Makefile +++ b/Makefile @@ -383,6 +383,19 @@ subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests 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. +CAP_CFLAGS = $(subst -Werror,,$(CFLAGS) $(QEMU_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 + +subdir-capstone: .git-submodule-status + $(call quiet-command,$(MAKE) -C $(SRC_PATH)/capstone CAPSTONE_SHARED=no BUILDDIR="$(BUILD_DIR)/capstone" CC="$(CC)" AR="$(AR)" LD="$(LD)" CFLAGS="$(CAP_CFLAGS)" $(SUBDIR_MAKEFLAGS) $(BUILD_DIR)/capstone/libcapstone.a) + $(SUBDIR_RULES): libqemuutil.a $(common-obj-y) $(chardev-obj-y) \ $(qom-obj-y) $(crypto-aes-obj-$(CONFIG_USER_ONLY)) diff --git a/.gitmodules b/.gitmodules index 7c981a42b6..1500579638 100644 --- a/.gitmodules +++ b/.gitmodules @@ -37,3 +37,6 @@ [submodule "ui/keycodemapdb"] path = ui/keycodemapdb url = git://git.qemu.org/keycodemapdb.git +[submodule "capstone"] + path = capstone + url = git://git.qemu.org/capstone.git diff --git a/capstone b/capstone new file mode 160000 index 0000000000..a279481dbf --- /dev/null +++ b/capstone @@ -0,0 +1 @@ +Subproject commit a279481dbfd54bb1e2336d771e89978cc6d43176 diff --git a/configure b/configure index 26e5ce7787..2807569f9f 100755 --- a/configure +++ b/configure @@ -1299,6 +1299,10 @@ for opt do ;; --enable-capstone) capstone="yes" ;; + --enable-capstone=git) capstone="git" + ;; + --enable-capstone=system) capstone="system" + ;; *) echo "ERROR: unknown option $opt" echo "Try '$0 --help' for more information" @@ -4419,18 +4423,49 @@ fi ########################################## # capstone -if test "$capstone" != no; then - if $pkg_config capstone; then - capstone=yes +case "$capstone" in + "" | yes) + if $pkg_config capstone; then + capstone=system + elif test -e "${source_path}/.git" ; 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 + git_submodules="${git_submodules} capstone" + fi + mkdir -p capstone + QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include" + LIBS="\$(BUILD_DIR)/capstone/libcapstone.a $LIBS" + ;; + + system) QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags capstone)" LIBS="$($pkg_config --libs capstone) $LIBS" - else - if test "$capstone" = yes; then - feature_not_found capstone - fi - capstone=no - fi -fi + ;; + + no) + ;; + *) + error_exit "Unknown state for capstone: $capstone" + ;; +esac ########################################## # check if we have fdatasync @@ -6165,7 +6200,7 @@ fi if test "$ivshmem" = "yes" ; then echo "CONFIG_IVSHMEM=y" >> $config_host_mak fi -if test "$capstone" = "yes" ; then +if test "$capstone" != "no" ; then echo "CONFIG_CAPSTONE=y" >> $config_host_mak fi @@ -6650,6 +6685,9 @@ done # for target in $targets if [ "$dtc_internal" = "yes" ]; then echo "config-host.h: subdir-dtc" >> $config_host_mak fi +if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then + echo "config-host.h: subdir-capstone" >> $config_host_mak +fi if test "$numa" = "yes"; then echo "CONFIG_NUMA=y" >> $config_host_mak
Do not require the submodule, but use it if present. Allow the command-line to override system or git submodule either way. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- Makefile | 13 +++++++++++++ .gitmodules | 3 +++ capstone | 1 + configure | 60 +++++++++++++++++++++++++++++++++++++++++++++++++----------- 4 files changed, 66 insertions(+), 11 deletions(-) create mode 160000 capstone