[PATCH v3 0/3] Enable clang build on Windows

[PATCH v3 0/3] Enable clang build on Windows

[Pierrick Bouvier]

For now, it was only possible to build plugins using GCC on Windows. However,
windows-aarch64 only supports Clang.
This biggest roadblock was to get rid of gcc_struct attribute, which is not
supported by Clang. After investigation, we proved it was safe to drop it.

Built and tested on Windows (all msys env)/Linux/MacOS for x86_64 and aarch64
hosts.

v1 contained warning fixes and various bits that have been upstreamed already.
The only bits left in this series are the gcc_struct removal, and fixing the
plugins build with clang.

This series is for 10.0, as we decided to not include the gcc_struct removal is
9.2 release.

v1: https://patchew.org/QEMU/20241031040426.772604-1-pierrick.bouvier@linaro.org/

v2:
- drop attribute gcc_struct instead of using -mno-ms-bitfields option
- add a section about bitfields in documentation

v3:
- explain why gcc_struct attribute matters in packed structs in commit message
- reword the bitfields documentation with suggestions given

Pierrick Bouvier (3):
  win32: remove usage of attribute gcc_struct
  docs/devel/style: add a section about bitfield, and disallow them for
    packed structures
  plugins: enable linking with clang/lld

 docs/devel/style.rst                      | 20 +++++++++++++++++++
 meson.build                               |  6 +++---
 include/qemu/compiler.h                   |  7 +------
 scripts/cocci-macro-file.h                |  6 +-----
 subprojects/libvhost-user/libvhost-user.h |  6 +-----
 contrib/plugins/meson.build               |  2 +-
 plugins/meson.build                       | 24 +++++++++++++++++++----
 tests/tcg/plugins/meson.build             |  3 +--
 8 files changed, 48 insertions(+), 26 deletions(-)

-- 
2.39.5

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

[Pierrick Bouvier]

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,
as they are the one potentially impacted by this change.

[1] https://lore.kernel.org/qemu-devel/66c346de-7e20-4831-b3eb-1cda83240af9@linaro.org/

Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
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(-)

diff --git a/meson.build b/meson.build
index a290dbfa331..97cefb7cdd7 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

[PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Pierrick Bouvier]

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 docs/devel/style.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 2f68b500798..2d73e6a8f7a 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -416,6 +416,26 @@ definitions instead of typedefs in headers and function prototypes; this
 avoids problems with duplicated typedefs and reduces the need to include
 headers from other headers.
 
