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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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