[Qemu-devel] [RFC PATCH] capstone: fix building using system package

[Qemu-devel] [RFC PATCH] capstone: fix building using system package

[Philippe Mathieu-Daudé]

The use of <capstone/capstone.h> is recommended by the upstream project:
  http://www.capstone-engine.org/lang_c.html
However when building the in-tree cloned submodule, the header is accessible
via <capstone.h>.

This fixes building on Gentoo (and Haiku OS - not supported since 898be3e0415):
    CC      disas.o
  /sources/qemu-2.11.0/include/disas/capstone.h:6:22: fatal error: capstone.h: No such file or directory
  #include <capstone.h>
           ^~~~~~~~~~~~

On Haiku `pkg-config --cflags capstone` reports "-I/usr/develop/headers".

Bug: https://bugs.gentoo.org/647570
Reported-by: Zoltán Mizsei <miqlas@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because this might be a Gentoo portage issue.

 configure                | 1 +
 include/disas/capstone.h | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 913e14839d..3657a61a35 100755
--- a/configure
+++ b/configure
@@ -7017,6 +7017,7 @@ if [ "$dtc_internal" = "yes" ]; then
   echo "config-host.h: subdir-dtc" >> $config_host_mak
 fi
 if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then
+  echo "CONFIG_LIBCAPSTONE_INTERNAL=y" >> $config_host_mak
   echo "config-host.h: subdir-capstone" >> $config_host_mak
 fi
 if test -n "$LIBCAPSTONE"; then
diff --git a/include/disas/capstone.h b/include/disas/capstone.h
index 84e214956d..aea9601f41 100644
--- a/include/disas/capstone.h
+++ b/include/disas/capstone.h
@@ -3,9 +3,13 @@
 
 #ifdef CONFIG_CAPSTONE
 
+#ifdef CONFIG_LIBCAPSTONE_INTERNAL
 #include <capstone.h>
-
 #else
+#include <capstone/capstone.h>
+#endif /* CONFIG_LIBCAPSTONE_INTERNAL */
+
+#else /* CONFIG_CAPSTONE */
 
 /* Just enough to allow backends to init without ifdefs.  */
 
-- 
2.16.1

Re: [Qemu-devel] [RFC PATCH] capstone: fix building using system package

[Sergei Trofimovich]

On Thu, 15 Feb 2018 14:35:39 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

>  #else
> +#include <capstone/capstone.h>

I think it's incorrect. 'pkg-config' already reports 'capstone/' path:
    $ pkg-config --cflags capstone
    -I/usr/include/capstone

    $ ls /usr/include/capstone/capstone.h
    /usr/include/capstone/capstone.h

Thus I would guess
    #include <capstone.h>
is still correct for system include path as well (contradicts the example).

qemu just needs to use 'pkg-config' to discover the include path and
libs. Maybe new capstone release has different pkgconfig setup?

-- 

  Sergei

Re: [Qemu-devel] [RFC PATCH] capstone: fix building using system package

[Philippe Mathieu-Daudé]

Hi Sergei,

On 02/15/2018 03:21 PM, Sergei Trofimovich wrote:
> On Thu, 15 Feb 2018 14:35:39 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>>  #else
>> +#include <capstone/capstone.h>
> 
> I think it's incorrect. 'pkg-config' already reports 'capstone/' path:
>     $ pkg-config --cflags capstone
>     -I/usr/include/capstone

Glad to hear feedback from a Gentoo developer!

Ok so the problem Haiku only, which we don't support anymore.

> 
>     $ ls /usr/include/capstone/capstone.h
>     /usr/include/capstone/capstone.h
> 
> Thus I would guess
>     #include <capstone.h>
> is still correct for system include path as well (contradicts the example).

My guess is the example is probabilisticly safer for people compiling
without using 'pkg-config'.

> qemu just needs to use 'pkg-config' to discover the include path and
> libs. Maybe new capstone release has different pkgconfig setup?

I think it is safer to drop this patch.

Thanks for your review!

Phil.

Re: [Qemu-devel] [RFC PATCH] capstone: fix building using system package

[miqlas]

Hi,

afaik (but not tested) pkgconfig --cflags reports /includes on linux, 
and it does the same on Haiku too.
I'm not against to change our capstone recipe, but please, if you can 
check it on Linux and report it back, as i don't want to break other 
software.
Thanks for the nice talk, guys!

--miqlas


2018-02-15 19:39 keltezéssel, Philippe Mathieu-Daudé írta:
> Hi Sergei,
>
> On 02/15/2018 03:21 PM, Sergei Trofimovich wrote:
>> On Thu, 15 Feb 2018 14:35:39 -0300
>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>>>   #else
>>> +#include <capstone/capstone.h>
>> I think it's incorrect. 'pkg-config' already reports 'capstone/' path:
>>      $ pkg-config --cflags capstone
>>      -I/usr/include/capstone
> Glad to hear feedback from a Gentoo developer!
>
> Ok so the problem Haiku only, which we don't support anymore.
>
>>      $ ls /usr/include/capstone/capstone.h
>>      /usr/include/capstone/capstone.h
>>
>> Thus I would guess
>>      #include <capstone.h>
>> is still correct for system include path as well (contradicts the example).
> My guess is the example is probabilisticly safer for people compiling
> without using 'pkg-config'.
>
>> qemu just needs to use 'pkg-config' to discover the include path and
>> libs. Maybe new capstone release has different pkgconfig setup?
> I think it is safer to drop this patch.
>
> Thanks for your review!
>
> Phil.