+Bitfields
+---------
+
+C bitfields can be a cause of non-portability issues, especially under windows
+where `MSVC has a different way to lay them out than gcc
+<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_, and on big and
+little endian hosts.
+
+For this reason, we disallow usage of bitfields in packed structures and in any
+structures which are supposed to exactly match a specific layout in guest
+memory. Some existing code may use it, and we carefully ensured the layout was
+the one expected.
+
+We also suggest avoiding bitfields even in structures where the exact
+layout does not matter, unless you can show that they provide a significant
+memory usage or usability benefit.
+
+We encourage the usage of ``include/hw/registerfields.h`` as a safe replacement
+for bitfields.
+
 Reserved namespaces in C and POSIX
 ----------------------------------
 
-- 
2.39.5

[PATCH v3 3/3] plugins: enable linking with clang/lld

[Pierrick Bouvier]

Windows uses a special mechanism to enable plugins to work (DLL delay
loading). Option for lld is different than ld.

MSYS2 clang based environment use lld by default, so restricting to this
config on Windows is safe, and will avoid false bug reports.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 meson.build                   |  5 +++++
 contrib/plugins/meson.build   |  2 +-
 plugins/meson.build           | 24 ++++++++++++++++++++----
 tests/tcg/plugins/meson.build |  3 +--
 4 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index 97cefb7cdd7..f286fb4f4a0 100644
--- a/meson.build
+++ b/meson.build
@@ -354,6 +354,11 @@ 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'
+  # plugins use delaylib, and clang needs to be used with lld to make it work.
+  if compiler.get_id() == 'clang' and compiler.get_linker_id() != 'ld.lld'
+    error('On windows, you need to use lld with clang - use msys2 clang64/clangarm64 env')
+  endif
 endif
 
 # Choose instruction set (currently x86-only)
diff --git a/contrib/plugins/meson.build b/contrib/plugins/meson.build
index 63a32c2b4f0..484b9a808c8 100644
--- a/contrib/plugins/meson.build
+++ b/contrib/plugins/meson.build
@@ -12,7 +12,7 @@ if get_option('plugins')
       t += shared_module(i, files(i + '.c') + 'win32_linker.c',
                         include_directories: '../../include/qemu',
                         link_depends: [win32_qemu_plugin_api_lib],
-                        link_args: ['-Lplugins', '-lqemu_plugin_api'],
+                        link_args: win32_qemu_plugin_api_link_flags,
                         dependencies: glib)
     else
       t += shared_module(i, files(i + '.c'),
diff --git a/plugins/meson.build b/plugins/meson.build
index 98542e926f8..d60be2a4d6d 100644
--- a/plugins/meson.build
+++ b/plugins/meson.build
@@ -17,14 +17,15 @@ if not enable_modules
       capture: true,
       command: ['sed', '-ne', 's/^[[:space:]]*\\(qemu_.*\\);/_\\1/p', '@INPUT@'])
     emulator_link_args += ['-Wl,-exported_symbols_list,plugins/qemu-plugins-ld64.symbols']
+  elif host_os == 'windows' and meson.get_compiler('c').get_id() == 'clang'
+    # LLVM/lld does not support exporting specific symbols. However, it works
+    # out of the box with dllexport/dllimport attribute we set in the code.
   else
     emulator_link_args += ['-Xlinker', '--dynamic-list=' + qemu_plugin_symbols.full_path()]
   endif
 endif
 
 if host_os == 'windows'
-  dlltool = find_program('dlltool', required: true)
-
   # Generate a .lib file for plugins to link against.
   # First, create a .def file listing all the symbols a plugin should expect to have
   # available in qemu
@@ -33,12 +34,27 @@ if host_os == 'windows'
     output: 'qemu_plugin_api.def',
     capture: true,
     command: ['sed', '-e', '0,/^/s//EXPORTS/; s/[{};]//g', '@INPUT@'])
+
   # then use dlltool to assemble a delaylib.
+  # The delaylib will have an "imaginary" name (qemu.exe), that is used by the
+  # linker file we add with plugins (win32_linker.c) to identify that we want
+  # to find missing symbols in current program.
+  win32_qemu_plugin_api_link_flags = ['-Lplugins', '-lqemu_plugin_api']
+  if meson.get_compiler('c').get_id() == 'clang'
+    # With LLVM/lld, delaylib is specified at link time (-delayload)
+    dlltool = find_program('llvm-dlltool', required: true)
+    dlltool_cmd = [dlltool, '-d', '@INPUT@', '-l', '@OUTPUT@', '-D', 'qemu.exe']
+    win32_qemu_plugin_api_link_flags += ['-Wl,-delayload=qemu.exe']
+  else
+    # With gcc/ld, delay lib is built with a specific delay parameter.
+    dlltool = find_program('dlltool', required: true)
+    dlltool_cmd = [dlltool, '--input-def', '@INPUT@',
+                   '--output-delaylib', '@OUTPUT@', '--dllname', 'qemu.exe']
+  endif
   win32_qemu_plugin_api_lib = configure_file(
     input: win32_plugin_def,
     output: 'libqemu_plugin_api.a',
-    command: [dlltool, '--input-def', '@INPUT@',
-              '--output-delaylib', '@OUTPUT@', '--dllname', 'qemu.exe']
+    command: dlltool_cmd
   )
 endif
 specific_ss.add(files(
diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
index f847849b1b7..87a17d67bd4 100644
--- a/tests/tcg/plugins/meson.build
+++ b/tests/tcg/plugins/meson.build
@@ -5,9 +5,8 @@ if get_option('plugins')
       t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
                         include_directories: '../../../include/qemu',
                         link_depends: [win32_qemu_plugin_api_lib],
-                        link_args: ['-Lplugins', '-lqemu_plugin_api'],
+                        link_args: win32_qemu_plugin_api_link_flags,
                         dependencies: glib)
-
     else
       t += shared_module(i, files(i + '.c'),
                         include_directories: '../../../include/qemu',
-- 
2.39.5

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

[Richard Henderson]

On 11/28/24 14:15, 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,
> as they are the one potentially impacted by this change.
> 
> [1] https://lore.kernel.org/qemu-devel/66c346de-7e20-4831-b3eb-1cda83240af9@linaro.org/
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Acked-by: Stefano Garzarella <sgarzare@redhat.com>
> 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(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Richard Henderson]

On 11/28/24 14:15, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   docs/devel/style.rst | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 2f68b500798..2d73e6a8f7a 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -416,6 +416,26 @@ definitions instead of typedefs in headers and function prototypes; this
>   avoids problems with duplicated typedefs and reduces the need to include
>   headers from other headers.
>   
> +Bitfields
> +---------
> +
> +C bitfields can be a cause of non-portability issues, especially under windows
> +where `MSVC has a different way to lay them out than gcc
> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_, and on big and
> +little endian hosts.
> +
> +For this reason, we disallow usage of bitfields in packed structures and in any
> +structures which are supposed to exactly match a specific layout in guest
> +memory. Some existing code may use it, and we carefully ensured the layout was
> +the one expected.
> +
> +We also suggest avoiding bitfields even in structures where the exact
> +layout does not matter, unless you can show that they provide a significant
> +memory usage or usability benefit.
> +
> +We encourage the usage of ``include/hw/registerfields.h`` as a safe replacement
> +for bitfields.
> +
>   Reserved namespaces in C and POSIX
>   ----------------------------------
>   

