Re: [PATCH 07/12] win32: use compiler option instead of attribute gcc_struct

[Thomas Huth <thuth@redhat.com>]

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