[v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

[PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Michael Tokarev 1 year, 5 months ago

The upcoming capstone 5.0 drops support for the old way
of including its header, due to this change:
https://github.com/capstone-engine/capstone/commit/6656bcb63ab4e87dc6079bd6b6b12cc8dd9b2ad8
The official way is to use <capstone/capstone.h>

This change has already been proposed before, see
https://patchwork.kernel.org/project/qemu-devel/patch/20180215173539.11033-1-f4bug@amsat.org/
but it didn't find its way into qemu at that time.

On current systems, using <capstone/capstone.h> works
now (despite the pkg-config-supplied -I/usr/include/capstone) -
since on all systems capstone headers are put into capstone/
subdirectory of a system include dir. So this change is
compatible with both the obsolete way of including it
and the only future way.

I dunno how relevant this is for 7.2, it's probably too
late already to test it on everything, but at the same
time, once capstone-5 will be released, there will be many
user questions about how to build qemu. This has already
been asked in #qemu - gentoo already have capstone-5.0-rc
and qemu fails to build with that one, but works fine
with this change.

Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
Since v1: include the forgotten-to-be-committed meson.build change

 include/disas/capstone.h | 2 +-
 meson.build              | 7 +------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/disas/capstone.h b/include/disas/capstone.h
index e29068dd97..d8fdc5d537 100644
--- a/include/disas/capstone.h
+++ b/include/disas/capstone.h
@@ -3,7 +3,7 @@
 
 #ifdef CONFIG_CAPSTONE
 
-#include <capstone.h>
+#include <capstone/capstone.h>
 
 #else
 
diff --git a/meson.build b/meson.build
index cf3e517e56..6f34c963f7 100644
--- a/meson.build
+++ b/meson.build
@@ -2680,12 +2680,7 @@ if not get_option('capstone').auto() or have_system or have_user
   capstone = dependency('capstone', version: '>=3.0.5',
                         kwargs: static_kwargs, method: 'pkg-config',
                         required: get_option('capstone'))
-
-  # Some versions of capstone have broken pkg-config file
-  # that reports a wrong -I path, causing the #include to
-  # fail later. If the system has such a broken version
-  # do not use it.
-  if capstone.found() and not cc.compiles('#include <capstone.h>',
+  if capstone.found() and not cc.compiles('#include <capstone/capstone.h>',
                                           dependencies: [capstone])
     capstone = not_found
     if get_option('capstone').enabled()
-- 
2.30.2

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Paolo Bonzini 1 year, 5 months ago

Queued, thanks.

Paolo

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Daniel P. Berrangé 1 year, 5 months ago

On Mon, Nov 14, 2022 at 02:49:39PM +0100, Paolo Bonzini wrote:
> Queued, thanks.

I presume you have unqueued this patch again after the discussion
yesterday ?

With 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 :|

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Paolo Bonzini 1 year, 5 months ago

On 11/15/22 09:11, Daniel P. Berrangé wrote:
> On Mon, Nov 14, 2022 at 02:49:39PM +0100, Paolo Bonzini wrote:
>> Queued, thanks.
> I presume you have unqueued this patch again after the discussion
> yesterday ?

Yes, of course.

Paolo

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Daniel P. Berrangé 1 year, 5 months ago

On Sun, Nov 13, 2022 at 11:09:42PM +0300, Michael Tokarev wrote:
> The upcoming capstone 5.0 drops support for the old way
> of including its header, due to this change:
> https://github.com/capstone-engine/capstone/commit/6656bcb63ab4e87dc6079bd6b6b12cc8dd9b2ad8
> The official way is to use <capstone/capstone.h>
> 
> This change has already been proposed before, see
> https://patchwork.kernel.org/project/qemu-devel/patch/20180215173539.11033-1-f4bug@amsat.org/
> but it didn't find its way into qemu at that time.
> 
> On current systems, using <capstone/capstone.h> works
> now (despite the pkg-config-supplied -I/usr/include/capstone) -
> since on all systems capstone headers are put into capstone/
> subdirectory of a system include dir. So this change is
> compatible with both the obsolete way of including it
> and the only future way.

AFAIR, macOS HomeBrew does not put anything into the system
include dir, and always requires -I flags to be correct.

> diff --git a/meson.build b/meson.build
> index cf3e517e56..6f34c963f7 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2680,12 +2680,7 @@ if not get_option('capstone').auto() or have_system or have_user
>    capstone = dependency('capstone', version: '>=3.0.5',
>                          kwargs: static_kwargs, method: 'pkg-config',
>                          required: get_option('capstone'))
> -
> -  # Some versions of capstone have broken pkg-config file
> -  # that reports a wrong -I path, causing the #include to
> -  # fail later. If the system has such a broken version
> -  # do not use it.
> -  if capstone.found() and not cc.compiles('#include <capstone.h>',
> +  if capstone.found() and not cc.compiles('#include <capstone/capstone.h>',
>                                            dependencies: [capstone])

To retain back compat this could probe for both ways

    if capstone.found()
        if cc.compiles('#include <capstone/capstone.h>',
	               dependencies: [capstone])
           ...
        else if cc.compiles('#include <capstone.h>',
	                    dependencies: [capstone])
           ...
        
then, the source file can try the correct #include based on what
we detect works here.


With 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 :|

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Michael Tokarev 1 year, 5 months ago

14.11.2022 11:58, Daniel P. Berrangé wrote:
..
>> On current systems, using <capstone/capstone.h> works
>> now (despite the pkg-config-supplied -I/usr/include/capstone) -
>> since on all systems capstone headers are put into capstone/
>> subdirectory of a system include dir. So this change is
>> compatible with both the obsolete way of including it
>> and the only future way.
> 
> AFAIR, macOS HomeBrew does not put anything into the system
> include dir, and always requires -I flags to be correct.

Does it work with the capstone-supplied --cflags and the proposed
include path?  What does pkg-config --cflags capstone return there?

..
>> -  if capstone.found() and not cc.compiles('#include <capstone.h>',
>> +  if capstone.found() and not cc.compiles('#include <capstone/capstone.h>',
>>                                             dependencies: [capstone])
> 
> To retain back compat this could probe for both ways
> 
>      if capstone.found()
>          if cc.compiles('#include <capstone/capstone.h>',
> 	               dependencies: [capstone])
>             ...
>          else if cc.compiles('#include <capstone.h>',
> 	                    dependencies: [capstone])
>             ...
>          
> then, the source file can try the correct #include based on what
> we detect works here.

I don't think this deserves the complexity really, unless there *is*
a system out there which actually needs this.

I mean, these little compat tweaks, - it becomes twisty with time,
and no one knows which code paths and config variables are needed
for what, and whole thing slowly becomes unmanageable... If it's
easy to make it unconditional, it should be done. IMHO anyway :)

Thanks!

/mjt

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Daniel P. Berrangé 1 year, 5 months ago

On Mon, Nov 14, 2022 at 12:13:48PM +0300, Michael Tokarev wrote:
> 14.11.2022 11:58, Daniel P. Berrangé wrote:
> ..
> > > On current systems, using <capstone/capstone.h> works
> > > now (despite the pkg-config-supplied -I/usr/include/capstone) -
> > > since on all systems capstone headers are put into capstone/
> > > subdirectory of a system include dir. So this change is
> > > compatible with both the obsolete way of including it
> > > and the only future way.
> > 
> > AFAIR, macOS HomeBrew does not put anything into the system
> > include dir, and always requires -I flags to be correct.
> 
> Does it work with the capstone-supplied --cflags and the proposed
> include path?  What does pkg-config --cflags capstone return there?

I see the QEMU build logs adding:

 -I/usr/local/Cellar/capstone/4.0.2/include/capstone

so  #include <capstone/capstone.h>   seems unlikely to work

> > > -  if capstone.found() and not cc.compiles('#include <capstone.h>',
> > > +  if capstone.found() and not cc.compiles('#include <capstone/capstone.h>',
> > >                                             dependencies: [capstone])
> > 
> > To retain back compat this could probe for both ways
> > 
> >      if capstone.found()
> >          if cc.compiles('#include <capstone/capstone.h>',
> > 	               dependencies: [capstone])
> >             ...
> >          else if cc.compiles('#include <capstone.h>',
> > 	                    dependencies: [capstone])
> >             ...
> > then, the source file can try the correct #include based on what
> > we detect works here.
> 
> I don't think this deserves the complexity really, unless there *is*
> a system out there which actually needs this.
> 
> I mean, these little compat tweaks, - it becomes twisty with time,
> and no one knows which code paths and config variables are needed
> for what, and whole thing slowly becomes unmanageable... If it's
> easy to make it unconditional, it should be done. IMHO anyway :)

Well you're proposing a change during RC time which is likely to
break builds if the assumption that its always in the system
include path is wrong. So I think the explicit compatibility is
required to reduce the risk of this creating a regression.

With 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 :|

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Peter Maydell 1 year, 5 months ago

On Sun, 13 Nov 2022 at 20:10, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> The upcoming capstone 5.0 drops support for the old way
> of including its header, due to this change:
> https://github.com/capstone-engine/capstone/commit/6656bcb63ab4e87dc6079bd6b6b12cc8dd9b2ad8
> The official way is to use <capstone/capstone.h>
>
> This change has already been proposed before, see
> https://patchwork.kernel.org/project/qemu-devel/patch/20180215173539.11033-1-f4bug@amsat.org/
> but it didn't find its way into qemu at that time.
>
> On current systems, using <capstone/capstone.h> works
> now (despite the pkg-config-supplied -I/usr/include/capstone) -
> since on all systems capstone headers are put into capstone/
> subdirectory of a system include dir. So this change is
> compatible with both the obsolete way of including it
> and the only future way.

That's only true if capstone happened to be installed
into a system include directory subdirectory. That
is probably true for most distros, but it isn't
necessarily true when an end user has built and
installed capstone locally themselves.

In other words, this is a breaking non-back-compatible
change by capstone upstream, which we now need to work
around somehow :-(


> diff --git a/include/disas/capstone.h b/include/disas/capstone.h
> index e29068dd97..d8fdc5d537 100644
> --- a/include/disas/capstone.h
> +++ b/include/disas/capstone.h
> @@ -3,7 +3,7 @@
>
>  #ifdef CONFIG_CAPSTONE
>
> -#include <capstone.h>
> +#include <capstone/capstone.h>
>
>  #else
>
> diff --git a/meson.build b/meson.build
> index cf3e517e56..6f34c963f7 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2680,12 +2680,7 @@ if not get_option('capstone').auto() or have_system or have_user
>    capstone = dependency('capstone', version: '>=3.0.5',
>                          kwargs: static_kwargs, method: 'pkg-config',
>                          required: get_option('capstone'))
> -
> -  # Some versions of capstone have broken pkg-config file
> -  # that reports a wrong -I path, causing the #include to
> -  # fail later. If the system has such a broken version
> -  # do not use it.
> -  if capstone.found() and not cc.compiles('#include <capstone.h>',
> +  if capstone.found() and not cc.compiles('#include <capstone/capstone.h>',
>                                            dependencies: [capstone])
>      capstone = not_found
>      if get_option('capstone').enabled()

We can do something like

config_host_data.set('HAVE_CAPSTONE_CAPSTONE_H',
cc.has_header('capstone/capstone.h', depedencies: [capstone])

to check that this capstone really does have capstone/capstone.h,
for instance.

Dan: is there a reason why in commit 8f4aea712ffc4 you wrote
the "check that capstone.h really exists" check with cc.compiles
rather than cc.has_header ?

thanks
-- PMM

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Michael Tokarev 1 year, 5 months ago

14.11.2022 14:59, Peter Maydell wrote:
..
> We can do something like
> 
> config_host_data.set('HAVE_CAPSTONE_CAPSTONE_H',
> cc.has_header('capstone/capstone.h', depedencies: [capstone])

This doesn't work, because has_header does not have "dependencies"
argument.  And without that, it doesn't take pkgconfig's --cflags
into account.

> Dan: is there a reason why in commit 8f4aea712ffc4 you wrote
> the "check that capstone.h really exists" check with cc.compiles
> rather than cc.has_header ?

Maybe that's why.

/mjt

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Peter Maydell 1 year, 5 months ago

On Tue, 15 Nov 2022 at 09:25, Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 14.11.2022 14:59, Peter Maydell wrote:
> ..
> > We can do something like
> >
> > config_host_data.set('HAVE_CAPSTONE_CAPSTONE_H',
> > cc.has_header('capstone/capstone.h', depedencies: [capstone])
>
> This doesn't work, because has_header does not have "dependencies"
> argument.

That's odd, the Meson documentation says it does:

https://mesonbuild.com/Reference-manual_returned_compiler.html#compilerhas_header

"dependencies dep | list[dep]
 Additionally dependencies required for compiling and / or linking."
and it's not marked with a "since version xxx" tag...

-- PMM

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Michael Tokarev 1 year, 5 months ago

15.11.2022 14:00, Peter Maydell wrote:
..
> https://mesonbuild.com/Reference-manual_returned_compiler.html#compilerhas_header
> 
> "dependencies dep | list[dep]
>   Additionally dependencies required for compiling and / or linking."

Oh sh**t.. I mistypoed it initially :)

It looks like I "un-learned" to do stuff right.. :(
Another version of this trivial thing is coming...

/mjt

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Peter Maydell 1 year, 5 months ago

On Tue, 15 Nov 2022 at 11:00, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 15 Nov 2022 at 09:25, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >
> > 14.11.2022 14:59, Peter Maydell wrote:
> > ..
> > > We can do something like
> > >
> > > config_host_data.set('HAVE_CAPSTONE_CAPSTONE_H',
> > > cc.has_header('capstone/capstone.h', depedencies: [capstone])
> >
> > This doesn't work, because has_header does not have "dependencies"
> > argument.
>
> That's odd, the Meson documentation says it does:
>
> https://mesonbuild.com/Reference-manual_returned_compiler.html#compilerhas_header
>
> "dependencies dep | list[dep]
>  Additionally dependencies required for compiling and / or linking."
> and it's not marked with a "since version xxx" tag...

...and we already have a few lines in meson.build that use it:

  if cc.has_header('epoxy/egl.h', dependencies: epoxy)

  if nettle.found() and not cc.has_header('nettle/xts.h', dependencies: nettle)

-- PMM

Re: [PATCH v2] capstone: use <capstone/capstone.h> instead of <capstone.h>

Posted by Daniel P. Berrangé 1 year, 5 months ago

On Mon, Nov 14, 2022 at 11:59:31AM +0000, Peter Maydell wrote:
> On Sun, 13 Nov 2022 at 20:10, Michael Tokarev <mjt@tls.msk.ru> wrote:
> >
> > The upcoming capstone 5.0 drops support for the old way
> > of including its header, due to this change:
> > https://github.com/capstone-engine/capstone/commit/6656bcb63ab4e87dc6079bd6b6b12cc8dd9b2ad8
> > The official way is to use <capstone/capstone.h>
> >
> > This change has already been proposed before, see
> > https://patchwork.kernel.org/project/qemu-devel/patch/20180215173539.11033-1-f4bug@amsat.org/
> > but it didn't find its way into qemu at that time.
> >
> > On current systems, using <capstone/capstone.h> works
> > now (despite the pkg-config-supplied -I/usr/include/capstone) -
> > since on all systems capstone headers are put into capstone/
> > subdirectory of a system include dir. So this change is
> > compatible with both the obsolete way of including it
> > and the only future way.
> 
> That's only true if capstone happened to be installed
> into a system include directory subdirectory. That
> is probably true for most distros, but it isn't
> necessarily true when an end user has built and
> installed capstone locally themselves.
> 
> In other words, this is a breaking non-back-compatible
> change by capstone upstream, which we now need to work
> around somehow :-(
> 
> 
> > diff --git a/include/disas/capstone.h b/include/disas/capstone.h
> > index e29068dd97..d8fdc5d537 100644
> > --- a/include/disas/capstone.h
> > +++ b/include/disas/capstone.h
> > @@ -3,7 +3,7 @@
> >
> >  #ifdef CONFIG_CAPSTONE
> >
> > -#include <capstone.h>
> > +#include <capstone/capstone.h>
> >
> >  #else
> >
> > diff --git a/meson.build b/meson.build
> > index cf3e517e56..6f34c963f7 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -2680,12 +2680,7 @@ if not get_option('capstone').auto() or have_system or have_user
> >    capstone = dependency('capstone', version: '>=3.0.5',
> >                          kwargs: static_kwargs, method: 'pkg-config',
> >                          required: get_option('capstone'))
> > -
> > -  # Some versions of capstone have broken pkg-config file
> > -  # that reports a wrong -I path, causing the #include to
> > -  # fail later. If the system has such a broken version
> > -  # do not use it.
> > -  if capstone.found() and not cc.compiles('#include <capstone.h>',
> > +  if capstone.found() and not cc.compiles('#include <capstone/capstone.h>',
> >                                            dependencies: [capstone])
> >      capstone = not_found
> >      if get_option('capstone').enabled()
> 
> We can do something like
> 
> config_host_data.set('HAVE_CAPSTONE_CAPSTONE_H',
> cc.has_header('capstone/capstone.h', depedencies: [capstone])
> 
> to check that this capstone really does have capstone/capstone.h,
> for instance.
> 
> Dan: is there a reason why in commit 8f4aea712ffc4 you wrote
> the "check that capstone.h really exists" check with cc.compiles
> rather than cc.has_header ?

I was probably just unaware of 'has_header' existing

With 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 :|