[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stu
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid() |
Date: |
Tue, 12 Nov 2013 00:11:30 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130923 Thunderbird/17.0.9 |
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
- [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Peter Maydell, 2013/11/11
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Andreas Tobler, 2013/11/11
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Paolo Bonzini, 2013/11/11
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Peter Maydell, 2013/11/11
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(),
Paolo Bonzini <=
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Anthony Liguori, 2013/11/11
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Paolo Bonzini, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Peter Maydell, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Paolo Bonzini, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Peter Maydell, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Paolo Bonzini, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Peter Maydell, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Gleb Natapov, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Gleb Natapov, 2013/11/12
- Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid(), Paolo Bonzini, 2013/11/12