| Message ID | 20241128201510.869974-3-pierrick.bouvier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Enable clang build on Windows | expand |
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 > ---------------------------------- >
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 > ---------------------------------- >
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
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
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.
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?
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 ----------------------------------
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> --- docs/devel/style.rst | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)