Re: [PATCH v2 1/3] win32: remove usage of attribute gcc_struct

[Pierrick Bouvier]

On 11/27/24 01:20, Stefano Garzarella wrote:
> On Tue, Nov 26, 2024 at 01:17:34PM -0800, Pierrick Bouvier wrote:
> > This attribute is not recognized by clang.
> >
> > An investigation has been performed to ensure this attribute has no
> > effect on layout of structures we use in QEMU [1], so it's safe to
> > remove now.
> >
> > In the future, we'll forbid introducing new bitfields in packed struct.
>
> Why?
> I suggest reporting in this commit description what you wrote in the
> following patch in the documentation (or a summary), because at first
> glance I could not understand the correlation between removing
> gcc_struct and bitfields in packed struct.

Thanks,
I'll mention the problem came explicitely from the bitfields.

> > [1] 
> > 66c346de-7e20-4831-b3eb-1cda83240af9@linaro.org/">https://lore.kernel.org/qemu-devel/66c346de-7e20-4831-b3eb-1cda83240af9@linaro.org/
> >
> > Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> > ---
> > meson.build                               | 5 -----
> > include/qemu/compiler.h                   | 7 +------
> > scripts/cocci-macro-file.h                | 6 +-----
> > subprojects/libvhost-user/libvhost-user.h | 6 +-----
> > 4 files changed, 3 insertions(+), 21 deletions(-)
>
> The patch LGTM.
>
> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
>
> > diff --git a/meson.build b/meson.build
> > index e0b880e4e13..fa6d24e1233 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -354,11 +354,6 @@ elif host_os == 'sunos'
> >    qemu_common_flags += '-D__EXTENSIONS__'
> > elif host_os == 'haiku'
> >    qemu_common_flags += ['-DB_USE_POSITIVE_POSIX_ERRORS', '-D_BSD_SOURCE', 
> > '-fPIC']
> > -elif host_os == 'windows'
> > -  if not compiler.compiles('struct x { int y; } __attribute__((gcc_struct));',
> > -                           args: '-Werror')
> > -    error('Your compiler does not support __attribute__((gcc_struct)) - please 
> > use GCC instead of Clang')
> > -  endif
> > endif
> >
> > # Choose instruction set (currently x86-only)
> > diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> > index c06954ccb41..d904408e5ed 100644
> > --- a/include/qemu/compiler.h
> > +++ b/include/qemu/compiler.h
> > @@ -22,12 +22,7 @@
> > #define QEMU_EXTERN_C extern
> > #endif
> >
> > -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> > -# define QEMU_PACKED __attribute__((gcc_struct, packed))
> > -#else
> > -# define QEMU_PACKED __attribute__((packed))
> > -#endif
> > -
> > +#define QEMU_PACKED __attribute__((packed))
> > #define QEMU_ALIGNED(X) __attribute__((aligned(X)))
> >
> > #ifndef glue
> > diff --git a/scripts/cocci-macro-file.h b/scripts/cocci-macro-file.h
> > index d247a5086e9..c64831d5408 100644
> > --- a/scripts/cocci-macro-file.h
> > +++ b/scripts/cocci-macro-file.h
> > @@ -23,11 +23,7 @@
> > #define G_GNUC_WARN_UNUSED_RESULT __attribute__((warn_unused_result))
> > #define G_GNUC_NULL_TERMINATED __attribute__((sentinel))
> >
> > -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> > -# define QEMU_PACKED __attribute__((gcc_struct, packed))
> > -#else
> > -# define QEMU_PACKED __attribute__((packed))
> > -#endif
> > +#define QEMU_PACKED __attribute__((packed))
> >
> > #define cat(x,y) x ## y
> > #define cat2(x,y) cat(x,y)
> > diff --git a/subprojects/libvhost-user/libvhost-user.h 
> > b/subprojects/libvhost-user/libvhost-user.h
> > index deb40e77b3f..2ffc58c11b1 100644
> > --- a/subprojects/libvhost-user/libvhost-user.h
> > +++ b/subprojects/libvhost-user/libvhost-user.h
> > @@ -186,11 +186,7 @@ typedef struct VhostUserShared {
> >      unsigned char uuid[UUID_LEN];
> > } VhostUserShared;
> >
> > -#if defined(_WIN32) && (defined(__x86_64__) || defined(__i386__))
> > -# define VU_PACKED __attribute__((gcc_struct, packed))
> > -#else
> > -# define VU_PACKED __attribute__((packed))
> > -#endif
> > +#define VU_PACKED __attribute__((packed))
> >
> > typedef struct VhostUserMsg {
> >      int request;
> > --
> > 2.39.5