[PATCH] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition

Michal Privoznik posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/adde1418118dc0d6327c6d2125f1c9febebdc22e.1679411265.git.mprivozn@redhat.com
meson.build | 7 +++++++
1 file changed, 7 insertions(+)
[PATCH] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Michal Privoznik 1 year, 1 month ago
For some older CLang (or in combination with !intel arch) the
-fsemantic-interposition might be not present. But in that case,
we still want our mocking in test suite to work properly. Disable
optimizations as that seems to be the only way.

Also, don't forget to drop this when FreeBSD 11 and macOS 12 are
dropped.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

This still doesn't work when somebody overrides CFLAGS as those are
placed AFTER our supported_cc_flags:

$ export CFLAGS="-O2 -ggdb"
$ meson setup _build
$ ninja -v -C _build/
  [1/1202] clang ... -O2 -g ... -O0 -O2 -ggdb -fPIC ... -c ../src/util/glibcompat.c

 meson.build | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/meson.build b/meson.build
index a0682e8d0b..249505d474 100644
--- a/meson.build
+++ b/meson.build
@@ -492,6 +492,13 @@ if get_option('warning_level') == '2'
   endif
 
 endif
+
+if cc.get_id() == 'clang' and not supported_cc_flags.contains('-fsemantic-interposition')
+  # If CLang is old enough to not support -fsemantic-interposition
+  # then this is our best bet to make mocking in tests working properly.
+  supported_cc_flags += [ '-O0' ]
+endif
+
 add_project_arguments(supported_cc_flags, language: 'c')
 
 if cc.has_argument('-Wsuggest-attribute=format')
-- 
2.39.2
Re: [PATCH] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Daniel P. Berrangé 1 year, 1 month ago
On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
> For some older CLang (or in combination with !intel arch) the
> -fsemantic-interposition might be not present. But in that case,
> we still want our mocking in test suite to work properly. Disable
> optimizations as that seems to be the only way.
> 
> Also, don't forget to drop this when FreeBSD 11 and macOS 12 are
> dropped.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> This still doesn't work when somebody overrides CFLAGS as those are
> placed AFTER our supported_cc_flags:
> 
> $ export CFLAGS="-O2 -ggdb"
> $ meson setup _build
> $ ninja -v -C _build/
>   [1/1202] clang ... -O2 -g ... -O0 -O2 -ggdb -fPIC ... -c ../src/util/glibcompat.c
> 
>  meson.build | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/meson.build b/meson.build
> index a0682e8d0b..249505d474 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -492,6 +492,13 @@ if get_option('warning_level') == '2'
>    endif
>  
>  endif
> +
> +if cc.get_id() == 'clang' and not supported_cc_flags.contains('-fsemantic-interposition')
> +  # If CLang is old enough to not support -fsemantic-interposition
> +  # then this is our best bet to make mocking in tests working properly.
> +  supported_cc_flags += [ '-O0' ]
> +endif

I don't like the idea of forcing -O0 for the production builds, just to
work around the problem of our broken tests. Can we approach it from the
opposite POV and disable building of tests, if we see meson optimization
level is > 0

eg something roughly like this:

  with_tests = true
  if cc.get_id() == 'clang' and
     not supported_cc_flags.contains('-fsemantic-interposition') and
     get_option('optimization') != 0
     with_tests = false
  endif

  if with_tests
    subdir('tests')
  endif


So people can choose to have tests work or not


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] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Michal Prívozník 1 year, 1 month ago
On 3/21/23 16:25, Daniel P. Berrangé wrote:
> On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
>> <snip/>
> 
> I don't like the idea of forcing -O0 for the production builds, just to
> work around the problem of our broken tests. Can we approach it from the
> opposite POV and disable building of tests, if we see meson optimization
> level is > 0
> 
> eg something roughly like this:
> 
>   with_tests = true
>   if cc.get_id() == 'clang' and
>      not supported_cc_flags.contains('-fsemantic-interposition') and
>      get_option('optimization') != 0
>      with_tests = false
>   endif
> 
>   if with_tests
>     subdir('tests')
>   endif
> 
> 
> So people can choose to have tests work or not

