Re: [PATCH 2/2] configure: Add -Wno-gnu-variable-sized-type-not-at-end

[Daniel P . Berrangé]

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 = {};

'kvm_msrs info' is variable in size, so offset of 'entries[1]' is
undefined by C99. I presume the GNU defined semantics are that the
variable length 'entries[]' field in 'info' is zero-sized, in order
to give predictable offset for 'entries[1]' in the local msr_data.

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

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|