Re: [PATCH v3 0/3] Enable clang build on Windows

[Pierrick Bouvier]

Hi all,

On 11/28/24 12:15, Pierrick Bouvier wrote:
> For now, it was only possible to build plugins using GCC on Windows. However,
> windows-aarch64 only supports Clang.
> This biggest roadblock was to get rid of gcc_struct attribute, which is not
> supported by Clang. After investigation, we proved it was safe to drop it.
> 
> Built and tested on Windows (all msys env)/Linux/MacOS for x86_64 and aarch64
> hosts.
> 
> v1 contained warning fixes and various bits that have been upstreamed already.
> The only bits left in this series are the gcc_struct removal, and fixing the
> plugins build with clang.
> 
> This series is for 10.0, as we decided to not include the gcc_struct removal is
> 9.2 release.
> 
> v1: https://patchew.org/QEMU/20241031040426.772604-1-pierrick.bouvier@linaro.org/
> 
> v2:
> - drop attribute gcc_struct instead of using -mno-ms-bitfields option
> - add a section about bitfields in documentation
> 
> v3:
> - explain why gcc_struct attribute matters in packed structs in commit message
> - reword the bitfields documentation with suggestions given
> 
> Pierrick Bouvier (3):
>    win32: remove usage of attribute gcc_struct
>    docs/devel/style: add a section about bitfield, and disallow them for
>      packed structures
>    plugins: enable linking with clang/lld
> 
>   docs/devel/style.rst                      | 20 +++++++++++++++++++
>   meson.build                               |  6 +++---
>   include/qemu/compiler.h                   |  7 +------
>   scripts/cocci-macro-file.h                |  6 +-----
>   subprojects/libvhost-user/libvhost-user.h |  6 +-----
>   contrib/plugins/meson.build               |  2 +-
>   plugins/meson.build                       | 24 +++++++++++++++++++----
>   tests/tcg/plugins/meson.build             |  3 +--
>   8 files changed, 48 insertions(+), 26 deletions(-)
> 

gentle ping for this series, so it can be integrated when the trunk will 
be reopened after the release.

Patches 2 (has all the changes requested before) and 3 need review.

Thanks,
Pierrick

Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Philippe Mathieu-Daudé]

