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 23:38, Peter Maydell ha scritto:
> 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.

I'm not sure, there are probably a few others.  Linux also relies on the
idiom (at least KVM does on x86).

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

There are very few stubs that call abort():

int kvm_cpu_exec(CPUState *cpu)
{
    abort();
}

int kvm_set_signal_mask(CPUState *cpu, const sigset_t *sigset)
{
    abort();
}

Calling abort() would be marginally better than returning 0, but why
defer checks to runtime when you can let the linker do them?

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

There's plenty of tests in the GCC testsuite that rely on DCE to test
that an optimization happened; some of them at -O0 too.  So it's become
a GCC feature in the end.

Code which relies on dead-code-elimination is not broken, it's relying
on the full power of the toolchain to ensure bugs are detected as soon
as possible, i.e. at build time.

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

It's very obviously a random tweak aimed at a specific compiler's bug in
dead-code elimination, I'm not denying that.  But the same compiler
feature is being exploited elsewhere.

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

Debug builds for qemu-system-{i386,x86_64} with clang on systems other
than x86/Linux.

Paolo