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