Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
On 13 November 2013 07:25, Paolo Bonzini pbonz...@redhat.com wrote: Il 13/11/2013 03:27, Richard Henderson ha scritto: I think it's also worthwhile to implement the kvm api in kvm-stub.c, unnecessary or not. If you really want compile-time feedback on those that ought to have been removed by optimization, you could elide them from the stub file depending on ifndef __OPTIMIZE__. Good idea. Peter, can you do that? I still think this serves no useful purpose and we'd be better off without such an ifndef, but I'd rather have a guarded stub than no stub, so I'll send a patch in a moment. thanks -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
On 11/13/2013 03:04 AM, Anthony Liguori wrote: On Tue, Nov 12, 2013 at 8:08 AM, Peter Maydell peter.mayd...@linaro.org wrote: On 12 November 2013 15:58, Paolo Bonzini pbonz...@redhat.com wrote: I don't really see a reason why QEMU should give clang more weight than Windows or Mac OS X. I'm not asking for more weight (and actually my main reason for caring about clang is exactly MacOSX). I'm just asking that when a bug is reported whose underlying cause is we don't work on clang because we're relying on undocumented behaviour of gcc with an attached patch that fixes this by not relying on the undocumented behaviour, that we apply the patch rather than saying why do we care about clang... QEMU has always been intimately tied to GCC. Heck, it all started as a giant GCC hack relying on entirely undocumented behavior (dyngen's disassembly of functions). There's nothing intrinsically bad about being tied to GCC. If you were making argument that we could do it a different way and the result would be as nice or nicer, then it wouldn't be a discussion. But if supporting clang means we have to remove useful things, then it's always going to be an uphill battle. In this case, the whole discussion is a bit silly. Have you actually For what it's worth, I think BOTH of the patches that have been posted should be applied. That is, the patch that does (X || 1) - (1 || X), and the patch that adds the stub. Frankly I'd have thought this was obvious and I'm a bit dismayed about how long this thread has continued. As far as GCC is concerned, we consider trivial dead code elimination like this to be a quality of implementation issue. We would never remove it, even from -O0. We can't guarantee how successful we can be, but that's what bug reports and regression tests are for. r~ -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
On 12 November 2013 18:54, Richard Henderson r...@twiddle.net wrote: For what it's worth, I think BOTH of the patches that have been posted should be applied. That is, the patch that does (X || 1) - (1 || X), and the patch that adds the stub. I think that makes sense and would be happy with that as a resolution. thanks -- PMM -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
Am 12.11.2013 19:57, schrieb Peter Maydell: On 12 November 2013 18:54, Richard Henderson r...@twiddle.net wrote: For what it's worth, I think BOTH of the patches that have been posted should be applied. That is, the patch that does (X || 1) - (1 || X), and the patch that adds the stub. I think that makes sense and would be happy with that as a resolution. thanks -- PMM +1. By the way: I added a stub for kvm_arch_get_supported_cpuid() in my kvm patch, too (see http://patchwork.ozlabs.org/patch/260512/). It called g_assert_not_reached()instead of returning 0.Maybe this can be added in a later patch. Regards, Stefan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
Il 12/11/2013 19:54, Richard Henderson ha scritto: For what it's worth, I think BOTH of the patches that have been posted should be applied. That is, the patch that does (X || 1) - (1 || X), and the patch that adds the stub. Frankly I'd have thought this was obvious It's not that obvious to me. If you add the stub, the patch that reorders operands is not necessary. If you reorder operands, the stub is not necessary. The patch that does (X || 1) - (1 || X) is unnecessary as a microoptimization, since this code basically runs once at startup. The code is also a little bit less clear with the reordered operands, but perhaps that's just me because I wrote the code that way. (Splitting the if in two would also make sense, and would not affect clarity). Why should both be applied? Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
On 11/13/2013 08:53 AM, Paolo Bonzini wrote: Il 12/11/2013 19:54, Richard Henderson ha scritto: For what it's worth, I think BOTH of the patches that have been posted should be applied. That is, the patch that does (X || 1) - (1 || X), and the patch that adds the stub. Frankly I'd have thought this was obvious It's not that obvious to me. If you add the stub, the patch that reorders operands is not necessary. If you reorder operands, the stub is not necessary. The patch that does (X || 1) - (1 || X) is unnecessary as a microoptimization, since this code basically runs once at startup. The code is also a little bit less clear with the reordered operands, but perhaps that's just me because I wrote the code that way. (Splitting the if in two would also make sense, and would not affect clarity). Why should both be applied? It's worth working around the clang missed optimization, if for nothing else than avoiding the noise of the bugs that would otherwise be filed against the release. I think it's also worthwhile to implement the kvm api in kvm-stub.c, unnecessary or not. If you really want compile-time feedback on those that ought to have been removed by optimization, you could elide them from the stub file depending on ifndef __OPTIMIZE__. r~ -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
Il 13/11/2013 03:27, Richard Henderson ha scritto: I think it's also worthwhile to implement the kvm api in kvm-stub.c, unnecessary or not. If you really want compile-time feedback on those that ought to have been removed by optimization, you could elide them from the stub file depending on ifndef __OPTIMIZE__. Good idea. Peter, can you do that? Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [PATCH for-1.7] target-i386: Fix build by providing stub kvm_arch_get_supported_cpuid()
On Wed, Nov 13, 2013 at 12:27:10PM +1000, Richard Henderson wrote: On 11/13/2013 08:53 AM, Paolo Bonzini wrote: Il 12/11/2013 19:54, Richard Henderson ha scritto: For what it's worth, I think BOTH of the patches that have been posted should be applied. That is, the patch that does (X || 1) - (1 || X), and the patch that adds the stub. Frankly I'd have thought this was obvious It's not that obvious to me. If you add the stub, the patch that reorders operands is not necessary. If you reorder operands, the stub is not necessary. The patch that does (X || 1) - (1 || X) is unnecessary as a microoptimization, since this code basically runs once at startup. The code is also a little bit less clear with the reordered operands, but perhaps that's just me because I wrote the code that way. (Splitting the if in two would also make sense, and would not affect clarity). Why should both be applied? It's worth working around the clang missed optimization, if for nothing else than avoiding the noise of the bugs that would otherwise be filed against the release. I think it's also worthwhile to implement the kvm api in kvm-stub.c, unnecessary or not. If you really want compile-time feedback on those that ought to have been removed by optimization, you could elide them from the stub file depending on ifndef __OPTIMIZE__. Sounds like a nice compromise. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