That could work too, yeah. My reasoning for going with -O0 was that it's
very rare that somebody would use such old CLang, but I guess disabling
tests is less invasive. I'll send v2 shortly.

Michal

Re: [PATCH] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Daniel P. Berrangé 1 year, 1 month ago
On Tue, Mar 21, 2023 at 04:31:00PM +0100, Michal Prívozník wrote:
> On 3/21/23 16:25, Daniel P. Berrangé wrote:
> > On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
> >> <snip/>
> > 
> > I don't like the idea of forcing -O0 for the production builds, just to
> > work around the problem of our broken tests. Can we approach it from the
> > opposite POV and disable building of tests, if we see meson optimization
> > level is > 0
> > 
> > eg something roughly like this:
> > 
> >   with_tests = true
> >   if cc.get_id() == 'clang' and
> >      not supported_cc_flags.contains('-fsemantic-interposition') and
> >      get_option('optimization') != 0
> >      with_tests = false
> >   endif
> > 
> >   if with_tests
> >     subdir('tests')
> >   endif
> > 
> > 
> > So people can choose to have tests work or not
> 
> That could work too, yeah. My reasoning for going with -O0 was that it's
> very rare that somebody would use such old CLang, but I guess disabling
> tests is less invasive. I'll send v2 shortly.

It isn't just old Clang. Latest clang lacks -fsemantic-interposition
on non-x86_64, which means current macOS M1 platform today. For our
CI, I guess we'll want to request non-optimized builds for macOS
and FreeBSD 12, so we still run unit tests.

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] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Pavel Hrdina 1 year, 1 month ago
On Tue, Mar 21, 2023 at 03:33:24PM +0000, Daniel P. Berrangé wrote:
> On Tue, Mar 21, 2023 at 04:31:00PM +0100, Michal Prívozník wrote:
> > On 3/21/23 16:25, Daniel P. Berrangé wrote:
> > > On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
> > >> <snip/>
> > > 
> > > I don't like the idea of forcing -O0 for the production builds, just to
> > > work around the problem of our broken tests. Can we approach it from the
> > > opposite POV and disable building of tests, if we see meson optimization
> > > level is > 0
> > > 
> > > eg something roughly like this:
> > > 
> > >   with_tests = true
> > >   if cc.get_id() == 'clang' and
> > >      not supported_cc_flags.contains('-fsemantic-interposition') and
> > >      get_option('optimization') != 0
> > >      with_tests = false
> > >   endif
> > > 
> > >   if with_tests
> > >     subdir('tests')
> > >   endif
> > > 
> > > 
> > > So people can choose to have tests work or not
> > 
> > That could work too, yeah. My reasoning for going with -O0 was that it's
> > very rare that somebody would use such old CLang, but I guess disabling
> > tests is less invasive. I'll send v2 shortly.
> 
> It isn't just old Clang. Latest clang lacks -fsemantic-interposition
> on non-x86_64, which means current macOS M1 platform today. For our
> CI, I guess we'll want to request non-optimized builds for macOS
> and FreeBSD 12, so we still run unit tests.

We can also utilize the meson 'buildtype' option [1] to figure out if we
need to disable/enable tests or modify the optimization that we use when
building.

When building from git the default value is 'debugoptimized' but when
building using RPM it changes to 'plain'. Not sure what FreeBSD and
macOS are using.

The 'buildtype' affects what value will be stored in 'debug' and
'optimization' options. That could allow us to run tests from git builds
where we could disable optimizations and disable tests when libvirt is
compiled using package manager.

Pavel

