| Message ID | 20221113200942.18882-1-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] capstone: use <capstone/capstone.h> instead of <capstone.h> | expand |
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
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
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
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
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
Queued, thanks. Paolo
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
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
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
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
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
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
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()
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(-)