Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On 10/30, Julia Lawall wrote:
>
> On Tue, 30 Oct 2018, Shayenne Moura wrote:
>
> > Hi,
> >
> > > On Sun, 28 Oct 2018, Himanshu Jha wrote:
> > >
> > > > On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > > > > The "possible alignement issues" in CHECK report is difficult to
> > > > > > figure
> > > > > > out by just doing a glance analysis. :)
> > > > > >
> > > > > > Linus also suggested to use bool as the base type i.e., `bool x:1`
> > > > > > but
> > > > > > again sizeof(_Bool) is implementation defined ranging from 1-4
> > > > > > bytes.
> > > > >
> > > > > If bool x:1 has the size of bool, then wouldn't int x:1 have the size
> > > > > of
> > > > > int? But my little experiments suggest that the size is the smallest
> > > > > that
> > > > > fits the requested bits and alignment chosen by the compiler,
> > > > > regardless of
> > > > > the type.
> > > >
> > > > Yes, correct!
> > > > And we can't use sizeof on bitfields *directly*, nor reference it using
> > > > a
> > > > pointer.
> > > >
> > > > It can be applied only when these bitfields are wrapped in a structure.
> > > >
> > > > Testing:
> > > >
> > > > #include
> > > > #include
> > > >
> > > > struct S {
> > > > bool a:1;
> > > > bool b:1;
> > > > bool c:1;
> > > > bool d:1;
> > > > };
> > > >
> > > > int main(void)
> > > > {
> > > > printf("%zu\n", sizeof(struct S));
> > > > }
> > > >
> > > > Output: 1
> > > >
> > > > If I change all bool to unsigned int, output is: *4*.
> > > >
> > > > So, conclusion is compiler doesn't squeeze the size less than
> > > > native size of the datatype i.e., if we changed all members to
> > > > unsigned int:1,
> > > > total width = 4 bits
> > > > padding = 4 bits
> > > >
> > > > Therefore, total size should have been = 1 byte!
> > > > But since sizeof(unsigned int) == 4, it can't be squeezed to
> > > > less than it.
> > >
> > > This conclusion does not seem to be correct, if you try the following
> > > program. I get 4 for everything, meaning that the four unsigned int bits
> > > are getting squeezed into one byte when it is convenient.
> > >
> > > #include
> > > #include
> > >
> > > struct S1 {
> > > bool a:1;
> > > bool b:1;
> > > bool c:1;
> > > bool d:1;
> > > char a1;
> > > char a2;
> > > char a3;
> > > };
> > >
> > > struct S2 {
> > > unsigned int a:1;
> > > unsigned int b:1;
> > > unsigned int c:1;
> > > unsigned int d:1;
> > > char a1;
> > > char a2;
> > > char a3;
> > > };
> > >
> > > int main(void)
> > > {
> > > printf("%zu\n", sizeof(struct S1));
> > > printf("%zu\n", sizeof(struct S2));
> > > printf("%zu\n", sizeof(unsigned int));
> > > }
> > >
> > > > Well, int x:1 can either have 0..1 or -1..0 range due implementation
> > > > defined behavior as I said in the previous reply.
> > > >
> > > > If you really want to consider negative values, then make it explicit
> > > > using `signed int x:1` which make range guaranteed to be -1..0
> > >
> > > The code wants booleans, not negative values.
> > >
> > > julia
> >
> > Thank you all for the discussion!
> >
> > However, I think I do not understand the conclusion.
> >
> > It means that the best way is to use only boolean instead of use unsigned
> > int with bitfield? I mean specifically in the case of my patch, where there
> > are some boolean variables are mixed with other variables types.
>
> To my recollection, your code had a bool with larger types on either side.
> In that case, I think bool is fine. The compiler it likely to align those
> larger typed values such that the field with the bool type will get more
> than one byte no matter what type you use. If there are several fields
> with very small types adjacent, there might be some benefit to thinking
> about what the type should be.
>
> julia
Got it! Thank you!
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On 10/30, Julia Lawall wrote:
>
> On Tue, 30 Oct 2018, Shayenne Moura wrote:
>
> > Hi,
> >
> > > On Sun, 28 Oct 2018, Himanshu Jha wrote:
> > >
> > > > On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > > > > The "possible alignement issues" in CHECK report is difficult to
> > > > > > figure
> > > > > > out by just doing a glance analysis. :)
> > > > > >
> > > > > > Linus also suggested to use bool as the base type i.e., `bool x:1`
> > > > > > but
> > > > > > again sizeof(_Bool) is implementation defined ranging from 1-4
> > > > > > bytes.
> > > > >
> > > > > If bool x:1 has the size of bool, then wouldn't int x:1 have the size
> > > > > of
> > > > > int? But my little experiments suggest that the size is the smallest
> > > > > that
> > > > > fits the requested bits and alignment chosen by the compiler,
> > > > > regardless of
> > > > > the type.
> > > >
> > > > Yes, correct!
> > > > And we can't use sizeof on bitfields *directly*, nor reference it using
> > > > a
> > > > pointer.
> > > >
> > > > It can be applied only when these bitfields are wrapped in a structure.
> > > >
> > > > Testing:
> > > >
> > > > #include
> > > > #include
> > > >
> > > > struct S {
> > > > bool a:1;
> > > > bool b:1;
> > > > bool c:1;
> > > > bool d:1;
> > > > };
> > > >
> > > > int main(void)
> > > > {
> > > > printf("%zu\n", sizeof(struct S));
> > > > }
> > > >
> > > > Output: 1
> > > >
> > > > If I change all bool to unsigned int, output is: *4*.
> > > >
> > > > So, conclusion is compiler doesn't squeeze the size less than
> > > > native size of the datatype i.e., if we changed all members to
> > > > unsigned int:1,
> > > > total width = 4 bits
> > > > padding = 4 bits
> > > >
> > > > Therefore, total size should have been = 1 byte!
> > > > But since sizeof(unsigned int) == 4, it can't be squeezed to
> > > > less than it.
> > >
> > > This conclusion does not seem to be correct, if you try the following
> > > program. I get 4 for everything, meaning that the four unsigned int bits
> > > are getting squeezed into one byte when it is convenient.
> > >
> > > #include
> > > #include
> > >
> > > struct S1 {
> > > bool a:1;
> > > bool b:1;
> > > bool c:1;
> > > bool d:1;
> > > char a1;
> > > char a2;
> > > char a3;
> > > };
> > >
> > > struct S2 {
> > > unsigned int a:1;
> > > unsigned int b:1;
> > > unsigned int c:1;
> > > unsigned int d:1;
> > > char a1;
> > > char a2;
> > > char a3;
> > > };
> > >
> > > int main(void)
> > > {
> > > printf("%zu\n", sizeof(struct S1));
> > > printf("%zu\n", sizeof(struct S2));
> > > printf("%zu\n", sizeof(unsigned int));
> > > }
> > >
> > > > Well, int x:1 can either have 0..1 or -1..0 range due implementation
> > > > defined behavior as I said in the previous reply.
> > > >
> > > > If you really want to consider negative values, then make it explicit
> > > > using `signed int x:1` which make range guaranteed to be -1..0
> > >
> > > The code wants booleans, not negative values.
> > >
> > > julia
> >
> > Thank you all for the discussion!
> >
> > However, I think I do not understand the conclusion.
> >
> > It means that the best way is to use only boolean instead of use unsigned
> > int with bitfield? I mean specifically in the case of my patch, where there
> > are some boolean variables are mixed with other variables types.
>
> To my recollection, your code had a bool with larger types on either side.
> In that case, I think bool is fine. The compiler it likely to align those
> larger typed values such that the field with the bool type will get more
> than one byte no matter what type you use. If there are several fields
> with very small types adjacent, there might be some benefit to thinking
> about what the type should be.
>
> julia
Got it! Thank you!
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Tue, 30 Oct 2018, Shayenne Moura wrote:
> Hi,
>
> > On Sun, 28 Oct 2018, Himanshu Jha wrote:
> >
> > > On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > > > The "possible alignement issues" in CHECK report is difficult to
> > > > > figure
> > > > > out by just doing a glance analysis. :)
> > > > >
> > > > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> > > >
> > > > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > > > int? But my little experiments suggest that the size is the smallest
> > > > that
> > > > fits the requested bits and alignment chosen by the compiler,
> > > > regardless of
> > > > the type.
> > >
> > > Yes, correct!
> > > And we can't use sizeof on bitfields *directly*, nor reference it using a
> > > pointer.
> > >
> > > It can be applied only when these bitfields are wrapped in a structure.
> > >
> > > Testing:
> > >
> > > #include
> > > #include
> > >
> > > struct S {
> > > bool a:1;
> > > bool b:1;
> > > bool c:1;
> > > bool d:1;
> > > };
> > >
> > > int main(void)
> > > {
> > > printf("%zu\n", sizeof(struct S));
> > > }
> > >
> > > Output: 1
> > >
> > > If I change all bool to unsigned int, output is: *4*.
> > >
> > > So, conclusion is compiler doesn't squeeze the size less than
> > > native size of the datatype i.e., if we changed all members to
> > > unsigned int:1,
> > > total width = 4 bits
> > > padding = 4 bits
> > >
> > > Therefore, total size should have been = 1 byte!
> > > But since sizeof(unsigned int) == 4, it can't be squeezed to
> > > less than it.
> >
> > This conclusion does not seem to be correct, if you try the following
> > program. I get 4 for everything, meaning that the four unsigned int bits
> > are getting squeezed into one byte when it is convenient.
> >
> > #include
> > #include
> >
> > struct S1 {
> > bool a:1;
> > bool b:1;
> > bool c:1;
> > bool d:1;
> > char a1;
> > char a2;
> > char a3;
> > };
> >
> > struct S2 {
> > unsigned int a:1;
> > unsigned int b:1;
> > unsigned int c:1;
> > unsigned int d:1;
> > char a1;
> > char a2;
> > char a3;
> > };
> >
> > int main(void)
> > {
> > printf("%zu\n", sizeof(struct S1));
> > printf("%zu\n", sizeof(struct S2));
> > printf("%zu\n", sizeof(unsigned int));
> > }
> >
> > > Well, int x:1 can either have 0..1 or -1..0 range due implementation
> > > defined behavior as I said in the previous reply.
> > >
> > > If you really want to consider negative values, then make it explicit
> > > using `signed int x:1` which make range guaranteed to be -1..0
> >
> > The code wants booleans, not negative values.
> >
> > julia
>
> Thank you all for the discussion!
>
> However, I think I do not understand the conclusion.
>
> It means that the best way is to use only boolean instead of use unsigned
> int with bitfield? I mean specifically in the case of my patch, where there
> are some boolean variables are mixed with other variables types.
To my recollection, your code had a bool with larger types on either side.
In that case, I think bool is fine. The compiler it likely to align those
larger typed values such that the field with the bool type will get more
than one byte no matter what type you use. If there are several fields
with very small types adjacent, there might be some benefit to thinking
about what the type should be.
julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Tue, 30 Oct 2018, Shayenne Moura wrote:
> Hi,
>
> > On Sun, 28 Oct 2018, Himanshu Jha wrote:
> >
> > > On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > > > The "possible alignement issues" in CHECK report is difficult to
> > > > > figure
> > > > > out by just doing a glance analysis. :)
> > > > >
> > > > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> > > >
> > > > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > > > int? But my little experiments suggest that the size is the smallest
> > > > that
> > > > fits the requested bits and alignment chosen by the compiler,
> > > > regardless of
> > > > the type.
> > >
> > > Yes, correct!
> > > And we can't use sizeof on bitfields *directly*, nor reference it using a
> > > pointer.
> > >
> > > It can be applied only when these bitfields are wrapped in a structure.
> > >
> > > Testing:
> > >
> > > #include
> > > #include
> > >
> > > struct S {
> > > bool a:1;
> > > bool b:1;
> > > bool c:1;
> > > bool d:1;
> > > };
> > >
> > > int main(void)
> > > {
> > > printf("%zu\n", sizeof(struct S));
> > > }
> > >
> > > Output: 1
> > >
> > > If I change all bool to unsigned int, output is: *4*.
> > >
> > > So, conclusion is compiler doesn't squeeze the size less than
> > > native size of the datatype i.e., if we changed all members to
> > > unsigned int:1,
> > > total width = 4 bits
> > > padding = 4 bits
> > >
> > > Therefore, total size should have been = 1 byte!
> > > But since sizeof(unsigned int) == 4, it can't be squeezed to
> > > less than it.
> >
> > This conclusion does not seem to be correct, if you try the following
> > program. I get 4 for everything, meaning that the four unsigned int bits
> > are getting squeezed into one byte when it is convenient.
> >
> > #include
> > #include
> >
> > struct S1 {
> > bool a:1;
> > bool b:1;
> > bool c:1;
> > bool d:1;
> > char a1;
> > char a2;
> > char a3;
> > };
> >
> > struct S2 {
> > unsigned int a:1;
> > unsigned int b:1;
> > unsigned int c:1;
> > unsigned int d:1;
> > char a1;
> > char a2;
> > char a3;
> > };
> >
> > int main(void)
> > {
> > printf("%zu\n", sizeof(struct S1));
> > printf("%zu\n", sizeof(struct S2));
> > printf("%zu\n", sizeof(unsigned int));
> > }
> >
> > > Well, int x:1 can either have 0..1 or -1..0 range due implementation
> > > defined behavior as I said in the previous reply.
> > >
> > > If you really want to consider negative values, then make it explicit
> > > using `signed int x:1` which make range guaranteed to be -1..0
> >
> > The code wants booleans, not negative values.
> >
> > julia
>
> Thank you all for the discussion!
>
> However, I think I do not understand the conclusion.
>
> It means that the best way is to use only boolean instead of use unsigned
> int with bitfield? I mean specifically in the case of my patch, where there
> are some boolean variables are mixed with other variables types.
To my recollection, your code had a bool with larger types on either side.
In that case, I think bool is fine. The compiler it likely to align those
larger typed values such that the field with the bool type will get more
than one byte no matter what type you use. If there are several fields
with very small types adjacent, there might be some benefit to thinking
about what the type should be.
julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi,
> On Sun, 28 Oct 2018, Himanshu Jha wrote:
>
> > On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > > The "possible alignement issues" in CHECK report is difficult to figure
> > > > out by just doing a glance analysis. :)
> > > >
> > > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> > >
> > > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > > int? But my little experiments suggest that the size is the smallest that
> > > fits the requested bits and alignment chosen by the compiler, regardless
> > > of
> > > the type.
> >
> > Yes, correct!
> > And we can't use sizeof on bitfields *directly*, nor reference it using a
> > pointer.
> >
> > It can be applied only when these bitfields are wrapped in a structure.
> >
> > Testing:
> >
> > #include
> > #include
> >
> > struct S {
> > bool a:1;
> > bool b:1;
> > bool c:1;
> > bool d:1;
> > };
> >
> > int main(void)
> > {
> > printf("%zu\n", sizeof(struct S));
> > }
> >
> > Output: 1
> >
> > If I change all bool to unsigned int, output is: *4*.
> >
> > So, conclusion is compiler doesn't squeeze the size less than
> > native size of the datatype i.e., if we changed all members to
> > unsigned int:1,
> > total width = 4 bits
> > padding = 4 bits
> >
> > Therefore, total size should have been = 1 byte!
> > But since sizeof(unsigned int) == 4, it can't be squeezed to
> > less than it.
>
> This conclusion does not seem to be correct, if you try the following
> program. I get 4 for everything, meaning that the four unsigned int bits
> are getting squeezed into one byte when it is convenient.
>
> #include
> #include
>
> struct S1 {
> bool a:1;
> bool b:1;
> bool c:1;
> bool d:1;
> char a1;
> char a2;
> char a3;
> };
>
> struct S2 {
> unsigned int a:1;
> unsigned int b:1;
> unsigned int c:1;
> unsigned int d:1;
> char a1;
> char a2;
> char a3;
> };
>
> int main(void)
> {
> printf("%zu\n", sizeof(struct S1));
> printf("%zu\n", sizeof(struct S2));
> printf("%zu\n", sizeof(unsigned int));
> }
>
> > Well, int x:1 can either have 0..1 or -1..0 range due implementation
> > defined behavior as I said in the previous reply.
> >
> > If you really want to consider negative values, then make it explicit
> > using `signed int x:1` which make range guaranteed to be -1..0
>
> The code wants booleans, not negative values.
>
> julia
Thank you all for the discussion!
However, I think I do not understand the conclusion.
It means that the best way is to use only boolean instead of use unsigned
int with bitfield? I mean specifically in the case of my patch, where there
are some boolean variables are mixed with other variables types.
Best,
Shayenne
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi,
> On Sun, 28 Oct 2018, Himanshu Jha wrote:
>
> > On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > > The "possible alignement issues" in CHECK report is difficult to figure
> > > > out by just doing a glance analysis. :)
> > > >
> > > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> > >
> > > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > > int? But my little experiments suggest that the size is the smallest that
> > > fits the requested bits and alignment chosen by the compiler, regardless
> > > of
> > > the type.
> >
> > Yes, correct!
> > And we can't use sizeof on bitfields *directly*, nor reference it using a
> > pointer.
> >
> > It can be applied only when these bitfields are wrapped in a structure.
> >
> > Testing:
> >
> > #include
> > #include
> >
> > struct S {
> > bool a:1;
> > bool b:1;
> > bool c:1;
> > bool d:1;
> > };
> >
> > int main(void)
> > {
> > printf("%zu\n", sizeof(struct S));
> > }
> >
> > Output: 1
> >
> > If I change all bool to unsigned int, output is: *4*.
> >
> > So, conclusion is compiler doesn't squeeze the size less than
> > native size of the datatype i.e., if we changed all members to
> > unsigned int:1,
> > total width = 4 bits
> > padding = 4 bits
> >
> > Therefore, total size should have been = 1 byte!
> > But since sizeof(unsigned int) == 4, it can't be squeezed to
> > less than it.
>
> This conclusion does not seem to be correct, if you try the following
> program. I get 4 for everything, meaning that the four unsigned int bits
> are getting squeezed into one byte when it is convenient.
>
> #include
> #include
>
> struct S1 {
> bool a:1;
> bool b:1;
> bool c:1;
> bool d:1;
> char a1;
> char a2;
> char a3;
> };
>
> struct S2 {
> unsigned int a:1;
> unsigned int b:1;
> unsigned int c:1;
> unsigned int d:1;
> char a1;
> char a2;
> char a3;
> };
>
> int main(void)
> {
> printf("%zu\n", sizeof(struct S1));
> printf("%zu\n", sizeof(struct S2));
> printf("%zu\n", sizeof(unsigned int));
> }
>
> > Well, int x:1 can either have 0..1 or -1..0 range due implementation
> > defined behavior as I said in the previous reply.
> >
> > If you really want to consider negative values, then make it explicit
> > using `signed int x:1` which make range guaranteed to be -1..0
>
> The code wants booleans, not negative values.
>
> julia
Thank you all for the discussion!
However, I think I do not understand the conclusion.
It means that the best way is to use only boolean instead of use unsigned
int with bitfield? I mean specifically in the case of my patch, where there
are some boolean variables are mixed with other variables types.
Best,
Shayenne
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, 28 Oct 2018, Himanshu Jha wrote:
> On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > The "possible alignement issues" in CHECK report is difficult to figure
> > > out by just doing a glance analysis. :)
> > >
> > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> >
> > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > int? But my little experiments suggest that the size is the smallest that
> > fits the requested bits and alignment chosen by the compiler, regardless of
> > the type.
>
> Yes, correct!
> And we can't use sizeof on bitfields *directly*, nor reference it using a
> pointer.
>
> It can be applied only when these bitfields are wrapped in a structure.
>
> Testing:
>
> #include
> #include
>
> struct S {
> bool a:1;
> bool b:1;
> bool c:1;
> bool d:1;
> };
>
> int main(void)
> {
> printf("%zu\n", sizeof(struct S));
> }
>
> Output: 1
>
> If I change all bool to unsigned int, output is: *4*.
>
> So, conclusion is compiler doesn't squeeze the size less than
> native size of the datatype i.e., if we changed all members to
> unsigned int:1,
> total width = 4 bits
> padding = 4 bits
>
> Therefore, total size should have been = 1 byte!
> But since sizeof(unsigned int) == 4, it can't be squeezed to
> less than it.
This conclusion does not seem to be correct, if you try the following
program. I get 4 for everything, meaning that the four unsigned int bits
are getting squeezed into one byte when it is convenient.
#include
#include
struct S1 {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
char a1;
char a2;
char a3;
};
struct S2 {
unsigned int a:1;
unsigned int b:1;
unsigned int c:1;
unsigned int d:1;
char a1;
char a2;
char a3;
};
int main(void)
{
printf("%zu\n", sizeof(struct S1));
printf("%zu\n", sizeof(struct S2));
printf("%zu\n", sizeof(unsigned int));
}
> Well, int x:1 can either have 0..1 or -1..0 range due implementation
> defined behavior as I said in the previous reply.
>
> If you really want to consider negative values, then make it explicit
> using `signed int x:1` which make range guaranteed to be -1..0
The code wants booleans, not negative values.
julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, 28 Oct 2018, Himanshu Jha wrote:
> On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > > The "possible alignement issues" in CHECK report is difficult to figure
> > > out by just doing a glance analysis. :)
> > >
> > > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
> >
> > If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> > int? But my little experiments suggest that the size is the smallest that
> > fits the requested bits and alignment chosen by the compiler, regardless of
> > the type.
>
> Yes, correct!
> And we can't use sizeof on bitfields *directly*, nor reference it using a
> pointer.
>
> It can be applied only when these bitfields are wrapped in a structure.
>
> Testing:
>
> #include
> #include
>
> struct S {
> bool a:1;
> bool b:1;
> bool c:1;
> bool d:1;
> };
>
> int main(void)
> {
> printf("%zu\n", sizeof(struct S));
> }
>
> Output: 1
>
> If I change all bool to unsigned int, output is: *4*.
>
> So, conclusion is compiler doesn't squeeze the size less than
> native size of the datatype i.e., if we changed all members to
> unsigned int:1,
> total width = 4 bits
> padding = 4 bits
>
> Therefore, total size should have been = 1 byte!
> But since sizeof(unsigned int) == 4, it can't be squeezed to
> less than it.
This conclusion does not seem to be correct, if you try the following
program. I get 4 for everything, meaning that the four unsigned int bits
are getting squeezed into one byte when it is convenient.
#include
#include
struct S1 {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
char a1;
char a2;
char a3;
};
struct S2 {
unsigned int a:1;
unsigned int b:1;
unsigned int c:1;
unsigned int d:1;
char a1;
char a2;
char a3;
};
int main(void)
{
printf("%zu\n", sizeof(struct S1));
printf("%zu\n", sizeof(struct S2));
printf("%zu\n", sizeof(unsigned int));
}
> Well, int x:1 can either have 0..1 or -1..0 range due implementation
> defined behavior as I said in the previous reply.
>
> If you really want to consider negative values, then make it explicit
> using `signed int x:1` which make range guaranteed to be -1..0
The code wants booleans, not negative values.
julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > The "possible alignement issues" in CHECK report is difficult to figure
> > out by just doing a glance analysis. :)
> >
> > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
>
> If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> int? But my little experiments suggest that the size is the smallest that
> fits the requested bits and alignment chosen by the compiler, regardless of
> the type.
Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a
pointer.
It can be applied only when these bitfields are wrapped in a structure.
Testing:
#include
#include
struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};
int main(void)
{
printf("%zu\n", sizeof(struct S));
}
Output: 1
If I change all bool to unsigned int, output is: *4*.
So, conclusion is compiler doesn't squeeze the size less than
native size of the datatype i.e., if we changed all members to
unsigned int:1,
total width = 4 bits
padding = 4 bits
Therefore, total size should have been = 1 byte!
But since sizeof(unsigned int) == 4, it can't be squeezed to
less than it.
> bool x:1 has the advantage that anything that is not 0 is considered true.
Yes, implicit conversion rules for boolean.
> So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false.
Well, int x:1 can either have 0..1 or -1..0 range due implementation
defined behavior as I said in the previous reply.
If you really want to consider negative values, then make it explicit
using `signed int x:1` which make range guaranteed to be -1..0
Regardless, integer conversion rules will kick in.
> But the :1 adds instructions, so at least for only one bool, where little
> space is saved, it is probably not worth it.
True, we should reply on a promised guideline rather than possibility.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sun, Oct 28, 2018 at 09:47:15AM +0100, Julia Lawall wrote:
> > The "possible alignement issues" in CHECK report is difficult to figure
> > out by just doing a glance analysis. :)
> >
> > Linus also suggested to use bool as the base type i.e., `bool x:1` but
> > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
>
> If bool x:1 has the size of bool, then wouldn't int x:1 have the size of
> int? But my little experiments suggest that the size is the smallest that
> fits the requested bits and alignment chosen by the compiler, regardless of
> the type.
Yes, correct!
And we can't use sizeof on bitfields *directly*, nor reference it using a
pointer.
It can be applied only when these bitfields are wrapped in a structure.
Testing:
#include
#include
struct S {
bool a:1;
bool b:1;
bool c:1;
bool d:1;
};
int main(void)
{
printf("%zu\n", sizeof(struct S));
}
Output: 1
If I change all bool to unsigned int, output is: *4*.
So, conclusion is compiler doesn't squeeze the size less than
native size of the datatype i.e., if we changed all members to
unsigned int:1,
total width = 4 bits
padding = 4 bits
Therefore, total size should have been = 1 byte!
But since sizeof(unsigned int) == 4, it can't be squeezed to
less than it.
> bool x:1 has the advantage that anything that is not 0 is considered true.
Yes, implicit conversion rules for boolean.
> So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false.
Well, int x:1 can either have 0..1 or -1..0 range due implementation
defined behavior as I said in the previous reply.
If you really want to consider negative values, then make it explicit
using `signed int x:1` which make range guaranteed to be -1..0
Regardless, integer conversion rules will kick in.
> But the :1 adds instructions, so at least for only one bool, where little
> space is saved, it is probably not worth it.
True, we should reply on a promised guideline rather than possibility.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
> The "possible alignement issues" in CHECK report is difficult to figure > out by just doing a glance analysis. :) > > Linus also suggested to use bool as the base type i.e., `bool x:1` but > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes. If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type. bool x:1 has the advantage that anything that is not 0 is considered true. So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false. But the :1 adds instructions, so at least for only one bool, where little space is saved, it is probably not worth it. julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
> The "possible alignement issues" in CHECK report is difficult to figure > out by just doing a glance analysis. :) > > Linus also suggested to use bool as the base type i.e., `bool x:1` but > again sizeof(_Bool) is implementation defined ranging from 1-4 bytes. If bool x:1 has the size of bool, then wouldn't int x:1 have the size of int? But my little experiments suggest that the size is the smallest that fits the requested bits and alignment chosen by the compiler, regardless of the type. bool x:1 has the advantage that anything that is not 0 is considered true. So for bool x:1, x = 4 is true, while for int x:1, x = 4 is false. But the :1 adds instructions, so at least for only one bool, where little space is saved, it is probably not worth it. julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi Sasha,
On Fri, Oct 26, 2018 at 04:42:25PM -0400, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
Agreed!
FYI original thread of discussion: https://lkml.org/lkml/2017/11/21/207
As Steve says:
"Thus, changing:
int a : 1;
int b : 1;
int c : 1;
int d : 1;
to
bool a;
bool b;
bool c;
bool d;
at best increases the size required from 1 byte to 4 bytes, and at
worse, it increases it from one byte to 16 bytes."
In the above cases, we have all bitfields members and no non-bitfields
members.
But before playing with these bitfields there are some points to be
noted:
https://port70.net/~nsz/c/c11/n1570.html#J.3.9
Implementation Defined
--
* Whether a ''plain'' int bit-field is treated as a signed int
bit-field or as an unsigned int bit-field.
eg: `int foo:3` may have a range from 0..7 or -4..3
So, changing `int foo:3` to `unsigned int foo:3` might be a sane
change to remove the ambiguity and make sure range is 0..7.
Also, such an change can also handle unsigned overflow
or better said "wrapping".
* Whether a bit-field can straddle a storage-unit boundary.
So, you can't guess what could be the possible unless you're familiar
or tested the change for different arch.
* The alignment of non-bit-field members of structures.
...
The "possible alignement issues" in CHECK report is difficult to figure
out by just doing a glance analysis. :)
Linus also suggested to use bool as the base type i.e., `bool x:1` but
again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
And since this issue is added to checkpatch now, very likely there would
be blast of patches sent on the same.
Not everyone who sends checkpatch would be able to disassemble or test
on different implementations.
But if anyone is interested check this:
https://godbolt.org/
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
Hi Sasha,
On Fri, Oct 26, 2018 at 04:42:25PM -0400, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
Agreed!
FYI original thread of discussion: https://lkml.org/lkml/2017/11/21/207
As Steve says:
"Thus, changing:
int a : 1;
int b : 1;
int c : 1;
int d : 1;
to
bool a;
bool b;
bool c;
bool d;
at best increases the size required from 1 byte to 4 bytes, and at
worse, it increases it from one byte to 16 bytes."
In the above cases, we have all bitfields members and no non-bitfields
members.
But before playing with these bitfields there are some points to be
noted:
https://port70.net/~nsz/c/c11/n1570.html#J.3.9
Implementation Defined
--
* Whether a ''plain'' int bit-field is treated as a signed int
bit-field or as an unsigned int bit-field.
eg: `int foo:3` may have a range from 0..7 or -4..3
So, changing `int foo:3` to `unsigned int foo:3` might be a sane
change to remove the ambiguity and make sure range is 0..7.
Also, such an change can also handle unsigned overflow
or better said "wrapping".
* Whether a bit-field can straddle a storage-unit boundary.
So, you can't guess what could be the possible unless you're familiar
or tested the change for different arch.
* The alignment of non-bit-field members of structures.
...
The "possible alignement issues" in CHECK report is difficult to figure
out by just doing a glance analysis. :)
Linus also suggested to use bool as the base type i.e., `bool x:1` but
again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.
And since this issue is added to checkpatch now, very likely there would
be blast of patches sent on the same.
Not everyone who sends checkpatch would be able to disassemble or test
on different implementations.
But if anyone is interested check this:
https://godbolt.org/
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sat, 27 Oct 2018, Joe Perches wrote:
> On Fri, 2018-10-26 at 22:54 +0200, Julia Lawall wrote:
> > [Adding Joe Perches]
> >
> > On Fri, 26 Oct 2018, Sasha Levin wrote:
> >
> > > On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > > > This change was suggested by checkpath.pl. Use unsigned int with
> > > > bitfield
> > > > allocate only one bit to the boolean variable.
> > > >
> > > > CHECK: Avoid using bool structure members because of possible alignment
> > > > issues
> > > >
> > > > Signed-off-by: Shayenne da Luz Moura
> > > > ---
> > > > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > > > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > > > b/drivers/staging/vboxvideo/vbox_drv.h
> > > > index 594f84272957..7d3e329a6b1c 100644
> > > > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > > > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > > > @@ -81,7 +81,7 @@ struct vbox_private {
> > > > u8 __iomem *vbva_buffers;
> > > > struct gen_pool *guest_pool;
> > > > struct vbva_buf_ctx *vbva_info;
> > > > - bool any_pitch;
> > > > + unsigned int any_pitch:1;
> > > > u32 num_crtcs;
> > > > /** Amount of available VRAM, including space used for buffers.
> > > > */
> > > > u32 full_vram_size;
> > >
> > > Using bitfields for booleans in these cases is less efficient than just
> > > using "regular" booleans for two reasons:
> > >
> > > 1. It will use the same amount of space. Due to alignment requirements,
> > > the compiler can't squeeze in anything into the 7 bits that are now
> > > "free". Each member, unless it's another bitfield, must start at a whole
> > > byte.
Since this is between a pointer and a u32, won't the compiler put a lot
more padding, in both cases?
> > >
> > > 2. This is actually less efficient (slower) for the compiler to work
> > > with. The smallest granularity we have to access memory is 1 byte; we
> > > can't set individual bits directly in memory. For the original code, the
> > > assembly for 'vbox_private.any_pitch = true' would look something like
> > > this:
> > >
> > > movl $0x1,-0x10(%rsp)
> > >
> > > As you can see, the compiler can directly write into the variable.
> > > However, when we switch to using bitfields, the compiler must preserve
> > > the original value of the other 7 bits, so it must first read them from
> > > memory, manipulate the value and write it back. The assembly would
> > > look something like this:
> > >
> > > movzbl -0x10(%rsp),%eax
> > > or $0x1,%eax
> > > mov%al,-0x10(%rsp)
> > >
> > > Which is less efficient than what was previously happening.
> >
> > Maybe checkpatch could be more precise about what kind of bools should be
> > changed?
>
> Probably so, what verbiage would you suggest?
I don't know what are the conditions. Sasha?
julia
> Also, any conversion from bool to int would
> have to take care than any assigment uses !!
> where appropriate.
>
>
>
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Sat, 27 Oct 2018, Joe Perches wrote:
> On Fri, 2018-10-26 at 22:54 +0200, Julia Lawall wrote:
> > [Adding Joe Perches]
> >
> > On Fri, 26 Oct 2018, Sasha Levin wrote:
> >
> > > On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > > > This change was suggested by checkpath.pl. Use unsigned int with
> > > > bitfield
> > > > allocate only one bit to the boolean variable.
> > > >
> > > > CHECK: Avoid using bool structure members because of possible alignment
> > > > issues
> > > >
> > > > Signed-off-by: Shayenne da Luz Moura
> > > > ---
> > > > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > > > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > > > b/drivers/staging/vboxvideo/vbox_drv.h
> > > > index 594f84272957..7d3e329a6b1c 100644
> > > > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > > > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > > > @@ -81,7 +81,7 @@ struct vbox_private {
> > > > u8 __iomem *vbva_buffers;
> > > > struct gen_pool *guest_pool;
> > > > struct vbva_buf_ctx *vbva_info;
> > > > - bool any_pitch;
> > > > + unsigned int any_pitch:1;
> > > > u32 num_crtcs;
> > > > /** Amount of available VRAM, including space used for buffers.
> > > > */
> > > > u32 full_vram_size;
> > >
> > > Using bitfields for booleans in these cases is less efficient than just
> > > using "regular" booleans for two reasons:
> > >
> > > 1. It will use the same amount of space. Due to alignment requirements,
> > > the compiler can't squeeze in anything into the 7 bits that are now
> > > "free". Each member, unless it's another bitfield, must start at a whole
> > > byte.
Since this is between a pointer and a u32, won't the compiler put a lot
more padding, in both cases?
> > >
> > > 2. This is actually less efficient (slower) for the compiler to work
> > > with. The smallest granularity we have to access memory is 1 byte; we
> > > can't set individual bits directly in memory. For the original code, the
> > > assembly for 'vbox_private.any_pitch = true' would look something like
> > > this:
> > >
> > > movl $0x1,-0x10(%rsp)
> > >
> > > As you can see, the compiler can directly write into the variable.
> > > However, when we switch to using bitfields, the compiler must preserve
> > > the original value of the other 7 bits, so it must first read them from
> > > memory, manipulate the value and write it back. The assembly would
> > > look something like this:
> > >
> > > movzbl -0x10(%rsp),%eax
> > > or $0x1,%eax
> > > mov%al,-0x10(%rsp)
> > >
> > > Which is less efficient than what was previously happening.
> >
> > Maybe checkpatch could be more precise about what kind of bools should be
> > changed?
>
> Probably so, what verbiage would you suggest?
I don't know what are the conditions. Sasha?
julia
> Also, any conversion from bool to int would
> have to take care than any assigment uses !!
> where appropriate.
>
>
>
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Fri, 2018-10-26 at 22:54 +0200, Julia Lawall wrote:
> [Adding Joe Perches]
>
> On Fri, 26 Oct 2018, Sasha Levin wrote:
>
> > On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > > allocate only one bit to the boolean variable.
> > >
> > > CHECK: Avoid using bool structure members because of possible alignment
> > > issues
> > >
> > > Signed-off-by: Shayenne da Luz Moura
> > > ---
> > > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > > b/drivers/staging/vboxvideo/vbox_drv.h
> > > index 594f84272957..7d3e329a6b1c 100644
> > > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > > @@ -81,7 +81,7 @@ struct vbox_private {
> > > u8 __iomem *vbva_buffers;
> > > struct gen_pool *guest_pool;
> > > struct vbva_buf_ctx *vbva_info;
> > > - bool any_pitch;
> > > + unsigned int any_pitch:1;
> > > u32 num_crtcs;
> > > /** Amount of available VRAM, including space used for buffers. */
> > > u32 full_vram_size;
> >
> > Using bitfields for booleans in these cases is less efficient than just
> > using "regular" booleans for two reasons:
> >
> > 1. It will use the same amount of space. Due to alignment requirements,
> > the compiler can't squeeze in anything into the 7 bits that are now
> > "free". Each member, unless it's another bitfield, must start at a whole
> > byte.
> >
> > 2. This is actually less efficient (slower) for the compiler to work
> > with. The smallest granularity we have to access memory is 1 byte; we
> > can't set individual bits directly in memory. For the original code, the
> > assembly for 'vbox_private.any_pitch = true' would look something like
> > this:
> >
> > movl $0x1,-0x10(%rsp)
> >
> > As you can see, the compiler can directly write into the variable.
> > However, when we switch to using bitfields, the compiler must preserve
> > the original value of the other 7 bits, so it must first read them from
> > memory, manipulate the value and write it back. The assembly would
> > look something like this:
> >
> > movzbl -0x10(%rsp),%eax
> > or $0x1,%eax
> > mov%al,-0x10(%rsp)
> >
> > Which is less efficient than what was previously happening.
>
> Maybe checkpatch could be more precise about what kind of bools should be
> changed?
Probably so, what verbiage would you suggest?
Also, any conversion from bool to int would
have to take care than any assigment uses !!
where appropriate.
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Fri, 2018-10-26 at 22:54 +0200, Julia Lawall wrote:
> [Adding Joe Perches]
>
> On Fri, 26 Oct 2018, Sasha Levin wrote:
>
> > On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > > allocate only one bit to the boolean variable.
> > >
> > > CHECK: Avoid using bool structure members because of possible alignment
> > > issues
> > >
> > > Signed-off-by: Shayenne da Luz Moura
> > > ---
> > > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > > 2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > > b/drivers/staging/vboxvideo/vbox_drv.h
> > > index 594f84272957..7d3e329a6b1c 100644
> > > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > > @@ -81,7 +81,7 @@ struct vbox_private {
> > > u8 __iomem *vbva_buffers;
> > > struct gen_pool *guest_pool;
> > > struct vbva_buf_ctx *vbva_info;
> > > - bool any_pitch;
> > > + unsigned int any_pitch:1;
> > > u32 num_crtcs;
> > > /** Amount of available VRAM, including space used for buffers. */
> > > u32 full_vram_size;
> >
> > Using bitfields for booleans in these cases is less efficient than just
> > using "regular" booleans for two reasons:
> >
> > 1. It will use the same amount of space. Due to alignment requirements,
> > the compiler can't squeeze in anything into the 7 bits that are now
> > "free". Each member, unless it's another bitfield, must start at a whole
> > byte.
> >
> > 2. This is actually less efficient (slower) for the compiler to work
> > with. The smallest granularity we have to access memory is 1 byte; we
> > can't set individual bits directly in memory. For the original code, the
> > assembly for 'vbox_private.any_pitch = true' would look something like
> > this:
> >
> > movl $0x1,-0x10(%rsp)
> >
> > As you can see, the compiler can directly write into the variable.
> > However, when we switch to using bitfields, the compiler must preserve
> > the original value of the other 7 bits, so it must first read them from
> > memory, manipulate the value and write it back. The assembly would
> > look something like this:
> >
> > movzbl -0x10(%rsp),%eax
> > or $0x1,%eax
> > mov%al,-0x10(%rsp)
> >
> > Which is less efficient than what was previously happening.
>
> Maybe checkpatch could be more precise about what kind of bools should be
> changed?
Probably so, what verbiage would you suggest?
Also, any conversion from bool to int would
have to take care than any assigment uses !!
where appropriate.
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
[Adding Joe Perches]
On Fri, 26 Oct 2018, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
>
> 2. This is actually less efficient (slower) for the compiler to work
> with. The smallest granularity we have to access memory is 1 byte; we
> can't set individual bits directly in memory. For the original code, the
> assembly for 'vbox_private.any_pitch = true' would look something like
> this:
>
> movl $0x1,-0x10(%rsp)
>
> As you can see, the compiler can directly write into the variable.
> However, when we switch to using bitfields, the compiler must preserve
> the original value of the other 7 bits, so it must first read them from
> memory, manipulate the value and write it back. The assembly would
> look something like this:
>
> movzbl -0x10(%rsp),%eax
> or $0x1,%eax
> mov%al,-0x10(%rsp)
>
> Which is less efficient than what was previously happening.
Maybe checkpatch could be more precise about what kind of bools should be
changed?
julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
[Adding Joe Perches]
On Fri, 26 Oct 2018, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> >
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> >
> > Signed-off-by: Shayenne da Luz Moura
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
> > drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h
> > b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > u8 __iomem *vbva_buffers;
> > struct gen_pool *guest_pool;
> > struct vbva_buf_ctx *vbva_info;
> > - bool any_pitch;
> > + unsigned int any_pitch:1;
> > u32 num_crtcs;
> > /** Amount of available VRAM, including space used for buffers. */
> > u32 full_vram_size;
>
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
>
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.
>
> 2. This is actually less efficient (slower) for the compiler to work
> with. The smallest granularity we have to access memory is 1 byte; we
> can't set individual bits directly in memory. For the original code, the
> assembly for 'vbox_private.any_pitch = true' would look something like
> this:
>
> movl $0x1,-0x10(%rsp)
>
> As you can see, the compiler can directly write into the variable.
> However, when we switch to using bitfields, the compiler must preserve
> the original value of the other 7 bits, so it must first read them from
> memory, manipulate the value and write it back. The assembly would
> look something like this:
>
> movzbl -0x10(%rsp),%eax
> or $0x1,%eax
> mov%al,-0x10(%rsp)
>
> Which is less efficient than what was previously happening.
Maybe checkpatch could be more precise about what kind of bools should be
changed?
julia
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
This change was suggested by checkpath.pl. Use unsigned int with bitfield
allocate only one bit to the boolean variable.
CHECK: Avoid using bool structure members because of possible alignment
issues
Signed-off-by: Shayenne da Luz Moura
---
drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/vboxvideo/vbox_drv.h
b/drivers/staging/vboxvideo/vbox_drv.h
index 594f84272957..7d3e329a6b1c 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -81,7 +81,7 @@ struct vbox_private {
u8 __iomem *vbva_buffers;
struct gen_pool *guest_pool;
struct vbva_buf_ctx *vbva_info;
- bool any_pitch;
+ unsigned int any_pitch:1;
u32 num_crtcs;
/** Amount of available VRAM, including space used for buffers. */
u32 full_vram_size;
Using bitfields for booleans in these cases is less efficient than just
using "regular" booleans for two reasons:
1. It will use the same amount of space. Due to alignment requirements,
the compiler can't squeeze in anything into the 7 bits that are now
"free". Each member, unless it's another bitfield, must start at a whole
byte.
2. This is actually less efficient (slower) for the compiler to work
with. The smallest granularity we have to access memory is 1 byte; we
can't set individual bits directly in memory. For the original code, the
assembly for 'vbox_private.any_pitch = true' would look something like
this:
movl $0x1,-0x10(%rsp)
As you can see, the compiler can directly write into the variable.
However, when we switch to using bitfields, the compiler must preserve
the original value of the other 7 bits, so it must first read them from
memory, manipulate the value and write it back. The assembly would
look something like this:
movzbl -0x10(%rsp),%eax
or $0x1,%eax
mov%al,-0x10(%rsp)
Which is less efficient than what was previously happening.
--
Thanks,
Sasha
Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool
On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
This change was suggested by checkpath.pl. Use unsigned int with bitfield
allocate only one bit to the boolean variable.
CHECK: Avoid using bool structure members because of possible alignment
issues
Signed-off-by: Shayenne da Luz Moura
---
drivers/staging/vboxvideo/vbox_drv.h| 14 +++---
drivers/staging/vboxvideo/vboxvideo_guest.h | 2 +-
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/vboxvideo/vbox_drv.h
b/drivers/staging/vboxvideo/vbox_drv.h
index 594f84272957..7d3e329a6b1c 100644
--- a/drivers/staging/vboxvideo/vbox_drv.h
+++ b/drivers/staging/vboxvideo/vbox_drv.h
@@ -81,7 +81,7 @@ struct vbox_private {
u8 __iomem *vbva_buffers;
struct gen_pool *guest_pool;
struct vbva_buf_ctx *vbva_info;
- bool any_pitch;
+ unsigned int any_pitch:1;
u32 num_crtcs;
/** Amount of available VRAM, including space used for buffers. */
u32 full_vram_size;
Using bitfields for booleans in these cases is less efficient than just
using "regular" booleans for two reasons:
1. It will use the same amount of space. Due to alignment requirements,
the compiler can't squeeze in anything into the 7 bits that are now
"free". Each member, unless it's another bitfield, must start at a whole
byte.
2. This is actually less efficient (slower) for the compiler to work
with. The smallest granularity we have to access memory is 1 byte; we
can't set individual bits directly in memory. For the original code, the
assembly for 'vbox_private.any_pitch = true' would look something like
this:
movl $0x1,-0x10(%rsp)
As you can see, the compiler can directly write into the variable.
However, when we switch to using bitfields, the compiler must preserve
the original value of the other 7 bits, so it must first read them from
memory, manipulate the value and write it back. The assembly would
look something like this:
movzbl -0x10(%rsp),%eax
or $0x1,%eax
mov%al,-0x10(%rsp)
Which is less efficient than what was previously happening.
--
Thanks,
Sasha
