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

On 3/21/23 11:46, 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. > > clang is the system compiler for those platforms, and IMHO we should > be compatible with it. Just as Fedora was long unhappy with certain > apps trying to force use of clang, when Fedora's system compiler was > GCC, the opposite is reasonable for other platforms. > >>> 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. > > Is that a problem in fact ? We only need to suppress inlining when > the calling function is in the same compilation unit as the called > function. Likewise for this return value optimization. So if 'noinline' > annotation was in the definition, rather than the declaration, it > would be fine for our purposes. > > I think it would make sense to have a VIR_MOCKABLE annotation that > covered 'noline', 'optnone' and 'noipa' attributes (the precise set > varying according to what compiler we target). > > If anything this is more appealing that having it in the .h file, > since this annotations are not intended to influence callers from > external compilation units. > > We would need to update our test program that validates the > existance of noinline for mocked functions to look in the .c > instead of .h The problem we're facing is not that functions we mock are optimized, but their callers are optimized in such way that our mocking is ineffective. IOW: int mockThisFunction() { ... } int someRegularNotMockedFunction() { ... mockThisFunction(); ... } we would need to annotate someRegularNotMockedFunction() and leave mockThisFunction() be. This will get hairy pretty soon. And just for the record: the correct set of attributes that work for me are: optnone and used.