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/68a20c7f36d1d51cc46c0bd17384c16b...
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 :|