Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 19/11/23 21:31, Daniel Hoffman wrote:
> `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> used to remove references to undefined symbols at the compile stage.
> Some build configurations with some compilers don't attempt to
> simplify this logic down in some cases (the pattern appears to be
> that the literal false must be the first term) and this was causing
> some builds to emit references to undefined symbols.
> 
> An example of such a configuration is clang 16.0.6 with the following
> configure: ./configure --enable-debug --without-default-features
> --target-list=x86_64-softmmu --enable-tcg-interpreter

Is the '--enable-debug' option triggering this?

I'm surprised the order of conditions matters for code elision...

> Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> ---
>   hw/i386/x86.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b3d054889bb..2b6291ad8d5 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -131,8 +131,12 @@ void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version)
>       /*
>        * Can we support APIC ID 255 or higher?  With KVM, that requires
>        * both in-kernel lapic and X2APIC userspace API.
> +     *
> +     * kvm_enabled() must go first to ensure that kvm_* references are
> +     * not emitted for the linker to consume (kvm_enabled() is
> +     * a literal `0` in configurations where kvm_* aren't defined)
>        */
> -    if (x86ms->apic_id_limit > 255 && kvm_enabled() &&
> +    if (kvm_enabled() && x86ms->apic_id_limit > 255 &&
>           (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
>           error_report("current -smp configuration requires kernel "
>                        "irqchip and X2APIC API support.");
> @@ -418,8 +422,13 @@ void x86_cpu_pre_plug(HotplugHandler *hotplug_dev,
>       }
>       cpu->thread_id = topo_ids.smt_id;
>   
> -    if (hyperv_feat_enabled(cpu, HYPERV_FEAT_VPINDEX) &&
> -        kvm_enabled() && !kvm_hv_vpindex_settable()) {
> +    /*
> +    * kvm_enabled() must go first to ensure that kvm_* references are
> +    * not emitted for the linker to consume (kvm_enabled() is
> +    * a literal `0` in configurations where kvm_* aren't defined)
> +    */
> +    if (kvm_enabled() && hyperv_feat_enabled(cpu, HYPERV_FEAT_VPINDEX) &&
> +        !kvm_hv_vpindex_settable()) {
>           error_setg(errp, "kernel doesn't allow setting HyperV VP_INDEX");
>           return;
>       }