Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()

[Peter Maydell]

On 11 November 2013 23:21, Anthony Liguori <address@hidden> wrote:
> We're not talking about something obscure here.  It's eliminating an
> if(0) block.

No, we're not talking about a simple "if (0)" expression.
What we had in this case was
 if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) || !kvm_enabled()) {
    break;
 }
 [stuff using kvm_arch_get_supported_cpuid()]

[where kvm_enabled() is #defined to constant-0].

For the compiler to eliminate this we are relying on:
 * dead-code elimination of code following a 'break'
   statement in a case block
 * constant-folding of "something || 1" to 1
 * the compiler having done enough reasoning to be
   sure that env is not NULL

Self-evidently, not all compilers will provide
all of the above. Andreas' patch swaps the order
of the if() conditionals, which seems to work for
a wider set of compilers, but there's no guarantee
that will always work.

So exactly how much do we require our compiler's
constant-folding to handle? For example,

   unsigned char a,b,c;
   [...]
   if (a != 0 && b != 0 && c != 0
       && a * a * a + b * b * b == c * c * c) {

is compile-time provable to be always false; can
we rely on the branch being eliminated?

My position here is straightforward: the only thing
we can rely on is what the compilers document that
they will guarantee to do, which is nothing. I
don't see that relying on dead-code elimination
gains us anything significant at all, and adding
an extra stub or two (that won't even be linked
in if your compiler does happen to optimise) is
totally trivial overhead.

You and Paolo have both mentioned "doing checks
at compile time" but why is this particular
detail of our code worth checking in that way
at the expense of reliably being able to compile?

As it happens, having a stub function that returns
0 would simplify several bits of code that currently
do:

        if (kvm_enabled() && cpu->enable_pmu) {
            KVMState *s = cs->kvm_state;

            *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
            *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
            *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
            *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
        } else {
            *eax = 0;
            *ebx = 0;
            *ecx = 0;
            *edx = 0;
        }

because you could get rid of the else block.

Or you could #ifdef CONFIG_KVM this code section,
as we already do for some of the more complicated
bits of code that call this function. That would
work too.

-- PMM