This attribute is not recognized by clang, but the associated option is.
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
meson.build | 8 ++++----
include/qemu/compiler.h | 7 +------
subprojects/libvhost-user/libvhost-user.h | 6 +-----
3 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/meson.build b/meson.build
index d8af08299e0..d0d5dbe1479 100644
--- a/meson.build
+++ b/meson.build
@@ -330,10 +330,10 @@ elif host_os == 'sunos'
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
+ # https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html
+ # We use this compilation option instead of relying on gcc_struct attribute
+ # because clang does not support it (but supports the option).
+ qemu_common_flags += ['-mno-ms-bitfields']
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/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.5On Wed, Oct 30, 2024 at 09:04:21PM -0700, Pierrick Bouvier wrote:
> This attribute is not recognized by clang, but the associated option is.
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> meson.build | 8 ++++----
> include/qemu/compiler.h | 7 +------
> subprojects/libvhost-user/libvhost-user.h | 6 +-----
> 3 files changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index d8af08299e0..d0d5dbe1479 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -330,10 +330,10 @@ elif host_os == 'sunos'
> 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
> + # https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html
> + # We use this compilation option instead of relying on gcc_struct attribute
> + # because clang does not support it (but supports the option).
> + qemu_common_flags += ['-mno-ms-bitfields']
> endif
Is this really safe for us to use ? The current gcc_struct
attribute affects only structs marked as QEMU_PACKED. This
flag will affect all code.
If we call from QEMU code into Windows native APIs, and pass
or receive structs, then those structs' layouts would be
affected by this flag. I don't have a specific example, but
this feels unsafe to me, otherwise we would have done this
originally rather than only targetting internal packed structs
with the gcc_struct attribute.
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 :|
On 31/10/2024 10.28, Daniel P. Berrangé wrote:
> On Wed, Oct 30, 2024 at 09:04:21PM -0700, Pierrick Bouvier wrote:
>> This attribute is not recognized by clang, but the associated option is.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>> meson.build | 8 ++++----
>> include/qemu/compiler.h | 7 +------
>> subprojects/libvhost-user/libvhost-user.h | 6 +-----
>> 3 files changed, 6 insertions(+), 15 deletions(-)
>>
>> diff --git a/meson.build b/meson.build
>> index d8af08299e0..d0d5dbe1479 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -330,10 +330,10 @@ elif host_os == 'sunos'
>> 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
>> + # https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html
>> + # We use this compilation option instead of relying on gcc_struct attribute
>> + # because clang does not support it (but supports the option).
>> + qemu_common_flags += ['-mno-ms-bitfields']
>> endif
>
> Is this really safe for us to use ? The current gcc_struct
> attribute affects only structs marked as QEMU_PACKED. This
> flag will affect all code.
>
> If we call from QEMU code into Windows native APIs, and pass
> or receive structs, then those structs' layouts would be
> affected by this flag. I don't have a specific example, but
> this feels unsafe to me, otherwise we would have done this
> originally rather than only targetting internal packed structs
> with the gcc_struct attribute.
I agree with Daniel, we likely cannot use this switch globally.
But seems like Clang folks are trying to include support for the attribute,
see: https://gitlab.com/qemu-project/qemu/-/issues/2476#note_2159643081
so I'd rather recommend to wait for that for proper Clang support here.
Thomas
On 10/31/24 03:44, Thomas Huth wrote:
> On 31/10/2024 10.28, Daniel P. Berrangé wrote:
>> On Wed, Oct 30, 2024 at 09:04:21PM -0700, Pierrick Bouvier wrote:
>>> This attribute is not recognized by clang, but the associated option is.
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>> meson.build | 8 ++++----
>>> include/qemu/compiler.h | 7 +------
>>> subprojects/libvhost-user/libvhost-user.h | 6 +-----
>>> 3 files changed, 6 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index d8af08299e0..d0d5dbe1479 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -330,10 +330,10 @@ elif host_os == 'sunos'
>>> 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
>>> + # https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html
>>> + # We use this compilation option instead of relying on gcc_struct attribute
>>> + # because clang does not support it (but supports the option).
>>> + qemu_common_flags += ['-mno-ms-bitfields']
>>> endif
>>
>> Is this really safe for us to use ? The current gcc_struct
>> attribute affects only structs marked as QEMU_PACKED. This
>> flag will affect all code.
>>
>> If we call from QEMU code into Windows native APIs, and pass
>> or receive structs, then those structs' layouts would be
>> affected by this flag. I don't have a specific example, but
>> this feels unsafe to me, otherwise we would have done this
>> originally rather than only targetting internal packed structs
>> with the gcc_struct attribute.
>
> I agree with Daniel, we likely cannot use this switch globally.
>
> But seems like Clang folks are trying to include support for the attribute,
> see: https://gitlab.com/qemu-project/qemu/-/issues/2476#note_2159643081
> so I'd rather recommend to wait for that for proper Clang support here.
>
> Thomas
>
Thanks for your reviews.
(adding Martin to the conversation, who worked on llvm-mingw toolchain,
and commented on GitLab issue).
As mentioned in gcc documentation, this option applies only to packed
structures, or when bitfield are used. So this is this second case that
may be a problem for us indeed.
Looking at mingw windows headers, I could find a few of them, but I
didn't check if they were used directly or indirectly by our code.
After reading the gitlab issue attached, and links associated, it seems
that QEMU is one of the only big projects using this. And clang support
is only blocked by this.
The upstream llvm support for this might take more time than expected
(the PR was opened more than 1 year ago... and the original report for
missing attribute support was in 2015), so I'm not very confident this
will appear "soon".
I noticed that Daniel conducted a small investigation using pahole, and
the report was that there were not so many difference (one found, but
only compile x86_64-softmmu target).
I would be willing to perform a full build (all dependencies, and all
targets), and compare what we obtain.
If we can fix the corner cases, would you be open to accept removing
gcc_struct from the codebase?
I can understand if it's a big NO, but I think it's unfortunate that we
are blocked today just because of this.
© 2016 - 2024 Red Hat, Inc.