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

[Eric Blake]

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