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

[Peter Maydell]

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.

thanks
-- PMM