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 <dhoff...@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