On 28/11/24 21:15, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   docs/devel/style.rst | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 2f68b500798..2d73e6a8f7a 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -416,6 +416,26 @@ definitions instead of typedefs in headers and function prototypes; this
>   avoids problems with duplicated typedefs and reduces the need to include
>   headers from other headers.
>   
> +Bitfields
> +---------
> +
> +C bitfields can be a cause of non-portability issues, especially under windows
> +where `MSVC has a different way to lay them out than gcc

"GCC" (as MSVC).

> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_, and on big and

"and on" sounds odd to me. Maybe ", or where endianness matters."?

> +little endian hosts.
> +
> +For this reason, we disallow usage of bitfields in packed structures and in any
> +structures which are supposed to exactly match a specific layout in guest
> +memory. Some existing code may use it, and we carefully ensured the layout was
> +the one expected.
> +
> +We also suggest avoiding bitfields even in structures where the exact
> +layout does not matter, unless you can show that they provide a significant
> +memory usage or usability benefit.

I don't think we should mention "significant memory usage benefit".

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +
> +We encourage the usage of ``include/hw/registerfields.h`` as a safe replacement
> +for bitfields.
> +
>   Reserved namespaces in C and POSIX
>   ----------------------------------
>   

Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Pierrick Bouvier]

Hi Philippe,

On 12/9/24 12:33, Philippe Mathieu-Daudé wrote:
> On 28/11/24 21:15, Pierrick Bouvier wrote:
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    docs/devel/style.rst | 20 ++++++++++++++++++++
>>    1 file changed, 20 insertions(+)
>>
>> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
>> index 2f68b500798..2d73e6a8f7a 100644
>> --- a/docs/devel/style.rst
>> +++ b/docs/devel/style.rst
>> @@ -416,6 +416,26 @@ definitions instead of typedefs in headers and function prototypes; this
>>    avoids problems with duplicated typedefs and reduces the need to include
>>    headers from other headers.
>>    
>> +Bitfields
>> +---------
>> +
>> +C bitfields can be a cause of non-portability issues, especially under windows
>> +where `MSVC has a different way to lay them out than gcc
> 
> "GCC" (as MSVC).
> 
>> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_, and on big and
> 
> "and on" sounds odd to me. Maybe ", or where endianness matters."?
> 
>> +little endian hosts.
>> +
>> +For this reason, we disallow usage of bitfields in packed structures and in any
>> +structures which are supposed to exactly match a specific layout in guest
>> +memory. Some existing code may use it, and we carefully ensured the layout was
>> +the one expected.
>> +
>> +We also suggest avoiding bitfields even in structures where the exact
>> +layout does not matter, unless you can show that they provide a significant
>> +memory usage or usability benefit.
> 
> I don't think we should mention "significant memory usage benefit".
> 
> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
>> +
>> +We encourage the usage of ``include/hw/registerfields.h`` as a safe replacement
>> +for bitfields.
>> +
>>    Reserved namespaces in C and POSIX
>>    ----------------------------------
>>    
> 

Thanks for the review, I'll integrate the changes in next version.
I'll wait a little bit to get feedback for patch 3 too.

Thanks,
Pierrick

Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Thomas Huth]

On 09/12/2024 21.33, Philippe Mathieu-Daudé wrote:
> On 28/11/24 21:15, Pierrick Bouvier wrote:
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
...
>> +For this reason, we disallow usage of bitfields in packed structures and 
>> in any
>> +structures which are supposed to exactly match a specific layout in guest
>> +memory. Some existing code may use it, and we carefully ensured the 
>> layout was
>> +the one expected.
>> +
>> +We also suggest avoiding bitfields even in structures where the exact
>> +layout does not matter, unless you can show that they provide a significant
>> +memory usage or usability benefit.
> 
> I don't think we should mention "significant memory usage benefit".

Why not? That's the point of bitfields, isn't it? Or do you mean it's 
included in "usability benefit" already?

  Thomas

Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Philippe Mathieu-Daudé]

