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

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

[Daniel Hoffman]

`kvm_enabled()` is compiled down to `0` and short-circuit logic is
used to remmove 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.

Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
---
 hw/i386/x86.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b3d054889bb..d339c8f3ef8 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -132,7 +132,7 @@ 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.
      */
-    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 +418,8 @@ 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()) {
+    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] hw/i386: fix short-circuit logic with non-optimizing builds

[Michael S. Tsirkin]

On Sat, Nov 18, 2023 at 10:25:31AM -0800, Daniel Hoffman wrote:
> `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> used to remmove 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.
> 
> Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>

Could we add a bit more detail here? Will help make sure
this does not break again in the future.

> ---
>  hw/i386/x86.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index b3d054889bb..d339c8f3ef8 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -132,7 +132,7 @@ 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.
>       */
> -    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 +418,8 @@ 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()) {
> +    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] hw/i386: fix short-circuit logic with non-optimizing builds

[Dan Hoffman]

On Sun, Nov 19, 2023 at 1:23 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Nov 18, 2023 at 10:25:31AM -0800, Daniel Hoffman wrote:
> > `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> > used to remmove 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.
> >
> > Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
>
> Could we add a bit more detail here? Will help make sure
> this does not break again in the future.

The configuration script was ran as such:  ../configure
--without-default-features --target-list=x86_64-softmmu,i386-softmmu
--enable-debug --enable-tcg-interpreter --enable-debug-tcg
--enable-debug-mutex

I'm pretty sure the only relevant flags here are
--without-default-features, --target-list including x86_64-softmmu and
--enable-debug

