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

[Anthony Liguori <anthony@codemonkey.ws>]

On Mon, Nov 11, 2013 at 3:11 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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).

And they are there because it's a useful tool.

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

Exactly.

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

We're not talking about something obscure here.  It's eliminating an
if(0) block.  There's no reason to leave an if (0) block around.  The
code is never reachable.

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

Honestly, it's hard to treat clang as a first class target.  We don't
have much infrastructure around so it's not getting that much testing.

We really need to figure out how we're going to do CI.

FWIW, I'd rather just add -O1 for debug builds than add more stub functions.

Regards,

Anthony Liguori

>
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html