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

diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 2f68b500798..13cb1ef626b 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -416,6 +416,16 @@ 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 layout them than gcc
+<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_.
+For this reason, we disallow usage of bitfields in packed structures.
+For general usage, using bitfields should be proven to add significant benefits
+regarding memory usage or usability.
+
 Reserved namespaces in C and POSIX
 ----------------------------------
 
-- 
2.39.5

On 26/11/2024 22.17, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   docs/devel/style.rst | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 2f68b500798..13cb1ef626b 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -416,6 +416,16 @@ 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 layout them than gcc
> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_.
> +For this reason, we disallow usage of bitfields in packed structures.

I'd maybe add a "in new code" in above sentence - otherwise people will 
complain that there are pre-existing examples with packed structures that 
have bitfields in them.

  Thomas


> +For general usage, using bitfields should be proven to add significant benefits
> +regarding memory usage or usability.
> +
>   Reserved namespaces in C and POSIX
>   ----------------------------------
>

On 11/26/24 22:38, Thomas Huth wrote:
> On 26/11/2024 22.17, Pierrick Bouvier wrote:
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    docs/devel/style.rst | 10 ++++++++++
>>    1 file changed, 10 insertions(+)
>>
>> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
>> index 2f68b500798..13cb1ef626b 100644
>> --- a/docs/devel/style.rst
>> +++ b/docs/devel/style.rst
>> @@ -416,6 +416,16 @@ 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 layout them than gcc
>> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_.
>> +For this reason, we disallow usage of bitfields in packed structures.
> 
> I'd maybe add a "in new code" in above sentence - otherwise people will
> complain that there are pre-existing examples with packed structures that
> have bitfields in them.
> 
>    Thomas
> 

I'll add this, with other changes Peter suggested.

> 
>> +For general usage, using bitfields should be proven to add significant benefits
>> +regarding memory usage or usability.
>> +
>>    Reserved namespaces in C and POSIX
>>    ----------------------------------
>>    
>

On Tue, 26 Nov 2024 at 21:18, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  docs/devel/style.rst | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 2f68b500798..13cb1ef626b 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -416,6 +416,16 @@ 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 layout them than gcc

"to lay them out"

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

We should mention also that the layout is different on big and
little endian hosts.

> +For this reason, we disallow usage of bitfields in packed structures.

maybe add "and in any structures which are supposed to exactly
match a specific layout in guest memory" ?

> +For general usage, using bitfields should be proven to add significant benefits
> +regarding memory usage or usability.

Maybe phrase this as

 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.

?

> +
>  Reserved namespaces in C and POSIX
>  ----------------------------------
>
> --
> 2.39.5

thanks
-- PMM

On 11/26/24 13:28, Peter Maydell wrote:
> On Tue, 26 Nov 2024 at 21:18, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   docs/devel/style.rst | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
>> index 2f68b500798..13cb1ef626b 100644
>> --- a/docs/devel/style.rst
>> +++ b/docs/devel/style.rst
>> @@ -416,6 +416,16 @@ 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 layout them than gcc
> 
> "to lay them out"
> 

Thanks for the language fix to a non-native speaker :).

>> +<https://gcc.gnu.org/onlinedocs/gcc/x86-Type-Attributes.html>`_.
> 
> We should mention also that the layout is different on big and
> little endian hosts.
> 
>> +For this reason, we disallow usage of bitfields in packed structures.
> 
> maybe add "and in any structures which are supposed to exactly
> match a specific layout in guest memory" ?
>

I'll add this.

>> +For general usage, using bitfields should be proven to add significant benefits
>> +regarding memory usage or usability.
> 
> Maybe phrase this as
> 
>   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.
> 
> ?
> 

Ok!

Should we push further and try to convince people bitfields are not 
worth all the problem that come with them (except when really needed)?

Except for saving memory in *very* specific case (a structure allocated 
tens of millions times for example), I hardly see a benefit vs using 
integer types.

>> +
>>   Reserved namespaces in C and POSIX
>>   ----------------------------------
>>
>> --
>> 2.39.5
> 
> thanks
> -- PMM

On 11/26/24 18:15, Pierrick Bouvier wrote:
> Except for saving memory in *very* specific case (a structure allocated tens of millions 
> times for example), I hardly see a benefit vs using integer types.

Even then, 'uint32_t flags' can be just as easy to use as unsigned foo:1, bar:1, etc:1. 
Plus you get knowledge of the actual structure layout, which is presumably important 
*because* it's allocated millions of times.


r~

On 11/27/24 09:46, Richard Henderson wrote:
> On 11/26/24 18:15, Pierrick Bouvier wrote:
>> Except for saving memory in *very* specific case (a structure allocated tens of millions
>> times for example), I hardly see a benefit vs using integer types.
> 
> Even then, 'uint32_t flags' can be just as easy to use as unsigned foo:1, bar:1, etc:1.
> Plus you get knowledge of the actual structure layout, which is presumably important
> *because* it's allocated millions of times.
> 
> 
> r~
> 

Do we have a specific API (or set of macros) in QEMU to help with this?
If yes, maybe I could mention it in the doc ("we recommend using X 
instead of bitfields").

On 11/27/24 19:01, Pierrick Bouvier wrote:
> On 11/27/24 09:46, Richard Henderson wrote:
>> On 11/26/24 18:15, Pierrick Bouvier wrote:
>>> Except for saving memory in *very* specific case (a structure allocated tens of millions
>>> times for example), I hardly see a benefit vs using integer types.
>>
>> Even then, 'uint32_t flags' can be just as easy to use as unsigned foo:1, bar:1, etc:1.
>> Plus you get knowledge of the actual structure layout, which is presumably important
>> *because* it's allocated millions of times.
>>
>>
>> r~
>>
> 
> Do we have a specific API (or set of macros) in QEMU to help with this?
> If yes, maybe I could mention it in the doc ("we recommend using X instead of bitfields").

Yes, include/hw/registerfields.h.


r~