The only error I see is this: [...]/hw/i386/x86.c:422:(.text+0x1004):
undefined reference to `kvm_hv_vpindex_settable' (the other
kvm_enabled() was moved for the sake of consistency). My compiler is
clang (16.0.6).

I haven't looked into the heuristics or logic for how the compile-time
short-circuit logic works, but I assumed only the first parameter is
"guaranteed" to be checked for a literal false (guaranteed is in
quotes because that's just how clang works, not because it's a feature
of the language IIRC).

This pattern relies on somes subtle behavior with the compiler, so my
suggestion going forward would be to not rely on code optimizations
removing undefined references based on short-circuit logic (instead
have some configuration macro defined that disables all relevant
code). I'm a new contributor, so I submitted the minimum to make it
work on my machine.

If you have any other questions, please let me know.

Thanks!

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

[Michael S. Tsirkin]

On Sun, Nov 19, 2023 at 11:03:54AM -0600, Dan Hoffman wrote:
> On Sun, Nov 19, 2023 at 1:23 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sat, Nov 18, 2023 at 10:25:31AM -0800, Daniel Hoffman wrote:
> > > `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> > > used to remmove 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.
> > >
> > > Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> >
> > Could we add a bit more detail here? Will help make sure
> > this does not break again in the future.
> 
> The configuration script was ran as such:  ../configure
> --without-default-features --target-list=x86_64-softmmu,i386-softmmu
> --enable-debug --enable-tcg-interpreter --enable-debug-tcg
> --enable-debug-mutex
> 
> I'm pretty sure the only relevant flags here are
> --without-default-features, --target-list including x86_64-softmmu and
> --enable-debug
> 
> The only error I see is this: [...]/hw/i386/x86.c:422:(.text+0x1004):
> undefined reference to `kvm_hv_vpindex_settable' (the other
> kvm_enabled() was moved for the sake of consistency). My compiler is
> clang (16.0.6).
> 
> I haven't looked into the heuristics or logic for how the compile-time
> short-circuit logic works, but I assumed only the first parameter is
> "guaranteed" to be checked for a literal false (guaranteed is in
> quotes because that's just how clang works, not because it's a feature
> of the language IIRC).
> 
> This pattern relies on somes subtle behavior with the compiler, so my
> suggestion going forward would be to not rely on code optimizations
> removing undefined references based on short-circuit logic (instead
> have some configuration macro defined that disables all relevant
> code). I'm a new contributor, so I submitted the minimum to make it
> work on my machine.
> 
> If you have any other questions, please let me know.
> 
> Thanks!

which compiler is this?

-- 
MST

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

[Dan Hoffman]

Clang 16.0.6

I can re-submit with the compiler and version if that helps.

On Sun, Nov 19, 2023 at 2:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Nov 19, 2023 at 11:03:54AM -0600, Dan Hoffman wrote:
> > On Sun, Nov 19, 2023 at 1:23 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sat, Nov 18, 2023 at 10:25:31AM -0800, Daniel Hoffman wrote:
> > > > `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> > > > used to remmove 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.
> > > >
> > > > Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> > >
> > > Could we add a bit more detail here? Will help make sure
> > > this does not break again in the future.
> >
> > The configuration script was ran as such:  ../configure
> > --without-default-features --target-list=x86_64-softmmu,i386-softmmu
> > --enable-debug --enable-tcg-interpreter --enable-debug-tcg
> > --enable-debug-mutex
> >
> > I'm pretty sure the only relevant flags here are
> > --without-default-features, --target-list including x86_64-softmmu and
> > --enable-debug
> >
> > The only error I see is this: [...]/hw/i386/x86.c:422:(.text+0x1004):
> > undefined reference to `kvm_hv_vpindex_settable' (the other
> > kvm_enabled() was moved for the sake of consistency). My compiler is
> > clang (16.0.6).
> >
> > I haven't looked into the heuristics or logic for how the compile-time
> > short-circuit logic works, but I assumed only the first parameter is
> > "guaranteed" to be checked for a literal false (guaranteed is in
> > quotes because that's just how clang works, not because it's a feature
> > of the language IIRC).
> >
> > This pattern relies on somes subtle behavior with the compiler, so my
> > suggestion going forward would be to not rely on code optimizations
> > removing undefined references based on short-circuit logic (instead
> > have some configuration macro defined that disables all relevant
> > code). I'm a new contributor, so I submitted the minimum to make it
> > work on my machine.
> >
> > If you have any other questions, please let me know.
> >
> > Thanks!
>
> which compiler is this?
>
> --
> MST
>

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

[Michael S. Tsirkin]

On Sun, Nov 19, 2023 at 02:19:25PM -0600, Dan Hoffman wrote:
> Clang 16.0.6
> 
> I can re-submit with the compiler and version if that helps.

Worth mentioning this and the flags used I think.

> On Sun, Nov 19, 2023 at 2:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Nov 19, 2023 at 11:03:54AM -0600, Dan Hoffman wrote:
> > > On Sun, Nov 19, 2023 at 1:23 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Sat, Nov 18, 2023 at 10:25:31AM -0800, Daniel Hoffman wrote:
> > > > > `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> > > > > used to remmove 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.
> > > > >
> > > > > Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> > > >
> > > > Could we add a bit more detail here? Will help make sure
> > > > this does not break again in the future.
> > >
> > > The configuration script was ran as such:  ../configure
> > > --without-default-features --target-list=x86_64-softmmu,i386-softmmu
> > > --enable-debug --enable-tcg-interpreter --enable-debug-tcg
> > > --enable-debug-mutex
> > >
> > > I'm pretty sure the only relevant flags here are
> > > --without-default-features, --target-list including x86_64-softmmu and
> > > --enable-debug
> > >
> > > The only error I see is this: [...]/hw/i386/x86.c:422:(.text+0x1004):
> > > undefined reference to `kvm_hv_vpindex_settable' (the other
> > > kvm_enabled() was moved for the sake of consistency). My compiler is
> > > clang (16.0.6).
> > >
> > > I haven't looked into the heuristics or logic for how the compile-time
> > > short-circuit logic works, but I assumed only the first parameter is
> > > "guaranteed" to be checked for a literal false (guaranteed is in
> > > quotes because that's just how clang works, not because it's a feature
> > > of the language IIRC).
> > >
> > > This pattern relies on somes subtle behavior with the compiler, so my
> > > suggestion going forward would be to not rely on code optimizations
> > > removing undefined references based on short-circuit logic (instead
> > > have some configuration macro defined that disables all relevant
> > > code). I'm a new contributor, so I submitted the minimum to make it
> > > work on my machine.
> > >
> > > If you have any other questions, please let me know.
> > >
> > > Thanks!
> >
> > which compiler is this?
> >
> > --
> > MST
> >

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

[Dan Hoffman]

Submitted a v3 with the minimum reproducible build configuration

On Sun, Nov 19, 2023 at 2:25 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Nov 19, 2023 at 02:19:25PM -0600, Dan Hoffman wrote:
> > Clang 16.0.6
> >
> > I can re-submit with the compiler and version if that helps.
>
> Worth mentioning this and the flags used I think.
>
> > On Sun, Nov 19, 2023 at 2:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sun, Nov 19, 2023 at 11:03:54AM -0600, Dan Hoffman wrote:
> > > > On Sun, Nov 19, 2023 at 1:23 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Sat, Nov 18, 2023 at 10:25:31AM -0800, Daniel Hoffman wrote:
> > > > > > `kvm_enabled()` is compiled down to `0` and short-circuit logic is
> > > > > > used to remmove 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.
> > > > > >
> > > > > > Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> > > > >
> > > > > Could we add a bit more detail here? Will help make sure
> > > > > this does not break again in the future.
> > > >
> > > > The configuration script was ran as such:  ../configure
> > > > --without-default-features --target-list=x86_64-softmmu,i386-softmmu
> > > > --enable-debug --enable-tcg-interpreter --enable-debug-tcg
> > > > --enable-debug-mutex
> > > >
> > > > I'm pretty sure the only relevant flags here are
> > > > --without-default-features, --target-list including x86_64-softmmu and
> > > > --enable-debug
> > > >
> > > > The only error I see is this: [...]/hw/i386/x86.c:422:(.text+0x1004):
> > > > undefined reference to `kvm_hv_vpindex_settable' (the other
> > > > kvm_enabled() was moved for the sake of consistency). My compiler is
> > > > clang (16.0.6).
> > > >
> > > > I haven't looked into the heuristics or logic for how the compile-time
> > > > short-circuit logic works, but I assumed only the first parameter is
> > > > "guaranteed" to be checked for a literal false (guaranteed is in
> > > > quotes because that's just how clang works, not because it's a feature
> > > > of the language IIRC).
> > > >
> > > > This pattern relies on somes subtle behavior with the compiler, so my
> > > > suggestion going forward would be to not rely on code optimizations
> > > > removing undefined references based on short-circuit logic (instead
> > > > have some configuration macro defined that disables all relevant
> > > > code). I'm a new contributor, so I submitted the minimum to make it
> > > > work on my machine.
> > > >
> > > > If you have any other questions, please let me know.
> > > >
> > > > Thanks!
> > >
> > > which compiler is this?
> > >
> > > --
> > > MST
> > >
>