On 10/12/24 08:52, Thomas Huth wrote:
> On 09/12/2024 21.33, Philippe Mathieu-Daudé wrote:
>> On 28/11/24 21:15, Pierrick Bouvier wrote:
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ...
>>> +For this reason, we disallow usage of bitfields in packed structures 
>>> and in any
>>> +structures which are supposed to exactly match a specific layout in 
>>> guest
>>> +memory. Some existing code may use it, and we carefully ensured the 
>>> layout was
>>> +the one expected.
>>> +
>>> +We also suggest avoiding bitfields even in structures where the exact
>>> +layout does not matter, unless you can show that they provide a 
>>> significant
>>> +memory usage or usability benefit.
>>
>> I don't think we should mention "significant memory usage benefit".
> 
> Why not? That's the point of bitfields, isn't it? Or do you mean it's 
> included in "usability benefit" already?

To not generate a reactance effect :) Developers trying to optimize
memory usage via bit field uses is extremely rare (at least in the
QEMU community).

Anyhow, I don't object to this patch as is.

Re: [PATCH v3 2/3] docs/devel/style: add a section about bitfield, and disallow them for packed structures

[Pierrick Bouvier]

On 12/10/24 02:10, Philippe Mathieu-Daudé wrote:
> On 10/12/24 08:52, Thomas Huth wrote:
>> On 09/12/2024 21.33, Philippe Mathieu-Daudé wrote:
>>> On 28/11/24 21:15, Pierrick Bouvier wrote:
>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ...
>>>> +For this reason, we disallow usage of bitfields in packed structures
>>>> and in any
>>>> +structures which are supposed to exactly match a specific layout in
>>>> guest
>>>> +memory. Some existing code may use it, and we carefully ensured the
>>>> layout was
>>>> +the one expected.
>>>> +
>>>> +We also suggest avoiding bitfields even in structures where the exact
>>>> +layout does not matter, unless you can show that they provide a
>>>> significant
>>>> +memory usage or usability benefit.
>>>
>>> I don't think we should mention "significant memory usage benefit".
>>
>> Why not? That's the point of bitfields, isn't it? Or do you mean it's
>> included in "usability benefit" already?
> 
> To not generate a reactance effect :) Developers trying to optimize
> memory usage via bit field uses is extremely rare (at least in the
> QEMU community).
> 
> Anyhow, I don't object to this patch as is.

I asked initially if we should simply advise against using bitfields, 
but it was not clearly answered if we want to do this or not.

 From your answers, it seems that people do not have a bias towards 
using bitfields for memory usage optimizations, so maybe we should 
simply discourage using them at all.
The only case I think of would be a specific struct format exchanged 
with the outside world, but most of the experienced developers know that 
using bitfields in "public" formats is a poor practice.

 From all that,
What should we say in the doc then?

Re: [PATCH v3 3/3] plugins: enable linking with clang/lld

[Philippe Mathieu-Daudé]

+Akihiko & Yonggang for
https://lore.kernel.org/qemu-devel/20201006120900.1579-1-luoyonggang@gmail.com/

