| Message ID | 20220908080749.32211-3-chenyi.qiang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update linux headers to v6.0-rc4 and fix the clang build error | expand |
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... -- PMM
On 9/8/22 09:07, Chenyi Qiang 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. > > Suggested-by: Daniel P. Berrangé<berrange@redhat.com> > Signed-off-by: Chenyi Qiang<chenyi.qiang@intel.com> > --- > configure | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> I'm surprised this is on with -std=gnu11, as opposed to -std=c11. r~
On Thu, Sep 08 2022, Peter Maydell <peter.maydell@linaro.org> 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... We only get the warnings once we define structures that include the imported structures, so I guess the header per se is not broken (and the kernel itself probably does not use them that way, given that there's an effort to build it with clang as well.)
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
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
On 9/8/22 10:09, Daniel P. Berrangé wrote: > '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. Correct. I invented this gcc extension for the benefit of glibc, which wanted to append N entries to that header, in static storage no less. I still find it odd that clang warns about a gnu extension when gnu extensions are requested via -std=gnu*. r~
On 9/8/2022 6:54 PM, Peter Maydell wrote: > 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. > Indeed. If necessary, I would expand the commit message and send the next version. > thanks > -- PMM
diff --git a/configure b/configure index 575dde1c1f..7e0a1a4187 100755 --- a/configure +++ b/configure @@ -1258,6 +1258,7 @@ add_to nowarn_flags -Wno-string-plus-int add_to nowarn_flags -Wno-typedef-redefinition add_to nowarn_flags -Wno-tautological-type-limit-compare add_to nowarn_flags -Wno-psabi +add_to nowarn_flags -Wno-gnu-variable-sized-type-not-at-end gcc_flags="$warn_flags $nowarn_flags"
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. Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com> --- configure | 1 + 1 file changed, 1 insertion(+)