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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 12/11/2013 12:07, Peter Maydell ha scritto:
> On 11 November 2013 23:21, Anthony Liguori <anthony@codemonkey.ws> 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

Yes, it's not trivial, but there are simpler ways to do it.

For example there is no need to make sure that env is non-NULL, only to
see that "something || 1" is never zero and thus "if (x) y;" is just
"(void)x; y;".  This seems easier to me than DCE after "break" which
clang is able to do.

Indeed, NULL checks (except perhaps very simple checks like &x != NULL)
are not something I'd expect the compiler to optimize out at -O0.

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

No, you couldn't because the "else" branch runs for "kvm_enabled() &&
!cpu->enable_pmu" too.  Relying on kvm_arch_get_supported_cpuid to
return 0 would only make code harder to review, and the above example
shows this fact very well.

kvm_arch_get_supported_cpuid abort()-ing would be fine, but it would
also make code harder to review, because you cannot rely anymore on the
compiler-linker combo to filter out the most obvious cases of forgetting
about TCG's existence.

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

Yes, but it wouldn't be _right_.  Most of the code below the "break" is
potentially applicable to TCG, even if it is not used now.

Paolo