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