[1] <https://mesonbuild.com/Builtin-options.html#details-for-buildtype>
Re: [PATCH] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Michal Prívozník 1 year, 1 month ago
On 3/21/23 17:04, Pavel Hrdina wrote:
> On Tue, Mar 21, 2023 at 03:33:24PM +0000, Daniel P. Berrangé wrote:
>> On Tue, Mar 21, 2023 at 04:31:00PM +0100, Michal Prívozník wrote:
>>> On 3/21/23 16:25, Daniel P. Berrangé wrote:
>>>> On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
>>>>> <snip/>
>>>>
>>>> I don't like the idea of forcing -O0 for the production builds, just to
>>>> work around the problem of our broken tests. Can we approach it from the
>>>> opposite POV and disable building of tests, if we see meson optimization
>>>> level is > 0
>>>>
>>>> eg something roughly like this:
>>>>
>>>>   with_tests = true
>>>>   if cc.get_id() == 'clang' and
>>>>      not supported_cc_flags.contains('-fsemantic-interposition') and
>>>>      get_option('optimization') != 0
>>>>      with_tests = false
>>>>   endif
>>>>
>>>>   if with_tests
>>>>     subdir('tests')
>>>>   endif
>>>>
>>>>
>>>> So people can choose to have tests work or not
>>>
>>> That could work too, yeah. My reasoning for going with -O0 was that it's
>>> very rare that somebody would use such old CLang, but I guess disabling
>>> tests is less invasive. I'll send v2 shortly.
>>
>> It isn't just old Clang. Latest clang lacks -fsemantic-interposition
>> on non-x86_64, which means current macOS M1 platform today. For our
>> CI, I guess we'll want to request non-optimized builds for macOS
>> and FreeBSD 12, so we still run unit tests.
> 
> We can also utilize the meson 'buildtype' option [1] to figure out if we
> need to disable/enable tests or modify the optimization that we use when
> building.
> 
> When building from git the default value is 'debugoptimized' but when
> building using RPM it changes to 'plain'. Not sure what FreeBSD and
> macOS are using.
> 
> The 'buildtype' affects what value will be stored in 'debug' and
> 'optimization' options. That could allow us to run tests from git builds
> where we could disable optimizations and disable tests when libvirt is
> compiled using package manager.

To summarize what we want:

if CLang doesn't support -fsemantic-interposition:
  if building from a git checkout:
    enforce -O0
  else
    disable tests

What worries me about this approach is that some distros might leave
.git/ dir in the checkout (e.g. live ebuilds from Gentoo) in which case
we detect the build as from git. But such distros surely pass
--buildtype=...

Now, we need to distinguish two scenarios: when the default value from
the beginning of the meson.build file was used (and thus we can enforce
-O0), OR when --buildtype=debugoptimized (or any other option that sets
optimization level) was passed on the cmd line (in which case we need to
disable tests). But I don't think there's a way to detect that.

But at the same time, disabling optimization whenever .git/ dir is found
feels very, very wrong.

Michal

Re: [PATCH] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Daniel P. Berrangé 1 year, 1 month ago
On Tue, Mar 21, 2023 at 05:34:45PM +0100, Michal Prívozník wrote:
> On 3/21/23 17:04, Pavel Hrdina wrote:
> > On Tue, Mar 21, 2023 at 03:33:24PM +0000, Daniel P. Berrangé wrote:
> >> On Tue, Mar 21, 2023 at 04:31:00PM +0100, Michal Prívozník wrote:
> >>> On 3/21/23 16:25, Daniel P. Berrangé wrote:
> >>>> On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
> >>>>> <snip/>
> >>>>
> >>>> I don't like the idea of forcing -O0 for the production builds, just to
> >>>> work around the problem of our broken tests. Can we approach it from the
> >>>> opposite POV and disable building of tests, if we see meson optimization
> >>>> level is > 0
> >>>>
> >>>> eg something roughly like this:
> >>>>
> >>>>   with_tests = true
> >>>>   if cc.get_id() == 'clang' and
> >>>>      not supported_cc_flags.contains('-fsemantic-interposition') and
> >>>>      get_option('optimization') != 0
> >>>>      with_tests = false
> >>>>   endif
> >>>>
> >>>>   if with_tests
> >>>>     subdir('tests')
> >>>>   endif
> >>>>
> >>>>
> >>>> So people can choose to have tests work or not
> >>>
> >>> That could work too, yeah. My reasoning for going with -O0 was that it's
> >>> very rare that somebody would use such old CLang, but I guess disabling
> >>> tests is less invasive. I'll send v2 shortly.
> >>
> >> It isn't just old Clang. Latest clang lacks -fsemantic-interposition
> >> on non-x86_64, which means current macOS M1 platform today. For our
> >> CI, I guess we'll want to request non-optimized builds for macOS
> >> and FreeBSD 12, so we still run unit tests.
> > 
> > We can also utilize the meson 'buildtype' option [1] to figure out if we
> > need to disable/enable tests or modify the optimization that we use when
> > building.
> > 
> > When building from git the default value is 'debugoptimized' but when
> > building using RPM it changes to 'plain'. Not sure what FreeBSD and
> > macOS are using.
> > 
> > The 'buildtype' affects what value will be stored in 'debug' and
> > 'optimization' options. That could allow us to run tests from git builds
> > where we could disable optimizations and disable tests when libvirt is
> > compiled using package manager.
> 
> To summarize what we want:
> 
> if CLang doesn't support -fsemantic-interposition:
>   if building from a git checkout:
>     enforce -O0
>   else
>     disable tests
> 
> What worries me about this approach is that some distros might leave
> .git/ dir in the checkout (e.g. live ebuilds from Gentoo) in which case
> we detect the build as from git. But such distros surely pass
> --buildtype=...
> 
> Now, we need to distinguish two scenarios: when the default value from
> the beginning of the meson.build file was used (and thus we can enforce
> -O0), OR when --buildtype=debugoptimized (or any other option that sets
> optimization level) was passed on the cmd line (in which case we need to
> disable tests). But I don't think there's a way to detect that.