On 28/11/24 21:15, Pierrick Bouvier wrote:
> Windows uses a special mechanism to enable plugins to work (DLL delay
> loading). Option for lld is different than ld.
> 
> MSYS2 clang based environment use lld by default, so restricting to this
> config on Windows is safe, and will avoid false bug reports.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> ---
>   meson.build                   |  5 +++++
>   contrib/plugins/meson.build   |  2 +-
>   plugins/meson.build           | 24 ++++++++++++++++++++----
>   tests/tcg/plugins/meson.build |  3 +--
>   4 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 97cefb7cdd7..f286fb4f4a0 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -354,6 +354,11 @@ 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'
> +  # plugins use delaylib, and clang needs to be used with lld to make it work.
> +  if compiler.get_id() == 'clang' and compiler.get_linker_id() != 'ld.lld'
> +    error('On windows, you need to use lld with clang - use msys2 clang64/clangarm64 env')
> +  endif
>   endif
>   
>   # Choose instruction set (currently x86-only)
> diff --git a/contrib/plugins/meson.build b/contrib/plugins/meson.build
> index 63a32c2b4f0..484b9a808c8 100644
> --- a/contrib/plugins/meson.build
> +++ b/contrib/plugins/meson.build
> @@ -12,7 +12,7 @@ if get_option('plugins')
>         t += shared_module(i, files(i + '.c') + 'win32_linker.c',
>                           include_directories: '../../include/qemu',
>                           link_depends: [win32_qemu_plugin_api_lib],
> -                        link_args: ['-Lplugins', '-lqemu_plugin_api'],
> +                        link_args: win32_qemu_plugin_api_link_flags,
>                           dependencies: glib)
>       else
>         t += shared_module(i, files(i + '.c'),
> diff --git a/plugins/meson.build b/plugins/meson.build
> index 98542e926f8..d60be2a4d6d 100644
> --- a/plugins/meson.build
> +++ b/plugins/meson.build
> @@ -17,14 +17,15 @@ if not enable_modules
>         capture: true,
>         command: ['sed', '-ne', 's/^[[:space:]]*\\(qemu_.*\\);/_\\1/p', '@INPUT@'])
>       emulator_link_args += ['-Wl,-exported_symbols_list,plugins/qemu-plugins-ld64.symbols']
> +  elif host_os == 'windows' and meson.get_compiler('c').get_id() == 'clang'
> +    # LLVM/lld does not support exporting specific symbols. However, it works
> +    # out of the box with dllexport/dllimport attribute we set in the code.
>     else
>       emulator_link_args += ['-Xlinker', '--dynamic-list=' + qemu_plugin_symbols.full_path()]
>     endif
>   endif
>   
>   if host_os == 'windows'
> -  dlltool = find_program('dlltool', required: true)
> -
>     # Generate a .lib file for plugins to link against.
>     # First, create a .def file listing all the symbols a plugin should expect to have
>     # available in qemu
> @@ -33,12 +34,27 @@ if host_os == 'windows'
>       output: 'qemu_plugin_api.def',
>       capture: true,
>       command: ['sed', '-e', '0,/^/s//EXPORTS/; s/[{};]//g', '@INPUT@'])
> +
>     # then use dlltool to assemble a delaylib.
> +  # The delaylib will have an "imaginary" name (qemu.exe), that is used by the
> +  # linker file we add with plugins (win32_linker.c) to identify that we want
> +  # to find missing symbols in current program.
> +  win32_qemu_plugin_api_link_flags = ['-Lplugins', '-lqemu_plugin_api']
> +  if meson.get_compiler('c').get_id() == 'clang'
> +    # With LLVM/lld, delaylib is specified at link time (-delayload)
> +    dlltool = find_program('llvm-dlltool', required: true)
> +    dlltool_cmd = [dlltool, '-d', '@INPUT@', '-l', '@OUTPUT@', '-D', 'qemu.exe']
> +    win32_qemu_plugin_api_link_flags += ['-Wl,-delayload=qemu.exe']
> +  else
> +    # With gcc/ld, delay lib is built with a specific delay parameter.
> +    dlltool = find_program('dlltool', required: true)
> +    dlltool_cmd = [dlltool, '--input-def', '@INPUT@',
> +                   '--output-delaylib', '@OUTPUT@', '--dllname', 'qemu.exe']
> +  endif
>     win32_qemu_plugin_api_lib = configure_file(
>       input: win32_plugin_def,
>       output: 'libqemu_plugin_api.a',
> -    command: [dlltool, '--input-def', '@INPUT@',
> -              '--output-delaylib', '@OUTPUT@', '--dllname', 'qemu.exe']
> +    command: dlltool_cmd
>     )
>   endif
>   specific_ss.add(files(
> diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
> index f847849b1b7..87a17d67bd4 100644
> --- a/tests/tcg/plugins/meson.build
> +++ b/tests/tcg/plugins/meson.build
> @@ -5,9 +5,8 @@ if get_option('plugins')
>         t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
>                           include_directories: '../../../include/qemu',
>                           link_depends: [win32_qemu_plugin_api_lib],
> -                        link_args: ['-Lplugins', '-lqemu_plugin_api'],
> +                        link_args: win32_qemu_plugin_api_link_flags,
>                           dependencies: glib)
> -
>       else
>         t += shared_module(i, files(i + '.c'),
>                           include_directories: '../../../include/qemu',