The virNumaNodeIsAvailable function is stubbed out when building
without libnuma, such that it just returns a constant value. When
CLang is optimizing, it does inter-procedural analysis across
function calls. When it sees that the call to virNumaNodeIsAvailable
returns a fixed constant, it elides the conditional check for errors
in the callers such as virNumaNodesetIsAvailable.

This is a valid optimization as the C standard declares that there
must only be one implementation of each function in a binary. This
is normally the case, but ELF allows for function overrides when
linking or at runtime with LD_PRELOAD, which is technically outside
the mandated C language behaviour.

So while CLang's optimization works fine at runtime, it breaks in our
test suite which aims to mock the virNumaNodeIsAvailable function so
that it has specific semantics regardless of whether libnuma is built
or not. The return value check optimization though means our mock
override won't have the right effect. The mock will be invoked, but
its return value is not used.

Potentially the same problem could be exhibited with GCC if certain
combinations of optimizations are enabled, though thus far we've
not seen it.

To be robust on both CLang and GCC we need to make it more explicit
that we want to be able to replace functions and thus optimization
of calls must be limited. Currently we rely on 'noinline' which
does succesfully prevent inlining of the function, but it cannot
stop the eliding of checks based on the constant return value.
Thus we need a bigger hammer.

There are a couple of options to disable this optimization:

 * Annotate a symbol as 'weak'. This is tells the compiler
   that the symbol is intended to be overridable at linktime
   or runtime, and thus it will avoid doing inter-procedural
   analysis for optimizations. This was tried previously but
   have to be reverted as it had unintended consequences
   when linking .a files into our final .so, resulting in all
   the weak symbol impls being lost. See commit
   407a281a8e2b6c5078ba1148535663ea64fd9314

 * Annotate a symbol with 'noipa'. This tells the compiler
   to avoid inter-procedural analysis for calls to just this
   function. This wold be ideal match for our scenario, but
   unfortunately it is only implemented for GCC currently:

     https://reviews.llvm.org/D101011

 * The '-fsemantic-interposition' argument tells the optimizer
   that any functions may be replaced with alternative
   implementations that have different semantics. It thus
   blocks any optimizations across function calls. This is
   quite a harsh block on the optimizer, but it appears to be
   the only one that is viable with CLang.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/meson.build b/meson.build
index 319ed790f9..c35823a79a 100644
--- a/meson.build
+++ b/meson.build
@@ -404,6 +404,26 @@ cc_flags += [
   '-Wwrite-strings',
 ]
 
+if cc.get_id() == 'clang'
+    # Stop CLang from doing inter-procedural analysis of calls
+    # between functions in the same compilation unit. Such an
+    # optimization has been know to break the test suite by
+    # making assumptions that a return value is a constant.
+    # This makes it impossible to mock certain functions with
+    # replacement definitions via LD_PRELOAD that have different
+    # semantics.
+    #
+    # This is a bit of a big hammer, but alternatives don't work:
+    #
+    #  - 'weak' attribute - weak symbols get dropped from
+    #    when the .a libs are combined into the .so
+    #    see commit 407a281a8e2b6c5078ba1148535663ea64fd9314
+    #
+    #  - 'noipa' attribute - only available with GCC currently
+    #    https://reviews.llvm.org/D101011
+    cc_flags += [ '-fsemantic-interposition' ]
+endif
+
 supported_cc_flags = []
 if get_option('warning_level') == '2'
   supported_cc_flags = cc.get_supported_arguments(cc_flags)
-- 
2.39.2

On a Thursday in 2023, Daniel P. Berrangé wrote:
>The virNumaNodeIsAvailable function is stubbed out when building
>without libnuma, such that it just returns a constant value. When
>CLang is optimizing, it does inter-procedural analysis across
>function calls. When it sees that the call to virNumaNodeIsAvailable
>returns a fixed constant, it elides the conditional check for errors
>in the callers such as virNumaNodesetIsAvailable.
>

[...]

>
>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>---
> meson.build | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

On Thu, Mar 16, 2023 at 13:05:43 +0000, Daniel P. Berrangé wrote:
> The virNumaNodeIsAvailable function is stubbed out when building
> without libnuma, such that it just returns a constant value. When
> CLang is optimizing, it does inter-procedural analysis across
> function calls. When it sees that the call to virNumaNodeIsAvailable
> returns a fixed constant, it elides the conditional check for errors
> in the callers such as virNumaNodesetIsAvailable.
> 
> This is a valid optimization as the C standard declares that there
> must only be one implementation of each function in a binary. This
> is normally the case, but ELF allows for function overrides when
> linking or at runtime with LD_PRELOAD, which is technically outside
> the mandated C language behaviour.
> 
> So while CLang's optimization works fine at runtime, it breaks in our
> test suite which aims to mock the virNumaNodeIsAvailable function so
> that it has specific semantics regardless of whether libnuma is built
> or not. The return value check optimization though means our mock
> override won't have the right effect. The mock will be invoked, but
> its return value is not used.
> 
> Potentially the same problem could be exhibited with GCC if certain
> combinations of optimizations are enabled, though thus far we've
> not seen it.
> 
> To be robust on both CLang and GCC we need to make it more explicit
> that we want to be able to replace functions and thus optimization
> of calls must be limited. Currently we rely on 'noinline' which
> does succesfully prevent inlining of the function, but it cannot
> stop the eliding of checks based on the constant return value.
> Thus we need a bigger hammer.
> 
> There are a couple of options to disable this optimization:
> 
>  * Annotate a symbol as 'weak'. This is tells the compiler
>    that the symbol is intended to be overridable at linktime
>    or runtime, and thus it will avoid doing inter-procedural
>    analysis for optimizations. This was tried previously but
>    have to be reverted as it had unintended consequences
>    when linking .a files into our final .so, resulting in all
>    the weak symbol impls being lost. See commit
>    407a281a8e2b6c5078ba1148535663ea64fd9314
> 
>  * Annotate a symbol with 'noipa'. This tells the compiler
>    to avoid inter-procedural analysis for calls to just this
>    function. This wold be ideal match for our scenario, but
>    unfortunately it is only implemented for GCC currently:
> 
>      https://reviews.llvm.org/D101011
> 
>  * The '-fsemantic-interposition' argument tells the optimizer
>    that any functions may be replaced with alternative
>    implementations that have different semantics. It thus
>    blocks any optimizations across function calls. This is
>    quite a harsh block on the optimizer, but it appears to be
>    the only one that is viable with CLang.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  meson.build | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Repeating approval from the MR.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>