On 3/21/23 15:19, Daniel P. Berrangé wrote:
> 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.
D'oh. Spoke too early. What I did also was rewrite virNumaGetNodeOfCPU()
as follows:
int VIR_MOCKABLE
virNumaGetNodeOfCPU(int cpu)
{
errno = ENOSYS;
if (cpu < 0)
return -1;
return -1;
}
(just don't make that a ternary operator, as it stops working)
Where VIR_MOCKABLE is:
#if defined(__clang__)
# define VIR_OPTNONE __attribute__((optnone))
#else
# define VIR_OPTNONE
#endif
#if defined(__clang__)
# define VIR_NOIPA
#else
# define VIR_NOIPA __attribute__((noipa))
#endif
#define VIR_MOCKABLE VIR_OPTNONE VIR_NOIPA __attribute__((noinline))
__attribute__((used))
BUT, I also had to annotate virNumaCPUSetToNodeset() which is NOT
mocked. And this is the most annoying thing about whole approach.
Since we already have a fix and now are fighting two failed CI builds, I
wonder whether it makes sense to just forcibly disable all optimizations
IF the compiler is clang and it doesn't have -fsemantic-interposition
option.
Michal
