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

[Michal Prívozník]

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.

Michal