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