* [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).