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 22:19, Paolo Bonzini <address@hidden> wrote:
> 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 <address@hidden>
>
> 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);

That's also a bug, and it's also fixed by this patch (since it's
the same function).

If we have other places where we're relying on dead code elimination
to not provide a function definition, please point them out, because
they're bugs we need to fix, ideally before they cause compilation
failures.

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

Huh? The point of stub functions is to provide versions of functions
which either need to return an "always fails" code, or which will never
be called, but in either case this is so we can avoid peppering the
code with #ifdefs. The latter category is why we have stubs which
do nothing but call abort(). (If you think this stub should call abort()
I'm happy to roll a new patch which does that.)

>> 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 patch says it is to "speed up these RTL optimizers and by allocating
less memory, reduce the compiler footprint and possible memory
fragmentation". So they might investigate it as a performance
regression, but it's only a "make compilation faster" feature, not
correctness. Code which relies on dead-code-elimination is broken.

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

I think Andreas's patch is a bad idea and am against it being
applied. It's very obviously a random tweak aimed at a specific
compiler's implementation of dead-code elimination, and it's the
wrong way to fix the problem.

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

??  Debug builds should absolutely work out of the box -- if
debug build fails that is IMHO a release critical bug.

-- PMM