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

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

Posted by Daniel Hoffman 1 year ago

`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

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;
     }
-- 
2.40.1

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

Posted by Philippe Mathieu-Daudé 1 year ago

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;
>       }

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

Posted by Dan Hoffman 1 year ago

As far as I can tell, yes. Any optimization level above O0 does not have
this issue (on this version of Clang, at least)

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;
> >       }
>
>

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

Posted by Michael S. Tsirkin 1 year ago

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;
>     >       }
> 
>

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

Posted by Philippe Mathieu-Daudé 1 year ago

(Cc'ing Eric)

On 20/11/23 10:28, Michael S. Tsirkin wrote:
> 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.

But then, why the other cases aren't problematic?

$ git grep -E ' (&&|\|\|) !?kvm_enabled'
hw/arm/boot.c:1228:    assert(!(info->secure_board_setup && kvm_enabled()));
hw/i386/microvm.c:270:        (mms->rtc == ON_OFF_AUTO_AUTO && 
!kvm_enabled())) {
hw/i386/x86.c:135:    if (x86ms->apic_id_limit > 255 && kvm_enabled() &&
hw/mips/cps.c:62:    return is_mt && !kvm_enabled();
system/physmem.c:760:    assert(asidx == 0 || !kvm_enabled());
target/arm/cpu64.c:288:    if (value && kvm_enabled() && 
!kvm_arm_sve_supported()) {
target/i386/cpu.c:7264:    if (requested_lbr_fmt && kvm_enabled()) {
target/ppc/kvm.c:345:    if (!cpu->hash64_opts || !kvm_enabled()) {
target/s390x/cpu_models.c:574:    if (xcc->kvm_required && !kvm_enabled()) {
target/s390x/cpu_models_sysemu.c:124:    if 
(S390_CPU_CLASS(oc)->kvm_required && !kvm_enabled()) {

> 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;
>>      >       }
>>
>>
>

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

Posted by Eric Blake 1 year ago

On Mon, Nov 20, 2023 at 11:20:52AM +0100, Philippe Mathieu-Daudé wrote:
> (Cc'ing Eric)
> 
> On 20/11/23 10:28, Michael S. Tsirkin wrote:
> > 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.
> 
> But then, why the other cases aren't problematic?
> 
> $ git grep -E ' (&&|\|\|) !?kvm_enabled'
> hw/arm/boot.c:1228:    assert(!(info->secure_board_setup && kvm_enabled()));

This one's obvious; no kvm_*() calls in the assert.

> hw/i386/microvm.c:270:        (mms->rtc == ON_OFF_AUTO_AUTO &&
> !kvm_enabled())) {

Ones like this require reading context to see whether the if() block
guarded any kvm_*() calls for the linker to elide - but still a fairly
easy audit.

> > > 
> > >      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())) {

Indeed, if clang -O0 treats 'if (cond1 && 0 && cond2)' differently
than 'if (0 && cond1 && cond2)' for purposes of eliding the code for
cond2, that seems like a blatant missed optimization that we should be
reporting to the clang folks.

While this patch may solve the immediate issue, it does not scale - if
we ever have two separate guards that can both be independently
hard-coded to 0 based on configure-time decisions, but which are both
used as guards in the same expression, it will become impossible to
avoid a '(cond1 && 0 && cond2)' scenario across all 4 possible
configurations of those two guards.

I have no objection to the patch, but it would be nice if the commit
message could point to a clang bug report, if one has been filed.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

Posted by Richard Henderson 1 year ago

On 11/20/23 02:20, Philippe Mathieu-Daudé wrote:
> (Cc'ing Eric)
> 
> On 20/11/23 10:28, Michael S. Tsirkin wrote:
>> 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.
> 
> But then, why the other cases aren't problematic?
> 
> $ git grep -E ' (&&|\|\|) !?kvm_enabled'
> hw/arm/boot.c:1228:    assert(!(info->secure_board_setup && kvm_enabled()));
> hw/i386/microvm.c:270:        (mms->rtc == ON_OFF_AUTO_AUTO && !kvm_enabled())) {
> hw/i386/x86.c:135:    if (x86ms->apic_id_limit > 255 && kvm_enabled() &&
> hw/mips/cps.c:62:    return is_mt && !kvm_enabled();
> system/physmem.c:760:    assert(asidx == 0 || !kvm_enabled());
> target/arm/cpu64.c:288:    if (value && kvm_enabled() && !kvm_arm_sve_supported()) {
> target/i386/cpu.c:7264:    if (requested_lbr_fmt && kvm_enabled()) {
> target/ppc/kvm.c:345:    if (!cpu->hash64_opts || !kvm_enabled()) {
> target/s390x/cpu_models.c:574:    if (xcc->kvm_required && !kvm_enabled()) {
> target/s390x/cpu_models_sysemu.c:124:    if (S390_CPU_CLASS(oc)->kvm_required && 
> !kvm_enabled()) {
> 

Because those are very simple tests that do not reference kvm-specific stuff, unlike...

>>>      > -    if (x86ms->apic_id_limit > 255 && kvm_enabled() &&
>>>      > +    if (kvm_enabled() && x86ms->apic_id_limit > 255 &&
>>>      >           (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {

... these function calls.


r~