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

On Tue, Mar 21, 2023 at 03:13:02PM +0100, Michal Prívozník wrote: > 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. Hmm, i tried adding optnone to both, and it didn't work for me.