[v5,9/9] disas: Add capstone as submodule

Message ID 20171017153742.10026-10-richard.henderson@linaro.org
State New
Headers show
Series
  • Support the Capstone disassembler
Related show

Commit Message

Richard Henderson Oct. 17, 2017, 3:37 p.m.
Do not require the submodule, but use it if present (in preference
even to a system copy).  This will allow us to easily use capstone
in older systems for which a package is not available, and also
easily track bug fixes from upstream.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 Makefile    | 13 +++++++++++++
 .gitmodules |  3 +++
 capstone    |  1 +
 configure   | 13 ++++++++++++-
 4 files changed, 29 insertions(+), 1 deletion(-)
 create mode 160000 capstone

-- 
2.13.6

Comments

Gerd Hoffmann Oct. 18, 2017, 5:48 a.m. | #1
Hi,

> +if [ "$capstone_internal" = "yes" ]; then

> +  echo "config-host.h: subdir-capstone" >> $config_host_mak

> +fi


I think this isn't going to work correctly.  In case both capstone and
dtc are used we need a single line with the dependencies, i.e.

  config-host.h: subdir-dtc subdir-capstone

cheers,
  Gerd
Richard Henderson Oct. 18, 2017, 1:26 p.m. | #2
On 10/17/2017 10:48 PM, Gerd Hoffmann wrote:
>   Hi,

> 

>> +if [ "$capstone_internal" = "yes" ]; then

>> +  echo "config-host.h: subdir-capstone" >> $config_host_mak

>> +fi

> 

> I think this isn't going to work correctly.  In case both capstone and

> dtc are used we need a single line with the dependencies, i.e.

> 

>   config-host.h: subdir-dtc subdir-capstone


Why?  Without a build rule, I thought separate dependency lines accumulated.


r~
Eric Blake Oct. 18, 2017, 1:29 p.m. | #3
On 10/18/2017 12:48 AM, Gerd Hoffmann wrote:
>   Hi,

> 

>> +if [ "$capstone_internal" = "yes" ]; then

>> +  echo "config-host.h: subdir-capstone" >> $config_host_mak

>> +fi

> 

> I think this isn't going to work correctly.  In case both capstone and

> dtc are used we need a single line with the dependencies, i.e.

> 

>   config-host.h: subdir-dtc subdir-capstone


Make allows the composition of dependencies, as in:

a: b
a: c

It works as long as at most one a: line contains the rule for building
a, and all the other lines just add dependencies.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
Daniel P. Berrangé Oct. 18, 2017, 1:34 p.m. | #4
On Tue, Oct 17, 2017 at 08:37:42AM -0700, Richard Henderson wrote:
> Do not require the submodule, but use it if present (in preference

> even to a system copy).  This will allow us to easily use capstone

> in older systems for which a package is not available, and also

> easily track bug fixes from upstream.


I don't like the idea that we should blindly use the submodule
even if the host OS has it installed. This means developers working
on git will never be testing against the capstone that will actually
be used when they deploy a release build on their distro.

It also gives inconsistent behaviour wrt the way the dtc module
is handled, where we always try to use the system version and
only try the submodule if the system version is missing.

Thus I think we really ought to deal with capstone in the same
way as dtc. We could make the check a little more advanced so that
it checks for the particular capstone feature QEMU wants. That way
if someone does have an outdated capstone we'd still use the submodule.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Richard Henderson Oct. 18, 2017, 2:14 p.m. | #5
On 10/18/2017 06:34 AM, Daniel P. Berrange wrote:
> Thus I think we really ought to deal with capstone in the same

> way as dtc. We could make the check a little more advanced so that

> it checks for the particular capstone feature QEMU wants. That way

> if someone does have an outdated capstone we'd still use the submodule.


There's nothing I'm aware of besides bug fixes in between e.g. the fedora26
version of capstone and the one from git.


r~

Patch

diff --git a/Makefile b/Makefile
index 90f91e54eb..ca5eb489b0 100644
--- a/Makefile
+++ b/Makefile
@@ -383,6 +383,19 @@  subdir-dtc: .git-submodule-status dtc/libfdt dtc/tests
 dtc/%:
 	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 11e3078936..5a305c0a69 100755
--- a/configure
+++ b/configure
@@ -4412,7 +4412,15 @@  fi
 # capstone
 
 if test "$capstone" != no; then
-  if $pkg_config capstone; then
+  # have GIT checkout, so activate capstone submodule
+  if test -e "${source_path}/.git" ; then
+    git_submodules="${git_submodules} capstone"
+    capstone=yes
+    capstone_internal=yes
+    mkdir -p capstone
+    QEMU_CFLAGS="$QEMU_CFLAGS -I\$(SRC_PATH)/capstone/include"
+    LIBS="\$(BUILD_DIR)/capstone/libcapstone.a $LIBS"
+  elif $pkg_config capstone; then
     capstone=yes
     QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags capstone)"
     LIBS="$($pkg_config --libs capstone) $LIBS"
@@ -6642,6 +6650,9 @@  done # for target in $targets
 if [ "$dtc_internal" = "yes" ]; then
   echo "config-host.h: subdir-dtc" >> $config_host_mak
 fi
+if [ "$capstone_internal" = "yes" ]; then
+  echo "config-host.h: subdir-capstone" >> $config_host_mak
+fi
 
 if test "$numa" = "yes"; then
   echo "CONFIG_NUMA=y" >> $config_host_mak