I don't believe we need to check 'buildtype'. A get_option("optimization")
call should reflect the results of the 'buildtype' option IIRC>

> But at the same time, disabling optimization whenever .git/ dir is found
> feels very, very wrong.

Yeah, I wouldn't enforce -O0, just disable tests, and print a message
at end of meson execution to use 'buildtype' to turn off optimization
or use a newer clang, if they want to re-enable tests.

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] meson: Disable optimizations if CLang doesn't support -fsemantic-interposition
Posted by Daniel P. Berrangé 1 year, 1 month ago
On Tue, Mar 21, 2023 at 05:04:20PM +0100, Pavel Hrdina wrote:
> On Tue, Mar 21, 2023 at 03:33:24PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Mar 21, 2023 at 04:31:00PM +0100, Michal Prívozník wrote:
> > > On 3/21/23 16:25, Daniel P. Berrangé wrote:
> > > > On Tue, Mar 21, 2023 at 04:11:33PM +0100, Michal Privoznik wrote:
> > > >> <snip/>
> > > > 
> > > > I don't like the idea of forcing -O0 for the production builds, just to
> > > > work around the problem of our broken tests. Can we approach it from the
> > > > opposite POV and disable building of tests, if we see meson optimization
> > > > level is > 0
> > > > 
> > > > eg something roughly like this:
> > > > 
> > > >   with_tests = true
> > > >   if cc.get_id() == 'clang' and
> > > >      not supported_cc_flags.contains('-fsemantic-interposition') and
> > > >      get_option('optimization') != 0
> > > >      with_tests = false
> > > >   endif
> > > > 
> > > >   if with_tests
> > > >     subdir('tests')
> > > >   endif
> > > > 
> > > > 
> > > > So people can choose to have tests work or not
> > > 
> > > That could work too, yeah. My reasoning for going with -O0 was that it's
> > > very rare that somebody would use such old CLang, but I guess disabling
> > > tests is less invasive. I'll send v2 shortly.
> > 
> > It isn't just old Clang. Latest clang lacks -fsemantic-interposition
> > on non-x86_64, which means current macOS M1 platform today. For our
> > CI, I guess we'll want to request non-optimized builds for macOS
> > and FreeBSD 12, so we still run unit tests.
> 
> We can also utilize the meson 'buildtype' option [1] to figure out if we
> need to disable/enable tests or modify the optimization that we use when
> building.

IIUC, we shouldn't look at 'buildtype' directly, as that's just an
alias that expands to 3 other options - optimization, warning_level
and debug.

> When building from git the default value is 'debugoptimized' but when
> building using RPM it changes to 'plain'. Not sure what FreeBSD and
> macOS are using.
> 
> The 'buildtype' affects what value will be stored in 'debug' and
> 'optimization' options. That could allow us to run tests from git builds
> where we could disable optimizations and disable tests when libvirt is
> compiled using package manager.

Right, we do that in libvirt-dbus:

  https://gitlab.com/libvirt/libvirt-dbus/-/blob/master/meson.build#L220


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