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

[Michael S. Tsirkin]

On Sun, Nov 19, 2023 at 07:34:58PM -0600, Dan Hoffman wrote:
> As far as I can tell, yes. Any optimization level above O0 does not have this
> issue (on this version of Clang, at least)

Aha, this is with -O0. That makes sense.
We have:
  ;;
  --enable-debug)
      # Enable debugging options that aren't excessively noisy
      meson_option_parse --enable-debug-tcg ""
      meson_option_parse --enable-debug-graph-lock ""
      meson_option_parse --enable-debug-mutex ""
      meson_option_add -Doptimization=0
      default_cflags='-O0 -g'


> On Sun, Nov 19, 2023 at 4:54 PM Philippe Mathieu-Daudé <philmd@linaro.org>
> wrote:
> 
>     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;
>     >       }
> 
>