Re: [Qemu-devel] [RFC PATCH] capstone: fix building using system package

[Philippe Mathieu-Daudé]

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

What is interesting with this patch, is that, forcing use of system
capstone, Travis builds ran much faster; longest build took 40min:
https://travis-ci.org/philmd/qemu/builds/341979248

This revealed (without profiling yet) that compiling the capstone C++
takes some time...

mingw32@i7-4600U# time make subdir-capstone
  CC      cs.o
  CC      utils.o
  CC      SStream.o
  CC      MCInstrDesc.o
  CC      MCRegisterInfo.o
  CC      arch/ARM/ARMDisassembler.o
  CC      arch/ARM/ARMInstPrinter.o
  CC      arch/ARM/ARMMapping.o
  CC      arch/ARM/ARMModule.o
  CC      arch/AArch64/AArch64BaseInfo.o
  CC      arch/AArch64/AArch64Disassembler.o
  CC      arch/AArch64/AArch64InstPrinter.o
  CC      arch/AArch64/AArch64Mapping.o
  CC      arch/AArch64/AArch64Module.o
  CC      arch/Mips/MipsDisassembler.o
  CC      arch/Mips/MipsInstPrinter.o
  CC      arch/Mips/MipsMapping.o
  CC      arch/Mips/MipsModule.o
  CC      arch/PowerPC/PPCDisassembler.o
  CC      arch/PowerPC/PPCInstPrinter.o
  CC      arch/PowerPC/PPCMapping.o
  CC      arch/PowerPC/PPCModule.o
  CC      arch/Sparc/SparcDisassembler.o
  CC      arch/Sparc/SparcInstPrinter.o
  CC      arch/Sparc/SparcMapping.o
  CC      arch/Sparc/SparcModule.o
  CC      arch/SystemZ/SystemZDisassembler.o
  CC      arch/SystemZ/SystemZInstPrinter.o
  CC      arch/SystemZ/SystemZMapping.o
  CC      arch/SystemZ/SystemZModule.o
  CC      arch/SystemZ/SystemZMCTargetDesc.o
  CC      arch/X86/X86DisassemblerDecoder.o
  CC      arch/X86/X86Disassembler.o
  CC      arch/X86/X86IntelInstPrinter.o
  CC      arch/X86/X86ATTInstPrinter.o
  CC      arch/X86/X86Mapping.o
  CC      arch/X86/X86Module.o
  CC      arch/XCore/XCoreDisassembler.o
  CC      arch/XCore/XCoreInstPrinter.o
  CC      arch/XCore/XCoreMapping.o
  CC      arch/XCore/XCoreModule.o
  CC      MCInst.o
  AR      capstone.lib
i686-w64-mingw32.shared-ar: creating capstone/capstone.lib
make: 'subdir-capstone' is up to date.

real	0m35.391s
user	0m32.857s
sys	0m2.414s

Not that bad after all.

So I still dunno why this patch improved build time...

On 02/15/2018 02:35 PM, Philippe Mathieu-Daudé wrote:
> The use of <capstone/capstone.h> is recommended by the upstream project:
>   http://www.capstone-engine.org/lang_c.html
> However when building the in-tree cloned submodule, the header is accessible
> via <capstone.h>.
> 
> This fixes building on Gentoo (and Haiku OS - not supported since 898be3e0415):
>     CC      disas.o
>   /sources/qemu-2.11.0/include/disas/capstone.h:6:22: fatal error: capstone.h: No such file or directory
>   #include <capstone.h>
>            ^~~~~~~~~~~~
> 
> On Haiku `pkg-config --cflags capstone` reports "-I/usr/develop/headers".
> 
> Bug: https://bugs.gentoo.org/647570
> Reported-by: Zoltán Mizsei <miqlas@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because this might be a Gentoo portage issue.
> 
>  configure                | 1 +
>  include/disas/capstone.h | 6 +++++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index 913e14839d..3657a61a35 100755
> --- a/configure
> +++ b/configure
> @@ -7017,6 +7017,7 @@ if [ "$dtc_internal" = "yes" ]; then
>    echo "config-host.h: subdir-dtc" >> $config_host_mak
>  fi
>  if [ "$capstone" = "git" -o "$capstone" = "internal" ]; then
> +  echo "CONFIG_LIBCAPSTONE_INTERNAL=y" >> $config_host_mak
>    echo "config-host.h: subdir-capstone" >> $config_host_mak
>  fi
>  if test -n "$LIBCAPSTONE"; then
> diff --git a/include/disas/capstone.h b/include/disas/capstone.h
> index 84e214956d..aea9601f41 100644
> --- a/include/disas/capstone.h
> +++ b/include/disas/capstone.h
> @@ -3,9 +3,13 @@
>  
>  #ifdef CONFIG_CAPSTONE
>  
> +#ifdef CONFIG_LIBCAPSTONE_INTERNAL
>  #include <capstone.h>
> -
>  #else
> +#include <capstone/capstone.h>
> +#endif /* CONFIG_LIBCAPSTONE_INTERNAL */
> +
> +#else /* CONFIG_CAPSTONE */
>  
>  /* Just enough to allow backends to init without ifdefs.  */
>  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]