* [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
@ 2023-11-19 20:31 Daniel Hoffman
2023-11-19 22:54 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Hoffman @ 2023-11-19 20:31 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Daniel Hoffman, Michael S. Tsirkin,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
`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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-19 20:31 [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds Daniel Hoffman
@ 2023-11-19 22:54 ` Philippe Mathieu-Daudé
2023-11-20 1:34 ` Dan Hoffman
0 siblings, 1 reply; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-19 22:54 UTC (permalink / raw)
To: Daniel Hoffman, qemu-devel, Richard Henderson
Cc: qemu-trivial, Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini,
Eduardo Habkost
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;
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-19 22:54 ` Philippe Mathieu-Daudé
@ 2023-11-20 1:34 ` Dan Hoffman
2023-11-20 9:28 ` Michael S. Tsirkin
0 siblings, 1 reply; 10+ messages in thread
From: Dan Hoffman @ 2023-11-20 1:34 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Eduardo Habkost, Marcel Apfelbaum, Michael S. Tsirkin,
Paolo Bonzini, Richard Henderson, qemu-devel, qemu-trivial
[-- Attachment #1: Type: text/plain, Size: 2915 bytes --]
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;
> > }
>
>
[-- Attachment #2: Type: text/html, Size: 3809 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-20 1:34 ` Dan Hoffman
@ 2023-11-20 9:28 ` Michael S. Tsirkin
2023-11-20 10:20 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2023-11-20 9:28 UTC (permalink / raw)
To: Dan Hoffman
Cc: Philippe Mathieu-Daudé, Eduardo Habkost, Marcel Apfelbaum,
Paolo Bonzini, Richard Henderson, qemu-devel, qemu-trivial
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;
> > }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-20 9:28 ` Michael S. Tsirkin
@ 2023-11-20 10:20 ` Philippe Mathieu-Daudé
2023-11-20 15:30 ` Richard Henderson
2023-11-21 16:15 ` Eric Blake
0 siblings, 2 replies; 10+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-20 10:20 UTC (permalink / raw)
To: Michael S. Tsirkin, Dan Hoffman
Cc: Eduardo Habkost, Marcel Apfelbaum, Paolo Bonzini,
Richard Henderson, qemu-devel, qemu-trivial, Eric Blake
(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;
>> > }
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-20 10:20 ` Philippe Mathieu-Daudé
@ 2023-11-20 15:30 ` Richard Henderson
2023-11-21 16:15 ` Eric Blake
1 sibling, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2023-11-20 15:30 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Michael S. Tsirkin, Dan Hoffman
Cc: Eduardo Habkost, Marcel Apfelbaum, Paolo Bonzini, qemu-devel,
qemu-trivial, Eric Blake
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~
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-20 10:20 ` Philippe Mathieu-Daudé
2023-11-20 15:30 ` Richard Henderson
@ 2023-11-21 16:15 ` Eric Blake
2023-11-21 18:28 ` Dan Hoffman
1 sibling, 1 reply; 10+ messages in thread
From: Eric Blake @ 2023-11-21 16:15 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Michael S. Tsirkin, Dan Hoffman, Eduardo Habkost,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, qemu-devel,
qemu-trivial
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-21 16:15 ` Eric Blake
@ 2023-11-21 18:28 ` Dan Hoffman
2023-11-22 1:24 ` Dan Hoffman
0 siblings, 1 reply; 10+ messages in thread
From: Dan Hoffman @ 2023-11-21 18:28 UTC (permalink / raw)
To: Eric Blake
Cc: Philippe Mathieu-Daudé, Michael S. Tsirkin, Eduardo Habkost,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, qemu-devel,
qemu-trivial
I'm writing a patch to clang's constant folding to address this case
(doesn't seem too difficult). I'll either follow up with a link to
some submissions I've made or a bug report on the project describing
the issue.
On Tue, Nov 21, 2023 at 10:15 AM Eric Blake <eblake@redhat.com> wrote:
>
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-21 18:28 ` Dan Hoffman
@ 2023-11-22 1:24 ` Dan Hoffman
2023-11-23 18:03 ` Dan Hoffman
0 siblings, 1 reply; 10+ messages in thread
From: Dan Hoffman @ 2023-11-22 1:24 UTC (permalink / raw)
To: Eric Blake
Cc: Philippe Mathieu-Daudé, Michael S. Tsirkin, Eduardo Habkost,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, qemu-devel,
qemu-trivial
From their Discord server in reply to a question about whether such a
patch would be upstreamed: "I suspect this only works in gcc -O0
because of its AST-level "fold", which clang intentionally doesn't
implement. So probably not."
I hope that's enough information to resolve this patch. If any of you
need anything else, please let me know.
On Tue, Nov 21, 2023 at 12:28 PM Dan Hoffman <dhoff749@gmail.com> wrote:
>
> I'm writing a patch to clang's constant folding to address this case
> (doesn't seem too difficult). I'll either follow up with a link to
> some submissions I've made or a bug report on the project describing
> the issue.
>
>
>
> On Tue, Nov 21, 2023 at 10:15 AM Eric Blake <eblake@redhat.com> wrote:
> >
> > 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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds
2023-11-22 1:24 ` Dan Hoffman
@ 2023-11-23 18:03 ` Dan Hoffman
0 siblings, 0 replies; 10+ messages in thread
From: Dan Hoffman @ 2023-11-23 18:03 UTC (permalink / raw)
To: Eric Blake
Cc: Philippe Mathieu-Daudé, Michael S. Tsirkin, Eduardo Habkost,
Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, qemu-devel,
qemu-trivial
I went ahead and wrote a clang-tidy pass that attempts to find other
cases of this behavior (i.e. compile-time short-circuit behavior that
could lead to undefined references). All of these cases should also be
caught by `-Wunreachable-code`, but that uncovers a lot and I'd like a
green light before I pursue that path (also some legitimate patterns
like file-local debug macro definitions will be flagged as warnings).
This check is intentionally broad (whether a symbol is defined is more
a property of the linker, so the compiler doesn't know).
I think this patch should be merged as-is (since it solves the
immediate functional issue I'm having) and I may submit a more
complete patch in the future that guards all references to
potentially-undefined KVM functions with `CONFIG_KVM_IS_POSSIBLE` or
something along those lines. Let me know what you think.
Here's some output from the clang-tidy pass against the current master branch.
```
../hw/i386/intel_iommu.c:4131:31: warning: function not declared in
this file is called from a short-circutable context
[misc-short-circuit-elision]
4131 | if (kvm_enabled() && !kvm_enable_x2apic()) {
| ^
../hw/i386/x86.c:136:39: warning: function not declared in this file
is called from a short-circutable context [misc-short-circuit-elision]
136 | (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) {
| ^
../hw/i386/x86.c:422:27: warning: function not declared in this file
is called from a short-circutable context [misc-short-circuit-elision]
422 | kvm_enabled() && !kvm_hv_vpindex_settable()) {
| ^
../hw/net/i82596.c:481:23: warning: function not declared in this file
is called from a short-circutable context [misc-short-circuit-elision]
481 | if (USE_TIMER && !timer_pending(s->flush_queue_timer)) {
| ^
../hw/ppc/spapr.c:4543:27: warning: function not declared in this file
is called from a short-circutable context [misc-short-circuit-elision]
4543 | if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) {
| ^
../system/physmem.c:1913:27: warning: function not declared in this
file is called from a short-circutable context
[misc-short-circuit-elision]
1913 | if (kvm_enabled() && !kvm_has_sync_mmu()) {
| ^
../target/i386/cpu.c:7125:27: warning: function not declared in this
file is called from a short-circutable context
[misc-short-circuit-elision]
7125 | if (kvm_enabled() && !kvm_hyperv_expand_features(cpu, errp)) {
| ^
../target/s390x/diag.c:174:30: warning: function not declared in this
file is called from a short-circutable context
[misc-short-circuit-elision]
174 | if (kvm_enabled() && kvm_s390_get_hpage_1m()) {
| ^
```
If any of you have any questions, feel free to let me know
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-11-23 18:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-19 20:31 [PATCH v3] hw/i386: fix short-circuit logic with non-optimizing builds Daniel Hoffman
2023-11-19 22:54 ` Philippe Mathieu-Daudé
2023-11-20 1:34 ` Dan Hoffman
2023-11-20 9:28 ` Michael S. Tsirkin
2023-11-20 10:20 ` Philippe Mathieu-Daudé
2023-11-20 15:30 ` Richard Henderson
2023-11-21 16:15 ` Eric Blake
2023-11-21 18:28 ` Dan Hoffman
2023-11-22 1:24 ` Dan Hoffman
2023-11-23 18:03 ` Dan Hoffman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).