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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 11/11/2013 22:22, Peter Maydell ha scritto:
> Fix build failures with clang when KVM is not enabled by
> providing a stub version of kvm_arch_get_supported_cpuid().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

No, please don't.  We are already relying on dead code elimination for
KVM code (I didn't introduce the idiom), even in very similar code like
this one:

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

This is the first time that it breaks, and only on clang.  So this
really points at the very least at a compiler quirk (though perhaps not
a bug in the formal sense).  The point of kvm-stub.c is not to work
around compiler quirks, it is to avoid peppering the code with
kvm_enabled().  Adding a stub is wrong if the return code makes no sense
(as is the case here).

> I wouldn't be surprised if this also affected debug gcc
> builds with KVM disabled, but I haven't checked.

No, it doesn't affect GCC.  See Andreas's bug report.  Is it a bug or a
feature?  Having some kind of -O0 dead-code elimination is definitely a
feature (http://gcc.gnu.org/ml/gcc-patches/2003-03/msg02443.html).  That
first implementation already handled "||" and "&&" just fine, though it
didn't handle "break"/"continue"/"goto" in the clauses of the "if"
statement.  I think it would be considered a regression if this ceased
to work, but of course I may be wrong.

I am okay with Andreas's patch of course, but it would also be fine with
me to split the "if" in two, each with its own separate break statement.

Since it only affects debug builds, there is no hurry to fix this in 1.7
if the approach cannot be agreed with.

> Incidentally, since this is an x86 specific function its
> prototype should be moved into target-i386/kvm_i386.h, but
> that's a separate patch.

Yes, this I obviously agree with.

Paolo

>  target-i386/kvm-stub.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index 11429c4..18fe938 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -16,3 +16,9 @@ bool kvm_allows_irq0_override(void)
>  {
>      return 1;
>  }
> +
> +uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
> +                                      uint32_t index, int reg)
> +{
> +    return 0;
> +}
>