On Thu, 8 Sept 2022 at 10:09, Daniel P. Berrangé <berrange@redhat.com> wrote:
On Thu, Sep 08, 2022 at 09:53:44AM +0100, Peter Maydell wrote:
On Thu, 8 Sept 2022 at 09:08, Chenyi Qiang <chenyi.qiang@intel.com> wrote:
After updating linux headers to v6.0-rc, clang build on x86 target would
generate warnings like:
target/i386/kvm/kvm.c:470:25: error: field 'info' with variable sized
type 'struct kvm_msrs' not at the end of a struct or class is a GNU
extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct kvm_msrs info;
^
target/i386/kvm/kvm.c:1701:27: error: field 'cpuid' with variable sized
type 'struct kvm_cpuid2' not at the end of a struct or class is a GNU
extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct kvm_cpuid2 cpuid;
^
target/i386/kvm/kvm.c:2868:25: error: field 'info' with variable sized
type 'struct kvm_msrs' not at the end of a struct or class is a GNU
extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
struct kvm_msrs info;
^
Considering that it is OK to use GNU extension in QEMU (e.g. g_auto stuff),
it is acceptable to turn off this warning, which is only relevant to people
striving for fully portable C code.
Can we get the kernel folks to fix their headers not to
use GCC extensions like this ? It's not a big deal for us
I guess, but in general it doesn't seem great that the
kernel headers rely on userspace to silence warnings...
The kernel headers are fine actually - it is C99 compliant to have
a unsized array field in structs. eg
The problem is in the QEMU code which is taking the kernel declared
struct, and then embedding it inside another struct. e.g. the
kernel exposes:
struct kvm_msrs {
__u32 nmsrs; /* number of msrs in entries */
__u32 pad;
struct kvm_msr_entry entries[];
};
which we then use as:
uint64_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index)
{
struct {
struct kvm_msrs info;
struct kvm_msr_entry entries[1];
} msr_data = {};
Hmm, and the kernel used to define the 'entries' field as 'entries[0]',
which is the GNU 'zero-length-array' extension. So the kernel has
switched to the C-standard-defined flexible array member. Those
(unlike the GNU zero-length-array) have some extra restrictions
like this "can't put the struct into another struct" one. So
effectively we've moved from a GCC extension that clang doesn't
complain about to one that it does complain about.
IOW, our locally defined struct is just there to force a stack
allocation large enough for 1 kvm_msr_entry. A clever trick, but
requires that we turn off the CLang portability warning
I guess that's the most reasonable thing. Might be worth
expanding on some of this in the commit message, though.
Also, this patch disabling the warning needs to come before
the patch where the headers are updated; otherwise this will
break bisection with clang.