Re: [PATCH 0/2] Work around broken clang. Again.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 21, 2023 at 10:35:13AM +0100, Michal Prívozník wrote:
> On 3/20/23 15:24, Daniel P. Berrangé wrote:
> > On Mon, Mar 20, 2023 at 03:17:24PM +0100, Michal Prívozník wrote:
> >> On 3/16/23 15:51, Andrea Bolognani wrote:
> >>> On Wed, Mar 15, 2023 at 08:10:26PM +0100, Michal Privoznik wrote:
> >>>> We ran into so many clang bugs recently, that I'm strongly in favor of
> >>>> making it optional and aim on gcc only. Debugging these issues burns our
> >>>> time needlessly.
> >>>>
> >>> [...]
> >>>>
> >>>> After these patches, there is still one qemuxml2argvtest case failing,
> >>>> and it's simply because no matter how hard I try, I can't stop clang
> >>>> from optimizing the function.
> >>>>
> >>> [...]
> >>>>
> >>>> At this point, I think we can call clang broken and focus our time on
> >>>> developing libvirt. Not coming up with hacks around broken compilers.
> >>>
> >>> clang is the default compiler for FreeBSD and macOS, both of which
> >>> are officially supported platforms. Unless we are willing to change
> >>> that, ignoring clang is simply out of the question.
> >>
> >> I'm fine with doing that change. FreeBSD offers option to install gcc.
> >> And for macOS - the fact we compile there says absolutely nothing.
> >> Anybody willing to make libvirt work on macOS is more than welcome to
> >> post patches.
> >>
> >>>
> >>>
> >>> I've only looked at your patches very briefly and I'm trying to wrap
> >>> something else up before the end of the week, so unfortunately I'm
> >>> unlikely to be able to do a proper review within a reasonable
> >>> timeframe. Plus IIUC even with these patches applied we'd still have
> >>> at least one failing test case, so they're not a complete solution.
> >>
> >> Yep.
> >>
> >>>
> >>> So, in the interest of returning the CI to green as soon as possible,
> >>> I would recommend reverting 95ae91fdd4da quickly. We can then worry
> >>> about improving the situation compared to the (admittedly poor)
> >>> status quo as a follow-up, once that urgency is gone.
> >>
> >> That won't do any good as it's not the root cause of this problem. The
> >> problem is (was until Dan fixed it) that even though I've specifically
> >> marked some functions to be not optimized, clang ignored that and
> >> optimized them anyway. And in this particular case it was
> >> virNumaCPUSetToNodeset() and virNumaGetNodeOfCPU() which are new. They
> >> were not implemented in the commit you're referencing.
> >>
> >>>
> >>> A thought about VIR_OPTNONE. It seems to me that we'd want to apply
> >>> this to all the functions that currently are marked with G_NO_INLINE
> >>> for mocking purposes. So, wouldn't it make sense to instead have a
> >>> single VIR_MOCKABLE annotation that combines the two, and use that
> >>> everywhere?
> >>>
> >>
> >> The problem is, __attribute__((optnone)) works only when passed at
> >> function definition, while noinline can be passed at function
> >> definition.
> >>
> >> At any rate, this is not solved (for new enough clang, i.e. 11.0+) and
> >> for even newer clang, where completely unrelated issue is happening,
> >> I've posted another fix [1]. Which brings me back to the question from
> >> the cover letter - how many clang related problems are acceptable for
> >> us?
> > 
> > Again, this is not a clang problem. 
> 
> The -fsemantic-interposition is not documented in clang. At least not in
> its manpage or --help output. I had to go to gcc's manpage to learn what
> the option is doing.
> 
> But okay, CLang itself is not broken. But the defaults they chose are
> weird and we are investing non-negligible amount of time trying to fix
> them.
> 
> Not to mention, that apparently the optimization was enabled way before
> users were able to turn it off. Speaking of which, the option was
> initially available for x86_64 only:
> 
> https://github.com/llvm/llvm-project/commit/68a20c7f36d1d51cc46c0bd17384c16bc7818fa2
> 
> Hence, FreeBSD 12 (clang 10.0.1) is foobared. And macOS very likely did
> not pick up aforementioned commit hence it claims it doesn't have the
> option.

Argh. I was mislead by the CI pipeline being green. I just discvoered
that we had Cirrus CI mis-configured in /libvirt namespace, so it only
run post-merge to master, not pre-merge. I've now marked the variables
as non-Protected, so Cirrus CI will always run.

It confirms we're still broken in FreeBSD 12 and macOS.

> > This is a libvirt problem caused by
> > us making bogus assumptions when mocking for our unit tests. The GCC
> > maintainers have told me they consider this CLang behaviour acceptable
> > and if we want our mocks to be reliable we need more than just "noinline".
> 
> For some reason, I am not able to reproduce this issue with no
> combination of -ON and -fno-semantic-interposition/-fsemantic
> interposition with GCC. So CLang must be doing something more than GCC
> and semantic-interposition stops it.

I've not tried especially to reproduce on GCC, I just accepted the GCC
maintainers opinion that what we're doing is unsafe to rely on even
for GCC.

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux